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.

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

fix(Model): retrieve correct associated target name #965

Merged
merged 22 commits into from
Jan 6, 2021

Conversation

ruiguoamz
Copy link
Contributor

@ruiguoamz ruiguoamz commented Dec 11, 2020

Issue #, if available:
Alternative of fixing: #512

Description of changes:
An alternative for this PR which was reverted: #885

Moved func modelName(forField: fieldName) to AmplifyPluginsCore so that both API and DataStore can reuse the logic.

  • API: While the getGraphQLFilter(_ modelSchema: ModelSchema) is returned with the correct fieldName
  • DataStore: At the time sql is translated from QueryPredicate, the correct columnName for belongsTo and hasOne field can be retrieved from modelSchema.association. In this case, CLI change is not required.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Dec 11, 2020

Codecov Report

Merging #965 (1795c2b) into main (9e32cf7) will increase coverage by 0.02%.
The diff coverage is 68.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #965      +/-   ##
==========================================
+ Coverage   55.54%   55.57%   +0.02%     
==========================================
  Files         634      634              
  Lines       18373    18401      +28     
==========================================
+ Hits        10206    10227      +21     
- Misses       8167     8174       +7     
Flag Coverage Δ
API_plugin_unit_test 48.09% <ø> (ø)
AWSPluginsCore 71.88% <69.04%> (-0.53%) ⬇️
Amplify 45.87% <0.00%> (+0.12%) ⬆️
Analytics_plugin_unit_test 73.41% <ø> (ø)
Auth_plugin_unit_test 73.60% <ø> (ø)
DataStore_plugin_unit_test 71.01% <83.33%> (-0.03%) ⬇️
Predictions_plugin_unit_test 33.48% <ø> (ø)
Storage_plugin_unit_test 57.72% <ø> (ø)

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

Impacted Files Coverage Δ
...ies/DataStore/Model/Collection/List+LazyLoad.swift 0.00% <0.00%> (ø)
...aphQLRequest/GraphQLRequest+AnyModelWithSync.swift 81.31% <0.00%> (ø)
...re/Model/GraphQLRequest/GraphQLRequest+Model.swift 70.42% <0.00%> (ø)
...yPlugin/Storage/SQLite/QueryPredicate+SQLite.swift 100.00% <ø> (ø)
...goryPlugin/Storage/SQLite/ModelSchema+SQLite.swift 97.05% <50.00%> (-2.95%) ⬇️
...insCore/Model/Support/QueryPredicate+GraphQL.swift 73.80% <76.31%> (-6.20%) ⬇️
...Plugin/Storage/SQLite/SQLStatement+Condition.swift 98.00% <90.00%> (-2.00%) ⬇️
...CategoryPlugin/Operation/AWSGraphQLOperation.swift 60.75% <0.00%> (-2.54%) ⬇️
.../DataStore/Model/Internal/Schema/ModelSchema.swift 88.23% <0.00%> (+2.94%) ⬆️
... and 3 more

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 9e32cf7...1795c2b. Read the comment docs.

@ruiguoamz ruiguoamz changed the title fix(DataStore): Another option for hasMany @connection columnName fix fix(API & DataStore): Another option for belongsTo & hasOne @connection columnName fix Dec 14, 2020
@ruiguoamz ruiguoamz changed the title fix(API & DataStore): Another option for belongsTo & hasOne @connection columnName fix fix(API & DataStore): belongsTo & hasOne @connection columnName fix Dec 14, 2020
@ruiguoamz ruiguoamz changed the title fix(API & DataStore): belongsTo & hasOne @connection columnName fix fix(Model): retrieve correct associated target name Dec 15, 2020
@lawmicha
Copy link
Contributor

looks like there's an extension on ModelField+Schema that already generates the columnName
https://github.com/aws-amplify/amplify-ios/blob/main/AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Storage/SQLite/ModelSchema+SQLite.swift#L42-L73

@lawmicha lawmicha mentioned this pull request Dec 21, 2020
@ruiguoamz ruiguoamz force-pushed the fix/DataStoreBelongsToColumnName branch from 248fad6 to cc1b492 Compare December 21, 2020 22:03
Comment on lines +82 to +83
preconditionFailure(
"Could not find QueryPredicateOperation or QueryPredicateGroup for \(String(describing: self))")
Copy link
Contributor

@lawmicha lawmicha Jan 6, 2021

Choose a reason for hiding this comment

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

no action needed just a thought exercise.. If developers use QueryPredicateConstant.all then it will fail. Is there a way for developers to get into this state?

  1. Calling API directly with a predicate .all and app will crash. This is probably okay since it is not supported to use .all
  2. Is there a scenario which is calling into DataStore, and then it gets synced to the API and fail here? If so, this may be a bigger concern, what should API do when they see a predicate that is ".all"?

Secondly, precondionFailure affects live apps as well. In one of my latest PR's, I've started using assert(false, "message") instead so that it crashes only during debug builds, and not on release builds

@@ -47,11 +70,27 @@ public struct GraphQLFilterConverter {
/// Extension to translate a `QueryPredicate` into a GraphQL query variables object
extension QueryPredicate {

/// - Warning: Although this has `public` access, it is intended for internal use and should not be used directly
/// by host applications. The behavior of this may change without warning.
public func graphQLFilter(for modelSchema: ModelSchema?) -> GraphQLFilter {
Copy link
Contributor

@lawmicha lawmicha Jan 6, 2021

Choose a reason for hiding this comment

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

add doc comment for the public API, i see up above

/// Extension to translate a `QueryPredicate` into a GraphQL query variables object

perhaps this information can be on top of the method as well. just a suggestion since i recall the discussion regarding adding doc comment and warning, etc. something like

/// Retrieve the GraphQL representation of the `QueryPredicate`. This translates the `QueryPredicate` into a 
/// JSON object, useful for passing as the "filter" or "condition" input of the GraphQL variables. For example
/// <Query Predicate example to JSON string>
///
/// - Warning: Although this has `public` access, it is intended for internal use and should not be used directly
///   by host applications. The behavior of this may change without warning.

@ruiguoamz ruiguoamz merged commit b4e0abf into main Jan 6, 2021
@ruiguoamz ruiguoamz deleted the fix/DataStoreBelongsToColumnName branch January 6, 2021 17:33
@diegocstn diegocstn added the pending-release Code has been merged but pending release Code has been merged but pending release label Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending-release Code has been merged but pending release Code has been merged but pending release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants