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(amplify-codegen): swift - add has-many associatedFields for CPK use case #538

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lawmicha
Copy link
Member

@lawmicha lawmicha commented Feb 2, 2023

Table of contents

Tagged release tagged-release-without-e2e-tests/cpk-has-many

Description of changes

This PR is a bug fix for CPK feature to add the foreign key fields of a has-many relationship as the "associatedFields" of the field. For uni-directional has-many relationships, with the parent having a CPK use case:

# LL.7. Implicit Uni-directional Has Many
# CLI.4. Implicit Uni-directional Has Many

type Post4 @model {
  postId: ID! @primaryKey(sortKeyFields:["title"])
  title: String!
  comments: [Comment4] @hasMany
}
type Comment4 @model {
  commentId: ID! @primaryKey(sortKeyFields:["content"])
  content: String!
}

# LL.11. Explicit Uni-directional Has Many
# CLI.8. Explicit Uni-directional Has Many

type Post8 @model {
  postId: ID! @primaryKey(sortKeyFields:["title"])
  title: String!
  comments: [Comment8] @hasMany(indexName:"byPost", fields:["postId", "title"])
}
type Comment8 @model {
  commentId: ID! @primaryKey(sortKeyFields:["content"])
  content: String!
  postId: ID @index(name: "byPost", sortKeyFields:["postTitle"]) # customized foreign key for parent primary key
  postTitle: String # customized foreign key for parent sort key
}
  • The generated swift schema file for Comment does not have information about which fields are foreign keys to the Post. "postId" and "postTitle" are string fields.
  • The parent model, Post, has a reference to the hasMany Comments. This reference has an associatedWith field which points to the primary key of the Post, ie.
.hasMany(post4.comments, is: .optional, ofType: Comment4.self, associatedWith: Comment4.keys.post4CommentsPostId),

The issue here is that it's missing the information about the entire CPK of Post, the library only knows that the comment is associated with the post by the field Comment4.keys.post4CommentsPostId on Comment.

By replacing associatedWith with associatedFields hasMany schema information to include all the foreign key composite key, we support CPK uni-directional has-many use cases. The final generated schema information will appear like this:

.hasMany(post4.comments, is: .optional, ofType: Comment4.self, associatedFields: [Comment4.keys.post4CommentsPostId, Comment4.keys.post4CommentsTitle]),

This is useful for querying for Comments by using associatedFields, done by the library's lazy loading list functionality. For more details on the library issue, see aws-amplify/amplify-swift#2730

Since this codegen change is behind the CPK feature flag and generates "associatedFields" on the hasMany, this means the library needs to define and release first, followed by the codegen changes. Since CPK feature is available for both v1 and main (v2) of Amplify Swift library. We need to introduce the changes to v1, main, and not just data-dev-preview (lazy reference + custom selecion set feature branch) of the library.

Issue #, if available

Description of how you validated changes

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • Breaking changes to existing customers are released behind a feature flag or major version update
  • Changes are tested using sample applications for all relevant platforms (iOS/android/flutter/Javascript) that use the feature added/modified

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

@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2023

Codecov Report

Merging #538 (1cbf6b1) into main (cf7f167) will increase coverage by 0.00%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main     #538   +/-   ##
=======================================
  Coverage   85.69%   85.70%           
=======================================
  Files         148      148           
  Lines        7380     7385    +5     
  Branches     1962     1965    +3     
=======================================
+ Hits         6324     6329    +5     
  Misses        959      959           
  Partials       97       97           
Impacted Files Coverage Δ
...elgen-plugin/src/visitors/appsync-swift-visitor.ts 97.00% <100.00%> (+0.03%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@lawmicha lawmicha force-pushed the lawmicha.cpk-hasmany-targetnames branch 2 times, most recently from 5b7f190 to c4234fc Compare February 2, 2023 21:48
@lawmicha lawmicha changed the title fix(amplify-codegen): swift - add has-many targetNames for CPK use case fix(amplify-codegen): swift - add has-many associatedWithFields for CPK use case Feb 2, 2023
@lawmicha lawmicha force-pushed the lawmicha.cpk-hasmany-targetnames branch from c4234fc to d8bb5d1 Compare February 2, 2023 22:14
@lawmicha lawmicha changed the title fix(amplify-codegen): swift - add has-many associatedWithFields for CPK use case fix(amplify-codegen): swift - add has-many associatedFields for CPK use case Feb 2, 2023
@lawmicha lawmicha force-pushed the lawmicha.cpk-hasmany-targetnames branch from d8bb5d1 to 24e4aed Compare February 2, 2023 23:00
model.fields(
.field(post.id, is: .required, ofType: .string),
.field(post.title, is: .required, ofType: .string),
.hasMany(post.comments, is: .optional, ofType: Comment.self, associatedFields: [Comment.keys.post]),
Copy link
Member Author

Choose a reason for hiding this comment

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

With CPK enabled, the bidirectional use case will now only generated associatedFields, which means the library changes must be merged and released first. Comment.keys.post is correct since, the reference field post on Comment can be used to resolve its targetNames, from the Comment schema

.belongsTo(comment.post, is: .optional, ofType: Post.self, targetNames: [\\"postCommentsId\\", \\"postCommentsTitle\\"]),

@lawmicha lawmicha force-pushed the lawmicha.cpk-hasmany-targetnames branch from cf38966 to 1cbf6b1 Compare February 3, 2023 00:07
model.fields(
.field(post.postId, is: .required, ofType: .string),
.field(post.title, is: .required, ofType: .string),
.hasMany(post.comments, is: .optional, ofType: Comment.self, associatedFields: [Comment.keys.postId]),
Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't look accurate

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened issue to track this: #539

Copy link
Contributor

Choose a reason for hiding this comment

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

PR for the fix #542

@@ -73,7 +309,7 @@ extension Post {
model.fields(
.field(post.id, is: .required, ofType: .string),
.field(post.title, is: .required, ofType: .string),
.hasMany(post.comments, is: .optional, ofType: Comment.self, associatedWith: Comment.keys.postCommentsId),
.hasMany(post.comments, is: .optional, ofType: Comment.self, associatedFields: [Comment.keys.postCommentsId, Comment.keys.postCommentsTitle]),
Copy link
Member Author

Choose a reason for hiding this comment

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

this looks correct

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants