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: CPK uni-directional has-many lazy list load #2730

Merged

Conversation

lawmicha
Copy link
Member

@lawmicha lawmicha commented Feb 2, 2023

Issue #

Table of contents

Description

This PR fixes the use case for lazy loading a list for uni-directional has-many use cases where the parent model contains a custom primary key. A uni-directional has-many use case is the following:

# 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 Post has a custom primary key (postId and title), and the child does not have a model reference to the post.

Querying for a post, and then traversing to the comment, and lazy loading the comment previously was incorrectly constructing the filter (Query for Comments by filter on post identifiers). It was missing the post's second identifier of the composite key.

LazyList's query for comments by postId == post.id

The correct logic is to do

LazyList's query for comments by postId == post.id && postTitle == post.title

This is working for bi-directional since the post reference on the comment can resolve the targetNames (fields on the Comment, which reference the post). It is not working on for uni-directional because the Comment does not have a reference to the post, only the fields of the post's composite key on the comment.

This PR manually modifies the codegenerated types to include the associatedFieldNames on the comments field on the Post. When querying for a post, the associatedFieldNames will be used if available to pass which fields to construct the filter on. This is only necessary when the parent model has a composite primary key. The changes in this PR is backwards compatible with previous codegenerated hasMany relationships which do not contain associatedFieldNames, however the use case can only be solved for the customer when they upgrade to the latest codegen and regenerate the models with the associatedFieldNames.

Examples use cases

Use case: bi-directional has-many belongs-to

Comment belongs-to Post will have a reference like "post"

struct Comment: Model {
  var post: Post
}

associatedFields will be ["post"].
associatedIdentifiers will be ["postId123"]

query(Comment.self, filter: field("post") == associatedIdentifiers[0])

Use case: bi-directional has-many belongs-to with CPK on parent

struct Comment: Model {
  var post: Post
}
extension Comment {
   .belongsTo(comment.post, is: .optional, ofType: Post.self, targetNames: ["postId", "postTitle"]),
}
struct Post: Model {
  var id: //
  var title // title being a primary key of post
}

associatedFields will be ["post"].
associatedIdentifiers will be ["postId123", "title123"]

let columnNames = getColumnNames(["post"]) // ["postId", "postTitle"]
query(Comment.self, filter: field("postId") == "postId123" && field("postTitle") == "title123")

Use case: uni-directional has-many

struct Comment: Model {
  var postId: String
}

struct Post: Model {
  var id: String
}

associatedFields will be ["postId"].
associatedIdentifiers will be ["postId123"]

query(Comment.self, filter: field("postId") == "postId123")

Use case: uni-directional has-many with CPK on parent

This is the use case that has a problem being fixed by this PR (and codegen changes to add associatedFields to the hasMany modelField)

struct Comment: Model {
  var postId: String
  var postTitle: String
}


struct Post: Model {
  var id: String
  var title: String
  var comments: List<Comment>
}
extension Post {
  .hasMany(post.comments, is: .optional, ofType: Comment4.self, associatedFields: [Comment.keys.postId, Comment.keys.postTitle]),
}

associatedFields will be ["postId", "postTitle"].
associatedIdentifiers will be ["postId123", "title123"]

query(Comment.self, filter: field("postId") == "postId123" && field("postTitle") == "title123")

General Checklist

  • Added new tests to cover change, if needed
  • Build succeeds with all target using Swift Package Manager
  • All unit tests pass
  • All integration tests pass
  • Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)
  • Documentation update for the change if required
  • PR title conforms to conventional commit style
  • New or updated tests include Given When Then inline code documentation and are named accordingly testThing_condition_expectation()
  • If breaking change, documentation/changelog update with migration instructions

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

@lawmicha lawmicha requested review from a team as code owners February 2, 2023 18:12
Comment on lines 63 to +64
assertList(comments, state: .isNotLoaded(associatedIdentifiers: [post.postId, post.title],
associatedField: "postId"))
associatedFields: ["postId", "postTitle"]))
Copy link
Member Author

Choose a reason for hiding this comment

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

Updated codegenerated schema files specify the targetNames, which are passed to the lazy list when instantiating it, thus a not loaded list will have the necessary field names of the post, that exist on the Comment, to query for the Comments by post identifiers.

Comment on lines 170 to 179
} else {
// If `targetNames` is > 1, then this is a uni-directional has-many, thus no reference field on the child
// Construct the lazy list based on the primary key values and the corresponding target names
let primaryKeyNames = schema.primaryKey.fields.map { $0.name }
let primaryKeyValues = primaryKeyNames
.map { getValue(from: element, by: path + [$0]) }
.compactMap { $0 }
.map { String(describing: $0) }
return DataStoreListDecoder.lazyInit(associatedIds: primaryKeyValues,
associatedWith: targetNames)
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 is the functional change to pass down the targetNames for lazy loading the uni-directional has-many use case. We have to extract out the values of the primary key values from the parent model, instead of just using the @@primaryKey field value.

Copy link
Member

Choose a reason for hiding this comment

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

doesn't the targetNames only indicate the foreignKey composite key?

Copy link
Member Author

@lawmicha lawmicha Feb 2, 2023

Choose a reason for hiding this comment

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

Yes I believe so, when the targetNames is on the hasMany modelField, they are the foreign keys fields of the parent model on the child model, which is an array due the parent model containing a composite key. For example, on the Post schema, the Post has a has-many reference to the Comments

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

The targetNames represent the "associated with fields". On the Comment model,

struct Comment {
  var post4CommentsPostId: String
  var post4CommentsTitle: String
}

Since the comment does not have a reference object to Post, there is no "post: Post" field on Comment, and the foreign key composite keys are just strings. The proposed change is to add "targetNames" to the parent model's hasMany fields to point to "post4CommentsPostId" and "post4CommentsTitle"

Comment on lines -71 to +70
// This is a bit off, the post.identifier is the CPK while the associated field is just "postId",
// Loading the comments by the post identifier should be
// "query all comments where the predicate is field("@@postForeignKey") == "[postId]#[title]"
// List fetching is broken for this use case "uni directional has-many"
assertList(comments, state: .isNotLoaded(associatedId: post.identifier,
associatedField: "postId"))
assertList(comments, state: .isNotLoaded(associatedIds: [post.postId, post.title],
associatedFields: ["postId", "postTitle"]))
Copy link
Member Author

Choose a reason for hiding this comment

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

Since the Post8+schema.swift codegenerated file has been updated to include targetNames, they are used and passed as "postId" and "postTitle", which are the fields on the Comment8 to build the filter on, when lazy loading

Comment on lines -119 to +118
appSyncAssociatedField: associatedField.name,
appSyncAssociatedFields: modelField.associatedFieldNames,
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 is the API functional change to pass the "targetNames" array or the "associatedKey", see associatedFieldNames

@lawmicha lawmicha force-pushed the data-dev-preview.uni-directional-has-many branch from 64f76ce to 55b43ab Compare February 2, 2023 18:24
@lawmicha lawmicha changed the title fix: uni-directional has-many lazy list load fix: CPK uni-directional has-many lazy list load Feb 2, 2023
@@ -28,7 +28,7 @@ extension Post4 {
model.fields(
.field(post4.postId, is: .required, ofType: .string),
.field(post4.title, is: .required, ofType: .string),
.hasMany(post4.comments, is: .optional, ofType: Comment4.self, associatedWith: Comment4.keys.post4CommentsPostId),
.hasMany(post4.comments, is: .optional, ofType: Comment4.self, associatedWith: Comment4.keys.post4CommentsPostId, targetNames: ["post4CommentsPostId", "post4CommentsTitle"]),
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 is the proposed codegen changes

@lawmicha lawmicha force-pushed the data-dev-preview.uni-directional-has-many branch from 55b43ab to e58490d Compare February 2, 2023 19:49
@lawmicha lawmicha force-pushed the data-dev-preview.uni-directional-has-many branch from 306b207 to db534d5 Compare February 2, 2023 21:29
@lawmicha lawmicha requested a review from 5d February 2, 2023 22:47
@lawmicha lawmicha force-pushed the data-dev-preview.uni-directional-has-many branch 2 times, most recently from 7a30859 to 2e38706 Compare February 3, 2023 13:27
@lawmicha lawmicha force-pushed the data-dev-preview.uni-directional-has-many branch from 2e38706 to 56fc25b Compare February 3, 2023 13:57
Copy link
Member

@5d 5d left a comment

Choose a reason for hiding this comment

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

LGTM!

@lawmicha lawmicha merged commit 42c111e into data-dev-preview Feb 3, 2023
@lawmicha lawmicha deleted the data-dev-preview.uni-directional-has-many branch February 3, 2023 20:48
lawmicha added a commit that referenced this pull request Feb 8, 2023
* fix: uni-directional has-many lazy list load

* remove redundant log

* rename targetNames to use associatedWithFields

* rename associatedWithFields to associatedFields
lawmicha added a commit that referenced this pull request Feb 8, 2023
* fix: uni-directional has-many lazy list load

* remove redundant log

* rename targetNames to use associatedWithFields

* rename associatedWithFields to associatedFields
harsh62 pushed a commit that referenced this pull request Jul 13, 2023
* fix: uni-directional has-many lazy list load

* remove redundant log

* rename targetNames to use associatedWithFields

* rename associatedWithFields to associatedFields
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.

2 participants