Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(DataStore): QueryPredicate translation #961

Merged
merged 6 commits into from
Dec 23, 2020
Merged

fix(DataStore): QueryPredicate translation #961

merged 6 commits into from
Dec 23, 2020

Conversation

ruiguoamz
Copy link
Contributor

Issue #, if available:
A fix for issue: #907

Description of changes:
The previous translation from QueryPredicate to sql statement is incorrect

For example:
If I have a predicate like

let predicate = (todo.name == "name2" && todo.tag == "optional") || todo.description == "wake up"

SQL statement generated by the incorrect code is:

select
  "root"."id" as "id", "root"."description" as "description", "root"."name" as "name",
  "root"."tag" as "tag"
from Todo as "root"
where 1 = 1
  and (
    or (
      "root"."name" = ?
      and "root"."tag" = ?
    )
    and "root"."description" = ?
  )

which fails DataStore.query()

The correct one should be:

select
  "root"."id" as "id", "root"."description" as "description", "root"."name" as "name",
  "root"."tag" as "tag"
from Todo as "root"
where 1 = 1
  and (
    (
      "root"."name" = ?
      and "root"."tag" = ?
    )
    or "root"."description" = ?
  )

My PR update the implementation of ConditionStatement generation
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ruiguoamz ruiguoamz added the datastore Issues related to the DataStore category label Dec 10, 2020
@ruiguoamz ruiguoamz self-assigned this Dec 10, 2020
@ruiguoamz ruiguoamz changed the title PR for fixing nested sql statement fix(DataStore): QueryPredicate translation Dec 10, 2020
@codecov
Copy link

codecov bot commented Dec 10, 2020 •
edited
Loading

Codecov Report

Merging #961 (16379d0) into main (6fb4841) will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #961      +/-   ##
==========================================
+ Coverage   70.39%   70.50%   +0.10%     
==========================================
  Files         920      920              
  Lines       39049    39194     +145     
==========================================
+ Hits        27490    27632     +142     
- Misses      11559    11562       +3
Flag Coverage Δ
API_plugin_unit_test 64.02% <ø> (-0.08%) ⬇️
Analytics_plugin_unit_test 72.38% <ø> (ø)
Auth_plugin_unit_test 75.13% <ø> (ø)
DataStore_plugin_unit_test 83.90% <100.00%> (+0.21%) ⬆️
Predictions_plugin_unit_test 50.43% <ø> (ø)
Storage_plugin_unit_test 74.74% <ø> (ø)
build_test_amplify 62.38% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...Plugin/Storage/SQLite/SQLStatement+Condition.swift 100.00% <100.00%> (ø)
...reCategoryPluginTests/Core/SQLStatementTests.swift 100.00% <100.00%> (ø)
...lugin/Operation/AWSAPIOperation+APIOperation.swift 78.37% <0.00%> (-5.41%) ⬇️
.../DataStore/Model/Internal/Schema/ModelSchema.swift 85.29% <0.00%> (-2.95%) ⬇️
...Hub/DefaultPluginTests/DefaultHubPluginTests.swift 87.20% <0.00%> (-1.17%) ⬇️
...s/AWSHubPlugin/Internal/HubChannelDispatcher.swift 97.05% <0.00%> (+2.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fb4841...f5aa6e0. Read the comment docs.

ruiguoamz
ruiguoamz commented Dec 14, 2020
View reviewed changes
AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/SQLite/SQLStatement+Condition.swift Outdated
let indentPrefix = " "
var indentSize = 1
var groupOpened = false

func translate(_ pred: QueryPredicate) {
func translate(_ pred: QueryPredicate, opAhead: Bool, groupType: QueryPredicateGroupType) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • opAhead: decide whether and or or operator should be added before bracket depends on

  • groupType: the operation applied to two QueryPredicateOperation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you have to explain the variable name here then consider renaming the variable and refactor as you try to rename it. If the variable still needs some explanation, then better add some documentation to the top of the function

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the android implementation, it looks pretty elegant:
https://github.com/aws-amplify/amplify-android/blob/fef0cc31b50c427c60e144346a896c20a2f440e4/aws-datastore/src/main/java/com/amplifyframework/datastore/storage/sqlite/adapter/SQLPredicate.java#L108

Maybe we can get some inspiration from their codebase?

@ruiguoamz ruiguoamz marked this pull request as ready for review December 14, 2020 05:45
@ruiguoamz ruiguoamz requested review from drochetti and a team December 14, 2020 05:46
lawmicha
lawmicha reviewed Dec 23, 2020
View reviewed changes
Copy link
Contributor

@lawmicha lawmicha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any tests at the unit test or integration test level that would run against a real SQL store to ensure the issue is fixed ?

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/SQLite/SQLStatement+Condition.swift Outdated
@@ -57,7 +64,7 @@ private func translateQueryPredicate(from modelSchema: ModelSchema,
}
}
}
translate(predicate)
translate(predicate, predicateIndex: -1, groupType: .and) //
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could also have default parameter values as -1 and .and for the translate method and comment on top why the default values are used. or add the comment on top here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added some descriptive words

ruiguoamz
ruiguoamz commented Dec 23, 2020
View reviewed changes
...yPlugins/DataStore/AWSDataStoreCategoryPluginIntegrationTests/DataStoreLocalStoreTests.swift
class DataStoreLocalStoreTests: LocalStoreIntegrationTestBase {

/// - Given: 4 posts that has been saved
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added one integration test

lawmicha
lawmicha approved these changes Dec 23, 2020
View reviewed changes
Copy link
Contributor

@lawmicha lawmicha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ruiguoamz ruiguoamz merged commit 9e32cf7 into main Dec 23, 2020
@ruiguoamz ruiguoamz deleted the fix/nestedsql branch December 23, 2020 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wooj2 wooj2 wooj2 left review comments

@lawmicha lawmicha lawmicha approved these changes

@drochetti drochetti Awaiting requested review from drochetti

Assignees

@ruiguoamz ruiguoamz

Labels
datastore Issues related to the DataStore category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ruiguoamz @lawmicha @wooj2