Android codegen bug: implicit @BelongsTo annotation not being applied to generated child models

[raphkim]

Issue

When the customer creates a GraphQL model schema with either hasOne or hasMany relationship, they are only required to explicitly specify @connection directive on the parent model (they do not need to explicitly mention belongsTo relationship on the child model). However, every relationship must be bi-directional and codegen should still generate child model with belongsTo relationship.

Currently, android codegen does not behave this way and does not generate child model correctly.

Schema for reference:

type Post @model {
  id: ID!
  title: String!
  comments: [Comment] @connection(keyName: "byPost", fields: ["id"])
}

type Comment @model
  @key(name: "byPost", fields: ["postID", "content"]) {
  id: ID!
  postID: ID!
  content: String!
}

This explicitly specifies a connection that Post has many Comment, but does not specify a connection directive on the Comment side. This results in java codegen forming a @HasMany relationship on the Post side but generates a Comment model without @BelongsTo connection to other models. Android DataStore depends on the explicit presence of this annotation when generating foreign key relationships.

Currently behavior:

public final class Post implements Model {
  private final @ModelField(targetType="ID", isRequired = true) String id;
  private final @ModelField(targetType="String", isRequired = true) String title;
  private final @ModelField(targetType="Comment") @HasMany(associatedWith = "postID", type = Comment.class) List<Comment> comments = null;
  ...
}

public final class Comment implements Model {
  private final @ModelField(targetType="ID", isRequired = true) String id;
  private final @ModelField(targetType="ID", isRequired = true) String postID;
  private final @ModelField(targetType="String", isRequired = true) String content;
  ...
}

Expected behavior:

public final class Post implements Model {
  private final @ModelField(targetType="ID", isRequired = true) String id;
  private final @ModelField(targetType="String", isRequired = true) String title;
  private final @ModelField(targetType="Comment") @HasMany(associatedWith = "post", type = Comment.class) List<Comment> comments = null;
  ...
}

public final class Comment implements Model {
  private final @ModelField(targetType="ID", isRequired = true) String id;
  private final @ModelField(targetType="Post", isRequired = true) @BelongsTo(targetName = "postID", type = Post.class) Post post;
  private final @ModelField(targetType="String", isRequired = true) String content;
  ...
}

Note: This is a requirement for both one-to-one and one-to-many relationships

[AaronZyLee]

@raphkim I am not sure whether this is a bug. According to the amplify doc, we only support explicit use case for belongsTo in a bi-direction connection, which is applied in all platform model-gen. Is there a strong demand from customer or are there any prevailing advantages of not using the explicit bi-direction schema as the following?

type Post @model {
  id: ID!
  title: String!
  comments: [Comment] @connection(keyName: "byPost", fields: ["id"])
}

type Comment @model
  @key(name: "byPost", fields: ["postID", "content"]) {
  id: ID!
  postID: ID!
  content: String!
  post: Post @connection(fields: ["postID"])
}

[raphkim]

According to the amplify doc, we only support explicit use case for belongsTo in a bi-direction connection

Every connection is bi-directional in nature; if a parent has a child, then a child has to have a parent. It simply doesn't make sense for the child to not belong to any parent. I do agree that our current doc suggests that bidirectionality is optional, which is simply not true. This must be addressed to clarify that while bidirectionality is a requirement, an explicit bidirectionality is optional (i.e. if "belongsTo" is not explicitly specified, then it is still assumed by the codegen/datastore).

This is a requirement that doesn't come from customer demand; in fact, the customer will not even need to know that there is a @BelongsTo relationship being formed implicitly. This is a requirement for the native platforms which use SQLite foreign key relationships to connect models, and without this annotation being generated by the modelgen, android and iOS will not know which tables need foreign key.

To clarify, the customers may also explicitly specify a "belongsTo" relationship as you say by adding @connective directive to the child model schema, which generates child model correctly. However, the documentation currently specifies this step as optional, which means the codegen is required to catch this and implicitly create a belongsTo relationship if the customer didn't specify it.

[raphkim]

Here's another interesting example. Here, an implicit field is formed on the child model (correct behavior) to be used as a foreign key. However, it is generated without a @BelongsTo annotation, so datastore doesn't recognize this new field as a foreign key.

Schema:

type Post @model {
  id: ID!
  comments: [Comment] @connection
}

type Comment @model {
  id: ID!
}

Current:

public final class Post implements Model {
  private final @ModelField(targetType="ID", isRequired = true) String id;
  private final @ModelField(targetType="Comment") @HasMany(associatedWith = "postCommentsId", type = Comment.class) List<Comment> comments = null;
  ...
}

public final class Comment implements Model {
  private final @ModelField(targetType="ID", isRequired = true) String id;
  private final @ModelField(targetType="ID") String postCommentsId;
  ...
}

Expected:

public final class Post implements Model {
  private final @ModelField(targetType="ID", isRequired = true) String id;
  private final @ModelField(targetType="Comment") @HasMany(associatedWith = "post", type = Comment.class) List<Comment> comments = null;
  ...
}

public final class Comment implements Model {
  private final @ModelField(targetType="ID", isRequired = true) String id;
  private final @ModelField(targetType="Post") @BelongsTo(targetName = "postCommentsId", type = Post.class) Post post;
  ...
}

[AaronZyLee]

@raphkim Not sure if the expected code snippet is correct. I try to make some code changes but find this part: https://github.com/aws-amplify/amplify-codegen/blob/588b9e4da7e40639bdb0d5cf346abd6ce3a0df5d/packages/appsync-modelgen-plugin/src/visitors/appsync-visitor.ts#L458-L459 So the targetName field is originally designed to be removed from the model. But from your output, you add BelongsTo to the targetName type itself, which is meant to be hidden from developer and should use model type instead.

Besides, this change will have impact on all the SDKs. I would like to get more contexts/data-points from other SDKs before making this changes.

Nevertheless, I would suggest that the temporary solution is to inform the user to use the bi-direction schema instead.

[raphkim]

So the targetName field is originally designed to be removed from the model. But from your output, you add BelongsTo to the targetName type itself, which is meant to be hidden from developer and should use model type instead.

You are right! Fixing the "expected behavior" sections accordingly.

Besides, this change will have impact on all the SDKs. I would like to get more contexts/data-points from other SDKs before making this changes.

You are absolutely right. I will raise this with the other platforms for review.

[phani-srikar]

Hi @raphkim. Based on the above comments, closing this issue until we have a thumbs up from Datastore teams that we need to add BelongsTo even for HasOne and HasMany connections.

[raphkim]

Still an issue with v2. If @belongsTo is not explicitly specified in the child model's schema, then sqlite fails to form foreign key relationship

[alharris-at]

Reopening per discussion w/ Android today. We should try to load this into the fix for #319

[alharris-at]

This shouldn't be tracked as an enhancement, but should instead be tracked as a bug, since this causes Cascading deletes in native platforms to fail.

[alharris-at]

I synced up w/ @phani-srikar on this tonight. He's concerned that generating an implicit belongsTo for customers will impact API users, as well as change the response shapes from queries. We need to determine if that's the right behavior, so we should vet options w/ all platform teams, and CLI as well before making a decision on how to proceed.