fix(datastore): remove connections with model field as undefined

[dpilch]

Description of changes

Allows removal of connection using the model field instead of the join fields.

await DataStore.save(Parent.copyOf(parentWithChild, draft => {
  draft.child = undefined;
}));

Example:

Given the following schema:

type Parent @model {
  id: ID!
  child: Child @hasOne
}

type Child @model {
  id: ID!
  parent: Parent @belongsTo
}

In order to remove a child from the parent the join field (parentChildId) must be set to undefined rather than the direct model field (child).

const parent = new Parent({});
const child = new Child({});
const savedParent = await DataStore.save(parent);
const savedChild = await DataStore.save(child);

const parentWithChild = await DataStore.save(Parent.copyOf(savedParent, draft => {
  draft.child = savedChild;
}));

await DataStore.save(Parent.copyOf(parentWithChild, draft => {
  draft.child = undefined; // does not remove the connection
}));

await DataStore.save(Parent.copyOf(parentWithChild, draft => {
  draft.parentChildId = undefined; // removes the connection
}));

With the change draft.child = undefined will now also remove the connection.

Issue #, if available

Resolves #10750

Description of how you validated changes

Tested with minimum repro above.

Checklist

• PR description included
• yarn test passes
• Tests are changed or added
• Relevant documentation is changed or added (and PR referenced)

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

[codecov-commenter]

Codecov Report

Merging #10983 (caa9639) into main (8f6dd70) will decrease coverage by 3.76%.
The diff coverage is n/a.

📣 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   #10983      +/-   ##
==========================================
- Coverage   85.65%   81.90%   -3.76%     
==========================================
  Files         196      194       -2     
  Lines       18261    19376    +1115     
  Branches     3892     4179     +287     
==========================================
+ Hits        15642    15870     +228     
- Misses       2543     3217     +674     
- Partials       76      289     +213     

Impacted Files	Coverage Δ
...ackages/pubsub/src/Providers/AWSAppSyncProvider.ts	15.38% <0.00%> (-52.75%)	⬇️
packages/datastore/src/predicates/index.ts	78.86% <0.00%> (-17.08%)	⬇️
packages/amazon-cognito-identity-js/src/Client.js	50.48% <0.00%> (-1.52%)	⬇️
...ackages/pubsub/src/Providers/MqttOverWSProvider.ts	91.53% <0.00%> (-1.06%)	⬇️
packages/pubsub/src/PubSub.ts	92.42% <0.00%> (-0.64%)	⬇️
...ages/amazon-cognito-identity-js/src/CognitoUser.js	79.06% <0.00%> (-0.12%)	⬇️
packages/datastore/__tests__/commonAdapterTests.ts	98.26% <0.00%> (-0.07%)	⬇️
packages/datastore/src/datastore/datastore.ts	88.30% <0.00%> (-0.03%)	⬇️
packages/datastore/src/types.ts	86.47% <0.00%> (ø)
packages/api-graphql/src/GraphQLAPI.ts	88.48% <0.00%> (ø)
... and 24 more

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

[svidgen]

Sorry I couldn't consolidate my feedback into fewer rounds on this. Slow brain days. 🙁

[svidgen]

Sorry I didn't think of all the feedbacks in one round. 😓 But, can we extend all of these tests a little to ensure parent nullification is saved correctly? I.e.,

1. Save the copied child without parent.
2. Retrieve the copied child.
3. Ensure the nullified parent is persisted correctly.

I'm also not clear on whether we need to confirm the round-trip to cloud storage successfully persists the removal there. Are we sure the right mutations are being generated? Have you tried it manually?

[dpilch]

I updated the tests to include assertions on the parent was well. I have tested manually using the original reproduction steps and it is persisting in cloud. Do you think we should add an E2E test for this as well?

[svidgen]

Do you think we should add an E2E test for this as well?

Yes. It would also be nice to clamp the e2e behavior on this against our gql fake in the no-longer-accurately-named connectivityHandling tests.

Both of those are nonblocking IMO. Get the fix out sooner rather than later. But, one and/or both of those should be fast-followups, IMO, to guard against regression.

[svidgen]

Thanks for the iterations!

[david-mcafee]

LGTM! 🎉