Data loss with new relationship approach

[cabcookie]

Environment information

System:
  OS: macOS 14.3.1
  CPU: (10) arm64 Apple M1 Pro
  Memory: 159.95 MB / 32.00 GB
  Shell: /bin/zsh
Binaries:
  Node: 18.19.0 - ~/.nvm/versions/node/v18.19.0/bin/node
  Yarn: undefined - undefined
  npm: 10.2.3 - ~/.nvm/versions/node/v18.19.0/bin/npm
  pnpm: undefined - undefined
NPM Packages:
  @aws-amplify/backend: 0.13.0-beta.20
  @aws-amplify/backend-cli: 0.12.0-beta.22
  aws-amplify: 6.0.28
  aws-cdk: 2.138.0
  aws-cdk-lib: 2.138.0
  typescript: 5.4.5
AWS environment variables:
  AWS_STS_REGIONAL_ENDPOINTS = regional
  AWS_NODEJS_CONNECTION_REUSE_ENABLED = 1
  AWS_SDK_LOAD_CONFIG = 1
No CDK environment variables

Description

The new relationship approach caused data loss for me.

Before the upgrade to @aws-amplify/[email protected] this has been my data model:

DayPlan: a
    .model({
      owner: a.string().authorization([a.allow.owner().to(["read", "delete"])]),
      day: a.date().required(),
      dayGoal: a.string().required(),
      done: a.boolean(),
      todos: a.hasMany("DayPlanTodo"),
    })
    .authorization([a.allow.owner()]),
  DayPlanTodo: a
    .model({
      owner: a.string().authorization([a.allow.owner().to(["read", "delete"])]),
      todo: a.string().required(),
      done: a.boolean(),
      doneOn: a.date(),
      dayPlan: a.belongsTo("DayPlan"),
    })
    .authorization([a.allow.owner()]),

As I have to provide references now, I changed the model to this:

DayPlan: a
    .model({
      owner: a.string().authorization([a.allow.owner().to(["read", "delete"])]),
      day: a.date().required(),
      dayGoal: a.string().required(),
      done: a.boolean(),
      todos: a.hasMany("DayPlanTodo", "dayPlanTodosId"),
    })
    .authorization([a.allow.owner()]),
  DayPlanTodo: a
    .model({
      owner: a.string().authorization([a.allow.owner().to(["read", "delete"])]),
      todo: a.string().required(),
      done: a.boolean(),
      doneOn: a.date(),
      dayPlanTodosId: a.id().required(),
      dayPlan: a.belongsTo("DayPlan", "dayPlanTodosId"),
    })
    .secondaryIndexes((index) => [index("dayPlanTodosId")])
    .authorization([a.allow.owner()]),

Unfortunately, all records in DayPlanTodo have been deleted as a consequence. Lukily, I only did it in my sandbox environment.

[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.