Skip to content

Conversation

@ondojo
Copy link

@ondojo ondojo commented Aug 5, 2020

Fix array notation bug related to #5040

Issue #, if available: #5040

Description of changes: Validation of model ignored the isRequired attribute in case the attribute value was of type array. Added this consideration to the if clause to allow for graphlQL annotations such as [foobar] or [foobar!]. Now the attributes can be set to null or empty arrays and the sync still works.

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

Fix array notation bug related to #5040
@ondojo ondojo requested a review from manueliglesias as a code owner August 5, 2020 14:50
@ondojo
Copy link
Author

ondojo commented Aug 6, 2020

Hello,

the circle ci error message says

Creating workspace archive...

Uploading workspace archive...

Error uploading workspace archive: AccessDenied: Access Denied
status code: 403, request id: 444FF68393D2C9DC, host id: 5giQT2G09PD0Ig+4Ei+SZLZXJo1sYyleelJIH9wZB15H9ZAAcjO5WKcN0Q6Yeb456o/6lO2INlg=

Is there any Chance we can fix this?

@amhinson amhinson requested a review from iartemiev August 20, 2020 15:40
@mauerbac
Copy link
Contributor

hi @ondojo - sorry for the delay. I just looked at the CircleCi tests on my end and it looks like one of the datastore unit tests failed. Are you able to run these locally and update the test? Thanks for the contribution

}

if ((<[]>v).some(e => typeof e !== jsType)) {
if (v !== null && (<[]>v).some(e => typeof e !== jsType)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should check for v being undefined as well. I have two reasons for this:

  1. The require check at line 236 checks for undefined as well
  2. Typescript allows copyOf records to be undefined but not null

Another note, 2 opens up a new issue. Setting the value to undefined will result in it being ignored as an input in the graphql request resulting in the field not being updated. If set to null it works, but Typescript checks says null is not valid in the copyOf mutation. I am unsure if this is due to this PR or it is an existing issue with DataStore that I discovered upon testing this PR.

@sammartinez
Copy link
Contributor

Resolving as the above PR looks to have been taken care of in a separate pull request which has be related to this pull request.

@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants