Adding predicate to mutate for GraphQL condition input

[lawmicha]

Details

• This exposes the ability to add a condition input for callers using Model based GraphQL APIs from APIPlugin. (Model based GraphQL APIs still need to be reviewed, this change is mostly POC/expose the capability/unblock existing preview features)
• The work on the Model-to-GraphQLDocument translators is pre-req for DataStore Delete API with predicate, DataStore Save API with predicate.

Major changes

• GraphQLDocument helpers have now been refactored to a new class called SyncEnabledGraphQLDocument. It is a decorator class which holds a GraphQLDocument.
• Add condition input to all mutations, exposed to the client as a predicate on the mutation.

Minor changes

• Remove AnyModel based GraphQL APIs as datastore is now constructing the requests for Document based GraphQL APIs with AnyModel as the responseType.

• Updated decoding logic to send error with [GraphQLError] instead of transformation error with raw response when errors can be decoded but data cannot be.

• Removed decodePath from GrpahQLDocument helpers and rely on caller to decide what the decode path is since they are also passing in responseType. Updated most callers to use document.name

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

[wooj2]

I think it make the code more readable if we avoided nested do/catch statements... what do you think?

[palpatim]

+1. I'd also favor breaking out the handling into a separate method after it grows beyond about 3 lines.

[lawmicha]

thanks, will do another pass on refactoring this

[lawmicha]

updated code, pulled out the nested try-catch into sequential logic so no longer a nested try-catch block. Couldn't think of a quick way it out into separate methods and name them accurately and will leave that for another time (still have TODO in that file to do separate methods)

[palpatim]

I'm less happy with the syncEnabled and syncEnabledVersion arguments every time I see them. Sync and conflict resolution should be:

• Separate concerns from the models themselves, as they are metadata about the operation and model, not part of the model
• Generic, such that some other provider could adopt a completely different strategy having nothing to do with versions lastChanged dates

I think we need to revisit this design.

[palpatim]

What is the use case for a create mutation with a predicate? I'm not sure I understand how that even works.

[lawmicha]

good question, i just tried to do this on the AppSync console:

mutation createPost($input:CreatePostInput!, $condition: ModelPostConditionInput) {
  createPost(input: $input, condition: $condition){
    content
    id
    title
  }
}

{
  "input": {
    "title": "title",
    "content": "content",
    "createdAt": "2011-11-11T11:11:11.111Z"
  },
  "condition": {
    "title": {
      "eq": "title"
    }
  }
}

I can't get pass the 400 ConditionalCheckFailedException. What I think we should do here is:

• This GraphQLCreateMutation should not take in a predicate to avoid confusion on what the use case is ("conditionally update on an update/delete mutation").
• The customer facing API - Model based GraphQL API's mutate takes in the predicate and mutation type. We can fail fast by doing client side validation if mutation type is create and predicate is passed in. This would be nice to have to provide a good recovery message rather than hitting the service and returning 400

[palpatim]

We need to go a bit deeper--why is the transformer generating create mutations with conditionals? Either it's a bug in the transformer, or there's a valid use case we're not catching. If the former, we need to get the CLI team to address. If the latter, we'll need to understand and support the use case.

Based on the DDB docs, and that fact that we're already explicitly setting a attribute_not_exists condition in the transformer-generated onCreate* resolvers, my belief is that conditional creates are not supported, and the transformer is incorrectly generating a mutation that allows a conditional input.

[lawmicha]

thanks for looking into the DDB docs, will confirm that this is a bug with AppSync service team. If it is, then we should for sure do client validation or do not expose a way to do a create mutation with a condition (depending on how the APIs are structured). For example, mutation type can be an Enum with create, update(condition), delete(condition). or another way is as i was mentioning earlier, to do fail on the client with validation against a call to the Model-based API mutation with create mutation type and passing in the condition.

[lawmicha]

re-read your comment, realized what you meant by "why is the transformer generating create mutations with conditionals?", it is amplify-cli transformation logic, which isn't AppSync service team's issue. Opening issue over in amplify-CLI repo aws-amplify/amplify-cli#3146

[lawmicha]

I'm less happy with the syncEnabled and syncEnabledVersion arguments every time I see them. Sync and conflict resolution should be:

• Separate concerns from the models themselves, as they are metadata about the operation and model, not part of the model
• Generic, such that some other provider could adopt a completely different strategy having nothing to do with versions lastChanged dates

I think we need to revisit this design.

thanks for the suggestions, i'm working on outlining a design of these classes and will review it with you guys later this week.

[palpatim]

What, if any, guarantees do we provide around consistency of the schema demanding a _version and us supplying it? Is it possible for the Input document generated by the CLI to want a _version but the variable to not supply it? What happens in that case, does the service generate an error?

[lawmicha]

the cases which the developer needs to be aware of is:

1. Create Mutation, Queries, and Subscribe do not need version. Create mutation does allow a version but will fail with internal error response from the service. There shouldn't be a use case where create mutation needs a version, but i think it might just be how the transformer code was written that all mutations including Create result in a PutItem call to DynamoDB. (similar to why a ConditionInput exists on a create mutation)
2. Update Mutation and Delete Mutation contains optional version in the variables. if not passed in, will bump existing version. If passed in, I'm assuming will do conflict resolution on service side

i tried representing this in client side logic but then decided to create a more flexible decorator which simply takes in an optional version, thus leaving it up to the caller to know what they are doing.

• currently the caller needs to know what they are doing to supply or not supply a version on the underlying graphQLDocument instance they are decorating.
• yes, it is possible . currently CLI transforms the schema and wants a _version for create/update/delete mutation. create should never be created with a version. update/delete is optional.
• only in the create mutation scenario, have i seen an error generated by the service

[palpatim]

Sounds like we still have some sharp edges on this. If not already in our backlog, please add an issue to resolve this with CLI team/transformer, and let's do what we can to reduce cases where a customer might be put into a place where they have to understand the inner workings of our conflict resolution strategies.

[palpatim]

Relying on string comparison in document names is a bug waiting to happen. We need a more robust and explicit way of determining that a selection set field returns a List, and specifically one of the AWS-defined Lists that have an items and nextToken field for pagination.

[lawmicha]

ah yes, the problem here is if name contains "list" where it was part of the user generated model's name then this all breaks. taking a look

[lawmicha]

hmm so i basically moved the responsibility of knowing whether it is a paginated selection set to the [SelectionSet] extension like [SelectionSet].paginate() will return append "items" and "nextToken" and [SelectionSet].isPaginated() -> Bool checks if first and last items in the array. may have to chat with you about where you see the custom List used

[palpatim]

If all you do is vary the document type in the switch cases, move the duplicated return GraphQLRequest statements outside of the switch

[lawmicha]

oh i see why i did this, i was getting
Protocol 'GraphQLDocument' can only be used as a generic constraint because it has Self or associated type requirements, not sure how to declare the 'document' type when I try let document: GraphQLDocument

[palpatim]

If we don't already have one let's get an issue filed for this so we can refer to it in the release notes/commit logs when we release this incremental update

[lawmicha]

added #291

[palpatim]

Enhancement for later: We should see if we can centralize name generation of these various documents, fields, input types, etc. There's a lot of string manipulation scattered throughout the graphql support code that is hard to remember and refer to later.

[lawmicha]

Added over in "API Plugin Improvement" Issue to track this #292

[jamesonwilliams]

I would do this in a separate PR, to reduce the size of this API PR, and keep the discussion focused to API changes.

[lawmicha]

closing in favour of #309