feat(graphql-connection-transformer): allow null key-based connections

[RossWilliams]

feat(graphql-connection-transformer): allow null key-based connections
Issue #, if available: #5152 #5129 #3275

Description of changes:
Allow connections to @key defined indexes to include nullable connection attributes.
Legacy @connection directive allowed use of nullable fields. When @connection with @key was added, code was put in place to prevent using a nullable scalar as a connection key. I traced the origin of this code and could find no justification for it and removed the check.

Further, when a request is made using attributes which are null, DynamoDB can return errors due to wrong types being provided in key condition values. There is no reason to make a request to DynamoDB when the pk is empty, or a specified field is null. Code is added to early return a null value or an empty list of items.

This PR also fixes an issue found when making a query with an Int type sort field whose value is null. Previously the NONE_INT_VALUE was wrapped in quotes, making it a string. This caused a DynamoDB mismatched type error.

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

[codecov]

Codecov Report

Merging #5153 (df8108f) into master (e28035a) will not change coverage.
The diff coverage is 66.66%.

@@           Coverage Diff           @@
##           master    aws-amplify/amplify-cli#5153   +/-   ##
=======================================
  Coverage   57.30%   57.30%           
=======================================
  Files         474      474           
  Lines       21669    21669           
  Branches     4306     4306           
=======================================
  Hits        12418    12418           
  Misses       8370     8370           
  Partials      881      881           

Impacted Files	Coverage Δ
...tion-transformer/src/ModelConnectionTransformer.ts	89.49% <50.00%> (-0.04%)	⬇️
...es/graphql-connection-transformer/src/resources.ts	78.48% <100.00%> (+0.27%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e28035a...df8108f. Read the comment docs.

[azatoth]

Is there anything blocking review of this pull request?

[jimlevett]

It'd be lovely to see this working.

[yuth]

@RossWilliams Thank you for the PR. This change does not support the case for composite sort key. If you look at the schema below

type Model1 @key(name: "byNameIdAndSort", fields: ["name", "id", "sort"]) {
  id: ID!
  sort: Int!
  name: String!
}

type Model3 @model {
  id: ID!
  model1Id: ID
  model1Sort: Int
  connectionSK: String
  model1Name: String
  connectionsWithCompositeKey: [Model1]
    @connection(
      keyName: "byNameIdAndSort"
      fields: ["model1Name", "model1Id", "model1Sort"]
    )
}

The following mutation would not throw any error

mutation createModel3WithBrokenConnection {
  createModel3(input: { model1Sort: 10, id: "2", model1Id: "1" }) {
    connectionsWithCompositeKey {
      items {
        id
        name
        sort
      }
    }
  }
}

This cause the requestTemplate to throw error. To fix this we could add an default value for fields that are NULL in the transformer and assign it.

[RossWilliams]

@RossWilliams Thank you for the PR. This change does not support the case for composite sort key. If you look at the schema below [snip]

This cause the requestTemplate to throw error. To fix this we could add an default value for fields that are NULL in the transformer and assign it.

@yuth I've failed to replicate or understand the problem you mentioned. Your example includes a missing pk for a connection. The resolver template does not throw an error and correctly returns early with an empty result, as there are no valid connected items.

I've added a commit with tests to match your above described schema. In the test the mutation and subsequent request templates do not throw errors. I also show when a sort key is missing from a composite sort key, and that an item is returned in a mutation when all keys are provided.

Can you try explaining the issue you found in more detail? Which request template do you see throwing an error? Maybe there is a misunderstanding of the expected behaviour? Did you mean to set a null composite sort key value, although this is also handled by the PR?

[xitanggg]

Any updates on this? I am forced to make some of my fields non-nullable.

[RossWilliams]

@xitanggg Sorry I’ve been busy. This weekend I’ll rebase this branch to main and see if that gets it a response

[xitanggg]

Thanks a lot Ross. Would love it see it work, but no worries. It is not an urgent issue, so take your time.

[icecog]

Any progress on this?

[tomhirschfeld]

@RossWilliams @yuth any progress here? A lot of us are waiting on this functionality.

[mansdahlstrom1]

Also waiting for this functionality, has the test case @yuth mentioned been solved? Can i help maybe? (first time contributor)

[SwaySway]

LGTM

[xitanggg]

Thanks for the approval. Can you share when will this likely be merged and released?

We are currently developing a feature where we are forced to fill a random value to make the connection key field non-nullable. This workaround works but is not perfect because the random value would unnecessarily trigger a connection query whereas the null key wouldn't normally. Would love to know if we might be able to ditch the workaround before rolling this feature to production. Thanks.

Change addressed

[arkllc]

@RossWilliams I am excited about this as this fixes aws-amplify/amplify-category-api#262 Could you please let me know how I can pull this in my local amplify environment using npm? Also, when will this be released? Thanks in advance

[RossWilliams]

@arkllc I’m not involved in releases.
You can download the main branch, run ‘yarn dev-build && yarn dev-link’ and your global amplify command will be linked to the main branch.

[jncheung]

hi guys, I have upgrade to 4.43.0 but i am not sure whether this fix is included or not. I can not find anything related to this fix in the latest release note.

BUT I can successfully generate new graphql file using amplify codegen and amplify api push with null GSI field.

is this release reliable or got published accidentally?

[xitanggg]

hi guys, I have upgrade to 4.43.0 but i am not sure whether this fix is included or not. I can not find anything related to this fix in the latest release note.

BUT I can successfully generate new graphql file using amplify codegen and amplify api push with null GSI field.

is this release reliable or got published accidentally?

It is part of 4.42.0 release. v4.41.1...v4.42.0

[lostcodingsomewhere]

hi all -- i'm on 4.43.0 and still facing this issue. any ideas what could be going on if this has been merged in?

[lostcodingsomewhere]

For anyone running into this issue (on v4.43.0), here's a workaround (pretty hackey but will suffice for now):

replace all of your API.graphql with the following helper:

const graphqlAPI = async (
   options,
   additionalHeaders?: {
      [key: string]: string;
   }
): Promise<any> => {
   try {
      return await API.graphql(options, additionalHeaders);
   } catch (err) {
      const realErrors = err.errors.filter(
         (e) => !e.message.startsWith('Cannot return null for non-nullable')
      );
      if (realErrors.length > 0) {
         console.log(`THROWING GRAPHQL ERROR`);
         throw err;
      } else {
         const realDataResult = { data: err.data };
         return realDataResult;
      }
   }
};

[xitanggg]

hi all -- i'm on 4.43.0 and still facing this issue. any ideas what could be going on if this has been merged in?

I don't have any issue with null key-based connection anymore, it works great in my end. Thanks again for the merge.

The purpose of this merge is to let you use @connection with a nullable field (i.e. field without !). Say I have a User type and a School type, some users go to schools would have a schoolID field while others don't go to school wouldn't. Previously, the following schema wouldn't be possible, because schoolID is used in @connection and therefore can't be nullable. And it would have to be non-nullable with ! and developers therefore are forced to assign some schoolID to some users even though they don't go to school (buggy). Now the merge resolves it.

User @model {
   id: String!
   schoolID: String  #Previously, it must be non-nullable using `String!`
   connectSchool: School @connection(fields: ["schoolID"])
}

From your code, it seems that you are trying to ignore the graphQL Cannot return null for non-nullable error.
In graphQL, if you make a field non-nullable by using !, it means you intentionally require data for a certain field, the missing of which would throw an error (it is a feature built in graphQL, because it can be an use case of guaranteeing the retrieval of important data, because partial data can be lost in transmission, and you might want to treat partial loss as error).

If after all, some data fields are not required, you can simply make it nullable by removing the ! in your schema.

[arkllc]

@RossWilliams Is this fix supposed to fix aws-amplify/amplify-category-api#262 too? If yes, I am on 4.50.0 but somehow I still am not able to create data with null secondary keys. appreciate it in advance, if you could please help clarify