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(api): do not include null values in ModelMutations.create #2504

Merged
merged 3 commits into from Jan 12, 2023
Merged

fix(api): do not include null values in ModelMutations.create #2504

merged 3 commits into from Jan 12, 2023

Conversation

ragingsquirrel3
Copy link
Contributor

@ragingsquirrel3 ragingsquirrel3 commented Dec 21, 2022

Addresses #2492

GraphQL model helpers have mutation helpers that generate JSON input for mutation variables based on models' toJson() methods. In the case of the model having a null value for an owner field, that null value gets passed to AppSync which will reject the mutation. This PR removes null fields from input variables when calling ModelMutations.create(). That avoids the error for the case of owner fields and generally simplifies client logic, allowing the server to set null values if omitted.

Note the original issue states "Model helpers should not pass null fields" but I believe that in general, there are valid cases of setting null values in mutations, like unsetting a nullable field that had previously been set to something non-null. For that reason, this PR only scopes the fix to create operations, which does not have any need to explicitly send null values.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ragingsquirrel3 ragingsquirrel3 marked this pull request as ready for review December 21, 2022 17:11
@ragingsquirrel3 ragingsquirrel3 requested a review from a team as a code owner December 21, 2022 17:11
@ragingsquirrel3 ragingsquirrel3 changed the title fix(api): do not include null values for owner fields in ModelMutations fix(api): do not include null values in ModelMutations.create Dec 27, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 27, 2022

Codecov Report

Merging #2504 (88ed7da) into next (7250c1c) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             next    #2504      +/-   ##
==========================================
+ Coverage   56.19%   56.21%   +0.02%     
==========================================
  Files         115      115              
  Lines        6987     6991       +4     
==========================================
+ Hits         3926     3930       +4     
  Misses       3061     3061              
Flag Coverage Δ
android-unit-tests ∅ <ø> (∅)
flutter-unit-tests 47.41% <100.00%> (+0.03%) ⬆️
ios-unit-tests 95.32% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...src/graphql/factories/graphql_request_factory.dart 95.45% <100.00%> (+0.14%) ⬆️
...src/graphql/factories/model_mutations_factory.dart 100.00% <100.00%> (ø)

Copy link
Contributor

@dnys1 dnys1 left a comment

Choose a reason for hiding this comment

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

What about update? I'm guessing that would suffer the same problem.

You're correct that we should be setting null in cases where the user passed null but it's possible to distinguish these cases precisely. In the variables map, the presence of that field's key indicates the user passed a value for it. You need to check that instead of the value.

final variables = {
  'setNull': null,
};

// Doing this gives no insight about the user's intentions.
print(variables['setNull'] == null); // true
print(variables['doNotSetNull'] == null); // true

// Here we see what their intentions were.
print(variables.containsKey('setNull')); // true
print(variables.containsKey('doNotSetNull')); // false

@ragingsquirrel3 ragingsquirrel3 merged commit d3795d7 into aws-amplify:next Jan 12, 2023
@ragingsquirrel3 ragingsquirrel3 deleted the chore/repro-2492 branch January 12, 2023 20:48
dnys1 pushed a commit to Jordan-Nelson/amplify-flutter that referenced this pull request Jan 30, 2023
- fix(auth)!: Fetch Auth Session offline behavior ([aws-amplify#2585](aws-amplify#2585))

- fix(api): do not include null values in ModelMutations.create ([aws-amplify#2504](aws-amplify#2504))
- fix(api): model helpers use targetNames in schemas with CPK enabled ([aws-amplify#2559](aws-amplify#2559))
- fix(auth): Clear credentials before redirect on Web ([aws-amplify#2603](aws-amplify#2603))
- fix(auth): Refresh token in non-state machine calls ([aws-amplify#2572](aws-amplify#2572))
- fix(authenticator): ARB syntax ([aws-amplify#2560](aws-amplify#2560))
- fix(aws_common): AWSFile contentType getter should not throw exception
- fix(datastore): prevent unhandled exception crashing App rebuilding sync expression
- fix(storage): incorrect transferred bytes emitted from upload task

- feat(analytics): Legacy data migration of Pinpoint Endpoint ID ([aws-amplify#2489](aws-amplify#2489))
- feat(smithy_aws): add copyWith to S3ClientConfig
- feat(storage): allow configuring transfer acceleration

Updated-Components: Amplify Flutter, Amplify Dart, Amplify UI, DB Common, Secure Storage, AWS Common, Smithy, Worker Bee
dnys1 pushed a commit to Jordan-Nelson/amplify-flutter that referenced this pull request Jan 30, 2023
- fix(auth)!: Fetch Auth Session offline behavior ([aws-amplify#2585](aws-amplify#2585))

- fix(api): do not include null values in ModelMutations.create ([aws-amplify#2504](aws-amplify#2504))
- fix(api): model helpers use targetNames in schemas with CPK enabled ([aws-amplify#2559](aws-amplify#2559))
- fix(auth): Clear credentials before redirect on Web ([aws-amplify#2603](aws-amplify#2603))
- fix(auth): Refresh token in non-state machine calls ([aws-amplify#2572](aws-amplify#2572))
- fix(authenticator): ARB syntax ([aws-amplify#2560](aws-amplify#2560))
- fix(aws_common): AWSFile contentType getter should not throw exception
- fix(datastore): prevent unhandled exception crashing App rebuilding sync expression
- fix(storage): incorrect transferred bytes emitted from upload task

- feat(analytics): Legacy data migration of Pinpoint Endpoint ID ([aws-amplify#2489](aws-amplify#2489))
- feat(smithy_aws): add copyWith to S3ClientConfig
- feat(storage): allow configuring transfer acceleration

Updated-Components: Amplify Flutter, Amplify Dart, Amplify UI, DB Common, Secure Storage, AWS Common, Smithy, Worker Bee
Jordan-Nelson added a commit that referenced this pull request Jan 30, 2023
* chore(repo): Update component definition

Adds `amplify_api_dart` to the mix and creates components for DB Common, Secure Storage, and AWS Common

* fix(aft): Update changelog logic

For the version commit message changelog, only include publishable packages. Also updates base ref logic to ensure packages can be moved in and out of components.

* chore(version): Bump version

- fix(auth)!: Fetch Auth Session offline behavior ([#2585](#2585))

- fix(api): do not include null values in ModelMutations.create ([#2504](#2504))
- fix(api): model helpers use targetNames in schemas with CPK enabled ([#2559](#2559))
- fix(auth): Clear credentials before redirect on Web ([#2603](#2603))
- fix(auth): Refresh token in non-state machine calls ([#2572](#2572))
- fix(authenticator): ARB syntax ([#2560](#2560))
- fix(aws_common): AWSFile contentType getter should not throw exception
- fix(datastore): prevent unhandled exception crashing App rebuilding sync expression
- fix(storage): incorrect transferred bytes emitted from upload task

- feat(analytics): Legacy data migration of Pinpoint Endpoint ID ([#2489](#2489))
- feat(smithy_aws): add copyWith to S3ClientConfig
- feat(storage): allow configuring transfer acceleration

Updated-Components: Amplify Flutter, Amplify Dart, Amplify UI, DB Common, Secure Storage, AWS Common, Smithy, Worker Bee

---------

Co-authored-by: Dillon Nys <nydillon@amazon.com>
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.

None yet

5 participants