fix(@aws-amplify/datastore): strictly define null vs undefined behavior on models

[svidgen]

Resubmitted

Originally submitted as #10915, which caused failing integ tests, due to owner: null being submitted in mutations when the the owner field was made explicit. AppSync rejects owner: null as "Unauthorized."

This updated PR omits the owner field in mutations (or custom owner field per model attributes.auth) when the value would be null or undefined.

Added tests for owner handling are in connectivityHandling.test.ts:102-168, with owner field validation occurring in the graphql fake in helper.ts.

The fix is for owner handling is in mutation.ts on lines 487-490 and lines 526-529.

You can also see the bulk of the owner field fix and tests together by looking at the 2nd commit in the PR.

The 2nd attempt under #10932 revealed model instances failing validation during copyOf of models explicit timestamp fields. Timestamp fields are never actually required by AppSync, so it's safe from a runtime perspective to omit them even when marked as required in the MIPR. Even though this is a bug in runtime validation, it's an existing bug that we need to consider independently, especially as fixing the validation could actually be construed as a breaking change.

To address this, this PR treats all timestamp fields as optional.

Description of changes

DataStore currently treats nullable model fields different before/after synchronization, which can lead to unexpected results when operating against or querying these instances. For example:

Given:

type Model @model {
  id: ID!
  value: String!
  otherValue: String
}

If we save an instance:

const saved = await DataStore.save(new Model({
    value: 'some value'
    /* otherValue: 'oh nevermind!' */
}));

We'll have this returned:

{
  "value": "some value",
  "id": "a9f6e211-1d91-45c6-8fbe-46173fb9f09b"
}

If we query by ID before the instance is synchronized to the cloud, we see the same thing. And, if we query for models where m.otherValue.eq(null), the model is not in the result set, because the field is actually undefined.

However, after synchronization, the the cloud storage provider (e.g., AppSync) will set optional fields to null:

{
  "id": "a9f6e211-1d91-45c6-8fbe-46173fb9f09b",
  "value": "unique instance 0.21662066323138118",

  // LOOK!
  "otherValue": null, 

  "createdAt": "2023-01-27T18:02:40.093Z",
  "updatedAt": "2023-01-27T18:02:40.093Z",
  "owner": "test",
  "_version": 1,
  "_lastChangedAt": 1674842560119,
  "_deleted": null
}

The model will then appear in the result set if we search for m => m.otherValue.eq(null).

This is an unintended behavior that customers should not have to work around. This PR normalizes that behavior so that optional fields default to null when unspecified or specified as undefined. (undefined can't actually be persisted in cloud storage.)

Issue #, if available

Description of how you validated changes

1. Added and updated unit tests.
2. Verified in sample apps.
3. Temporarily added integ testing to this branch where all unit and integ tests pass.

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 #11009 (3497a25) into main (aa379c5) will decrease coverage by 0.01%.
The diff coverage is 83.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   #11009      +/-   ##
==========================================
- Coverage   82.21%   82.21%   -0.01%     
==========================================
  Files         193      193              
  Lines       19296    19389      +93     
  Branches     4165     4192      +27     
==========================================
+ Hits        15865    15940      +75     
- Misses       3143     3161      +18     
  Partials      288      288              

Impacted Files	Coverage Δ
packages/datastore/__tests__/helpers.ts	84.17% <75.36%> (-1.32%)	⬇️
packages/datastore/__tests__/commonAdapterTests.ts	98.26% <100.00%> (ø)
packages/datastore/src/datastore/datastore.ts	88.52% <100.00%> (+0.21%)	⬆️
packages/datastore/src/predicates/next.ts	94.98% <100.00%> (+0.03%)	⬆️
packages/datastore/src/sync/processors/mutation.ts	82.80% <100.00%> (+0.64%)	⬆️
...tastore/src/storage/adapter/AsyncStorageAdapter.ts	93.33% <0.00%> (-0.71%)	⬇️

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

[david-mcafee]

Besides a super minor nit on a comment, LGTM! 🎉 As always, really appreciate the in-depth level of testing, as well as the informative comments.

[david-mcafee]

LGTM! 🎉

[manueliglesias]

LGTM 🎉

[david-mcafee]

LGTM!