fix: add targetName to hasOne relationships #926

lawmicha · 2020-12-01T22:58:44Z

Issue #, if available:
#920

Description of changes:
The fix in this PR is enabled by the CLI codegen changes which adds the targetName to hasOne relationships so that this becomes an additive, non-breaking change for developers.

The fix targets making the following schema usable by API, and thus DataStore when sync is enabled:

# 2 - Project with explicit field for team’s id
type Project2 @model {
  id: ID!
  name: String
  teamID: ID!
  team: Team2 @connection(fields: ["teamID"])
}

type Team2 @model {
  id: ID!
  name: String!
}

See #920 for more details

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

codecov · 2020-12-02T21:57:34Z

Codecov Report

Merging #926 (58449ea) into main (d49854f) will decrease coverage by 0.23%.
The diff coverage is 10.00%.

@@            Coverage Diff             @@
##             main     #926      +/-   ##
==========================================
- Coverage   67.29%   67.05%   -0.24%     
==========================================
  Files         895      917      +22     
  Lines       35389    35532     +143     
==========================================
+ Hits        23814    23826      +12     
- Misses      11575    11706     +131

Flag	Coverage Δ
API_plugin_unit_test	`62.59% <80.00%> (+0.03%)`	⬆️
Analytics_plugin_unit_test	`72.38% <ø> (ø)`
Auth_plugin_unit_test	`48.62% <ø> (ø)`
DataStore_plugin_unit_test	`83.03% <ø> (ø)`
Predictions_plugin_unit_test	`49.69% <ø> (ø)`
Storage_plugin_unit_test	`74.74% <ø> (ø)`
build_test_amplify	`62.31% <7.58%> (-0.81%)`	⬇️

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

Impacted Files	Coverage Δ
...Model/Internal/Schema/ModelSchema+Definition.swift	`61.40% <0.00%> (ø)`
...stCommon/Models/Collection/1/Project1+Schema.swift	`0.00% <0.00%> (ø)`
...plifyTestCommon/Models/Collection/1/Project1.swift	`0.00% <0.00%> (ø)`
...yTestCommon/Models/Collection/1/Team1+Schema.swift	`0.00% <0.00%> (ø)`
AmplifyTestCommon/Models/Collection/1/Team1.swift	`0.00% <0.00%> (ø)`
...stCommon/Models/Collection/2/Project2+Schema.swift	`0.00% <0.00%> (ø)`
...plifyTestCommon/Models/Collection/2/Project2.swift	`0.00% <0.00%> (ø)`
...yTestCommon/Models/Collection/2/Team2+Schema.swift	`0.00% <0.00%> (ø)`
AmplifyTestCommon/Models/Collection/2/Team2.swift	`0.00% <0.00%> (ø)`
...stCommon/Models/Collection/3/Comment3+Schema.swift	`0.00% <0.00%> (ø)`
... and 42 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 d49854f...58449ea. Read the comment docs.

.../AWSAPICategoryPluginFunctionalTests/GraphQLModelBased/GraphQLConnectionScenario1Tests.swift

AmplifyPlugins/Core/AWSPluginsCore/Model/Support/Model+GraphQL.swift

palpatim · 2020-12-02T22:44:26Z

AmplifyPlugins/Core/AWSPluginsCore/Model/Support/Model+GraphQL.swift

+    // When the associated object is found, take this value over the explicit field's value by replacing the correct
+    // entry for the field name of the associated model.


I'm not necessarily disagreeing, but why is it right to take the object rather than the specified field? How do we let the customer know which one will be used?

If they do any of the below, the correct team identifier will be used

let project = Project2(teamID: team.id, team: team) let project = Project2(teamID: team.id) // note `teamID` is always required

So the decision to take the object over the specified field only matters when the developer sets their values differently, ie.

let project = Project2(teamID: "nonExistentTeamID", team: team)

If the developer started off with

let project = Project2(teamID: "nonExistentTeamID")

This will be persisted but the association points to an non existent team. They'll eventually end up with one of the three scenarios once they get working code, by persisting/retrieving the team object before using it in the project object

let project = Project2(teamID: team.id) // 1. they updated their code to use team.id, and it works without passing in an optional `team` let project = Project2(teamID: team.id, team: team) // 2. they updated it in both places, because it's confusing where to add it to let project = Project2(teamID: "nonExistentTeamID", team: team) // 3. they only passed it into `team` parameter, but it starts working

Another thought I had for this was thinking that we will omit the specific field in the future with a breaking codegen change, so that our documentation always promotes the use of passing the object (since belongsTo associations already omit the specific field). So where it's not omitted, we would continue to document it as let project = Project2(teamID: team.id, team: team) to communicate that developers should work with the model, while satisfying requirement that teamID is a required field

palpatim · 2020-12-02T22:47:09Z

AmplifyPlugins/Core/AWSPluginsCore/Model/Support/Model+GraphQL.swift

+
+        return nil
+    }
+


Thanks for this refactor!

AmplifyPlugins/Core/AWSPluginsCore/Model/Support/Model+GraphQL.swift

lawmicha · 2020-12-04T17:31:53Z

...WSDataStoreCategoryPluginIntegrationTests/Connection/DataStoreConnectionScenario3Tests.swift

+        wait(for: [queriedCommentCompleted], timeout: networkTimeout)
+    }
+
+    // TODO: Issue https://github.com/aws-amplify/amplify-ios/issues/939


Capturing the issue separately: #939

Amplify/Categories/DataStore/Model/Internal/Schema/ModelField+Association.swift

AmplifyTests/CategoryTests/DataStore/ModelFieldAssociationTests.swift

royjit · 2020-12-04T21:40:38Z

AmplifyPlugins/Core/AWSPluginsCore/Model/Support/Model+GraphQL.swift

+    /// This information is also stored in the schema as `targetName` which is codegenerated to be the same as the
+    /// default or an explicit field specified by the developer.
+    private func getFieldNameForAssociatedModels(modelField: ModelField) -> String {
+        let defaultFieldName = modelName.camelCased() + modelField.name + "Id"


"Id" was not added before this change.. is this a bug fix ? Earlier it was modelName.camelCased() + field.graphQLName

graphQLName internally resolves to name.pascalCased() + "Id" for associations. oh shoot. this is incorrect then it should be:
modelName.camelCased() + modelField.name.pascalCased() + "Id"

updated to correct association field, we always pascal case the modeField.name for the associated field

Cool! Wondering whether we have coverage on this logic. It would nice if you can add this or add a backlog to add this logic to unit test.

royjit · 2020-12-04T21:47:04Z

AmplifyPlugins/Core/AWSPluginsCore/Model/Support/Model+GraphQL.swift

-            guard let fieldValue = fieldOptionalValue else {
-                input.updateValue(nil, forKey: name)
+            guard let fieldValue = fieldValueOptional else {
+                input.updateValue(nil, forKey: modelField.name)


We used field.graphQLName before? Just confirming both works?

graphQLName returns name by default, and only in the case it's a belongsTo it will return name.pascalCase()+"Id". this value name.pascalCase() + "Id" is not valid until we've prepended the current model name, and is done in the case model scenario. which is why i've refactored this to just modelField.name and consolidated the logic down below

https://github.com/aws-amplify/amplify-ios/blob/main/AmplifyPlugins/Core/AWSPluginsCore/Model/Support/ModelField+GraphQL.swift

if it is belongsTo and returns targetName because targetName exists, then it may set targetName to nil here. I don't think this is ever the case because you cannot have nil belongsTo assocations, but i will revert this change and only make it if we're sure that there isn't a scenario that i've missed

reverted this change

royjit · 2020-12-04T21:49:19Z

AmplifyPlugins/Core/AWSPluginsCore/Model/Support/Model+GraphQL.swift

+            return primaryKeyValue
+        }
+
+        preconditionFailure("Could not retrieve the identifier from associated model: \(value)")


Hope this wont break? Previous code returned nil here.

i wrote this on the assumption that it should always get an id from the value, but i'm more inclined to return nil here to introduce less changes to the code

royjit · 2020-12-04T21:53:53Z

AmplifyPlugins/Core/AWSPluginsCore/Model/GraphQLRequest/GraphQLRequest+AnyModelWithSync.swift

@@ -64,13 +64,13 @@ extension GraphQLRequest: ModelSyncGraphQLRequestFactory {
                                                   decodePath: document.name)
    }

-    public static func createMutation(of model: Model, version: Int?) -> GraphQLRequest<MutationSyncResult> {


Why do we need these changes?

I noticed in the integration tests over in AWSAPIPlugin that tests these paths, for some reason were not compiling. until i added the default parameters here. Not sure how those were written without the nil default parameter on these methods

lawmicha added 2 commits December 1, 2020 14:54

fix: add targetName to hasOne relationships

e52a222

Add implementation for GraphQLInput translation and tests

c308d07

lawmicha requested a review from palpatim December 2, 2020 22:27

lawmicha marked this pull request as ready for review December 2, 2020 22:27

lawmicha mentioned this pull request Dec 2, 2020

HasOne Relationships missing targetName #920

Closed

palpatim reviewed Dec 2, 2020

View reviewed changes

lawmicha added 2 commits December 2, 2020 16:33

address some PR comments

9db8380

set up and add datastore connection test

3b603a6

lawmicha self-assigned this Dec 3, 2020

Add remaining API and DataStore tests for connection scenarios

f8b1830

lawmicha force-pushed the fix/hasone-targetname branch from a988f65 to f8b1830 Compare December 4, 2020 17:30

lawmicha commented Dec 4, 2020

View reviewed changes

lawmicha mentioned this pull request Dec 4, 2020

DataStore HasMany scenario 3 - Post with Comments Lazy Load failing on "comment3PostIDId" #939

Closed

royjit reviewed Dec 4, 2020

View reviewed changes

Amplify/Categories/DataStore/Model/Internal/Schema/ModelField+Association.swift Outdated Show resolved Hide resolved

palpatim approved these changes Dec 4, 2020

View reviewed changes

backwards compatibility with flutter usage on public APIs

f2cc649

royjit reviewed Dec 4, 2020

View reviewed changes

revert logic changes for graphQLName and nil value for model case

58449ea

royjit approved these changes Dec 5, 2020

View reviewed changes

lawmicha merged commit 97b96df into main Dec 6, 2020

lawmicha deleted the fix/hasone-targetname branch December 6, 2020 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add targetName to hasOne relationships #926

fix: add targetName to hasOne relationships #926

lawmicha commented Dec 1, 2020 •

edited

Loading

codecov bot commented Dec 2, 2020 •

edited

Loading

palpatim Dec 2, 2020

lawmicha Dec 3, 2020 •

edited

Loading

palpatim Dec 2, 2020

lawmicha Dec 4, 2020 •

edited

Loading

royjit Dec 4, 2020

lawmicha Dec 4, 2020

lawmicha Dec 4, 2020

royjit Dec 5, 2020

royjit Dec 4, 2020

lawmicha Dec 4, 2020 •

edited

Loading

lawmicha Dec 4, 2020

lawmicha Dec 4, 2020

royjit Dec 4, 2020

lawmicha Dec 4, 2020

royjit Dec 4, 2020

lawmicha Dec 4, 2020

		// When the associated object is found, take this value over the explicit field's value by replacing the correct
		// entry for the field name of the associated model.

fix: add targetName to hasOne relationships #926

fix: add targetName to hasOne relationships #926

Conversation

lawmicha commented Dec 1, 2020 • edited Loading

codecov bot commented Dec 2, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

lawmicha Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawmicha Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawmicha Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawmicha commented Dec 1, 2020 •

edited

Loading

codecov bot commented Dec 2, 2020 •

edited

Loading

lawmicha Dec 3, 2020 •

edited

Loading

lawmicha Dec 4, 2020 •

edited

Loading

lawmicha Dec 4, 2020 •

edited

Loading