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

[Bug? DX issue?] DataStore opaquely coerces my NonNull field to Null and silently fails without error #7127

Closed
swyxio opened this issue Nov 6, 2020 · 8 comments
Labels
DataStore Related to DataStore category to-be-reproduced Used in order for Amplify to reproduce said issue

Comments

@swyxio
Copy link
Contributor

swyxio commented Nov 6, 2020

Describe the bug

My schema.graphql:

type Board @model @auth(rules: [{ allow: owner }]) {
  id: ID!
  name: String!
  lists: [List] @connection(keyName: "byBoard", fields: ["id"])
}

type List
  @model
  @key(name: "byBoard", fields: ["boardID"], queryField: "listsByBoard")
  @auth(rules: [{ allow: owner }]) {
  id: ID!
  title: String!
  boardID: ID!
  board: Board @connection(fields: ["boardID"])
  cards: [Card] @connection(keyName: "byList", fields: ["id"])
}

type Card
  @model
  @key(name: "byList", fields: ["listID", "content"], queryField: "cardsByList")
  @auth(rules: [{ allow: owner }]) {
  id: ID!
  listID: ID!
  list: List @connection(fields: ["listID"])
  content: String!
}

Here is what i am trying to do:

    const newCard = {
      id: '23',
      content: cardFormState.content,
      listID: listId, // i have checked that this is not null
    }
	await DataStore.save(new Card(newCard))

Yet when I run this code it does not error, does not save this to AppSync/DynamoDB, and logs this warning in the browser:

"Variable 'input' has coerced Null value for NonNull type 'ID!'"

the full response is {"data":null,"errors":[{"path":null,"locations":[{"line":1,"column":20,"sourceName":null}],"message":"Variable 'input' has coerced Null value for NonNull type 'ID!'"}]}

I assume this is just graphql helpfully suppressing the error for us. I would expect DataStore to surface this error.

However even this error message is not helpful, and I have no idea why DataStore is coercing what i have clearly checked is a string, to null. I am out of ideas on how to fix this.

To Reproduce

you can clone https://github.com/sw-yx/trello-amplified and add auth and add api (my attempt here)

or

you can watch my 2 minute video bug report here: https://youtu.be/25PWIBfhfZE

Expected behavior

  1. it should error when there is an error
  2. error message should give more information than this
  3. why is it even coercing null value for my non null input

context

@swyxio swyxio added the to-be-reproduced Used in order for Amplify to reproduce said issue label Nov 6, 2020
@wlee221 wlee221 added the DataStore Related to DataStore category label Nov 6, 2020
wei added a commit to wei/amplify-js-7127 that referenced this issue Nov 8, 2020
@wei
Copy link
Contributor

wei commented Nov 8, 2020

I can reproduce.

I believe this is the exact same issue as mentioned in #5161 However, that issue does not seem to have a clear solution posted.


With your code, I can reproduce the same "Variable 'input' has coerced Null value for NonNull type 'ID!'" error when adding a list and it can be traced to:

case 'BELONGS_TO':
if (instance[rItem.fieldName]) {
let modelInstance: T;
try {
modelInstance = modelInstanceCreator(
modelConstructor,
instance[rItem.fieldName]
);
} catch (error) {
// Do nothing
}
const isDeleted = (<ModelInstanceMetadata>(
draftInstance[rItem.fieldName]
))._deleted;
if (!isDeleted) {
result.push({
modelName: rItem.modelName,
item: instance[rItem.fieldName],
instance: modelInstance,
});
}
}
(<any>draftInstance)[rItem.targetName] = draftInstance[
rItem.fieldName
]
? (<PersistentModel>draftInstance[rItem.fieldName]).id
: null;
delete draftInstance[rItem.fieldName];
break;

Relevant files:

Since we only set list.boardID field, rItem.fieldName (board) which is undefined. The logic on L260-264 is setting rItem.targetName (boardID) to null when list.board is falsey. This results in datastore sending a null list.boardID in the graphql request even though it was provided correctly.

(I don't understand what L236-258 is doing though)

I think you are expecting datastore to populate list.board (or card.list) automatically by just specifying list.boardID (or card.listID) Not sure if we need to manually set list.board = board manually as it doesn't look like something datastore is doing automatically❓


Anyways, after reviewing graphql docs and datastore docs. I'm not sure if your expected behavior above is supported, it seems like the relational modeling has limited support in datastore compared to GraphQL. #5054 (comment) also shows certain operations are just not yet possible with datastore at this time.

I've made some modifications strictly following the datastore docs example and it's now functional. The repo is at wei/amplify-js-7127 and the commit can be seen at wei/amplify-js-7127@f03feac

Summary of my changes in the commit:

  • Removed the the list.board @connection
  • Fetch cards using await DataStore.query(CardModel)
  • Pass in cards to List component differently, like this

Lastly, going back to the original question, this is the piece of code that runs on our error:

function defaultErrorHandler(error: SyncError) {
logger.warn(error);
}

and it looks like:
image

I agree it's not immediately apparent that there has been an error, perhaps as an easy first step, we can change it to include the word Sync error like it's already been done here:

error: err => {
logger.warn('Sync error', err);
this.initReject();
},

or a different phrase to indicate an error. Any feedback❓

/cc @iartemiev

@swyxio
Copy link
Contributor Author

swyxio commented Nov 8, 2020

i'm super grateful that you spent the time to repro!!!

  1. yes, i would expect DataStore to populate that for me - notice that it does so for list.board but not list.cards... thats a separate issue i ran into that was a dealbreaker for me :(
  2. i HATE silent errors with a passion. logging a warn isn't good enough for me imo, i would actually throw an error. why do we automatically downgrade errors to warnings? this is the sort of monkey business people criticize graphql for, and with datastore we are trying to provide a cleaner abstraction over graphql.

@wei
Copy link
Contributor

wei commented Nov 8, 2020

Sure thing!

RE (1): Could you start fresh with a new clone, checkout swyxDataStoreVersion branch, use a new amplify env and run amplify codegen models before amplify push, Clear the local indexdb too or use incognito.
it's not populating list.board for me. It actually throws an error on this line because list.board is undefined:
https://github.com/sw-yx/trello-amplified/blob/64f5dcd8401b6d7cd90608cbefc3d0e5b34db7f0/src/pages/Board.js#L15

RE (2): I did notice some data sync errors show up as warnings, will need to defer to datastore maintainer for the reasoning behind that.

@wei
Copy link
Contributor

wei commented Nov 8, 2020

thats a separate issue i ran into that was a dealbreaker for me :(

RE (1): Check out #6973 ; )

DataStore doesn't currently support syncing any connections to a model

I guess it's just not possible right now. For the time being you can work on top of my repo which is using what's currently possible with datastore to create trello-amplified.

@swyxio
Copy link
Contributor Author

swyxio commented Nov 9, 2020

appreciate the superfast feedback!! so i guess the remaining question is:

  1. is there a bug or is it working "as intended" for now?
  2. there is definitely a DX issue we could discuss around whether errors should be errors, or errors should be warnings, when using AppSync.

@wei
Copy link
Contributor

wei commented Nov 12, 2020

  1. I confirmed with @iartemiev that (1) is working is working "as intended" for now. Datastore does not auto populate connections you'll have to query by connected id for now as seen in my repo.

  2. One reason is that we don't want to throw errors when one model fails to sync; so others continue syncing to minimize data loss. Definitely still worth more discussions.


Now, why did the model pass local validations `validateModelFields` before sending a appsync graphql request?

boardID: ID! = null passed because it is missing a field definition in generated models/schema.js:

Snippet from schema.graphql:

type List
  @model
  @key(name: "byBoard", fields: ["boardID"], queryField: "listsByBoard")
  @auth(rules: [{ allow: owner }]) {
  id: ID!
  title: String!
  boardID: ID!
  board: Board @connection(fields: ["boardID"])
  cards: [Card] @connection(keyName: "byList", fields: ["id"])
}
Snippet from `schema.js` missing boardID field definition:
"List": {
    "name": "List",
    "fields": {
        "id": {
            "name": "id",
            "isArray": false,
            "type": "ID",
            "isRequired": true,
            "attributes": []
        },
        "title": {
            "name": "title",
            "isArray": false,
            "type": "String",
            "isRequired": true,
            "attributes": []
        },
        "board": {
            "name": "board",
            "isArray": false,
            "type": {
                "model": "Board"
            },
            "isRequired": false,
            "attributes": [],
            "association": {
                "connectionType": "BELONGS_TO",
                "targetName": "boardID"
            }
        },
        "cards": {
            "name": "cards",
            "isArray": true,
            "type": {
                "model": "Card"
            },
            "isRequired": false,
            "attributes": [],
            "isArrayNullable": true,
            "association": {
                "connectionType": "HAS_MANY",
                "associatedWith": "list"
            }
        },
        "owner": {
            "name": "owner",
            "isArray": false,
            "type": "String",
            "isRequired": false,
            "attributes": []
        }
    },
    "syncable": true,
    "pluralName": "Lists",
    "attributes": [
        {
            "type": "model",
            "properties": {}
        },
        {
            "type": "key",
            "properties": {
                "name": "byBoard",
                "fields": [
                    "boardID"
                ],
                "queryField": "listsByBoard"
            }
        },
        {
            "type": "auth",
            "properties": {
                "rules": [
                    {
                        "provider": "userPools",
                        "ownerField": "owner",
                        "allow": "owner",
                        "identityClaim": "cognito:username",
                        "operations": [
                            "create",
                            "update",
                            "delete",
                            "read"
                        ]
                    }
                ]
            }
        }
    ]
},

So the code snippet in my first comment is setting boardID to null and then the local validation is passing it without checks as the boardID field definition is undefined.

@mauerbac mauerbac closed this as completed Dec 2, 2020
@swyxio
Copy link
Contributor Author

swyxio commented Dec 3, 2020

@mauerbac we still were at "Definitely still worth more discussions." in the previous comment

@github-actions
Copy link

github-actions bot commented Dec 4, 2021

This issue 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 Dec 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
DataStore Related to DataStore category to-be-reproduced Used in order for Amplify to reproduce said issue
Projects
None yet
Development

No branches or pull requests

4 participants