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(data): update all operations to return flattened response, null, or empty array response on error #13179

Closed
wants to merge 22 commits into from

Conversation

david-mcafee
Copy link
Contributor

@david-mcafee david-mcafee commented Mar 26, 2024

Description of changes

NOTE:

These changes have been ported over to the amplify-api-next repo, and a new PR has been created for the tests added to this repo:
https://github.com/aws-amplify/amplify-api-next/pull/145/files
#13279

fix(data): update all operations to return flattened response, null, or empty array response on error

Note: unit tests will not pass until the related types PR has been merged.

As of today, there are several response shapes for data returned to the customer when an error is present. data can be:

  1. null
  2. an empty object
  3. "populated" but with a null value: { getPost: null }
  4. an actual record: { getPost: { id: '1', title: 'Hello, World!' } }

This PR handles each of the above scenarios for all model operations by flattening the error response when necessary, and returning either null or an empty array unless actual data is present.

Issue #, if available

Description of how you validated changes

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

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

@david-mcafee david-mcafee changed the title Mcafd/fix models list return types fix(data): incorrect data/types returned in error cases Mar 26, 2024

export function getFactory(
client: ClientWithModels,
modelIntrospection: ModelIntrospectionSchema,
model: SchemaModel,
operation: ModelOperation,
useContext = false
useContext = false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have always used trailing commas, but this PR removed them during a Prettier upgrade.

Will discuss during standup, but it's worth us making a decision on this now so we can either 1) reformat the library, or 2) change our local config to avoid these changes being included in future PRs

/**
* Handle errors for list and index query operations
*/
export function handleListGraphQlError(error: any) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered using a single utility that accepted a flag (something like isList), but decided against it as this approach seems slightly less error prone / more straightforward to parse. This is only a slight preference, so if someone feels more strongly that handleListGraphQlError and handleSingularGraphQlError should be a single utility function, I'm happy to combine them.

@david-mcafee david-mcafee changed the title fix(data): incorrect data/types returned in error cases fix(data): update all operations to return null or empty array response on error Mar 28, 2024
@david-mcafee david-mcafee marked this pull request as ready for review March 29, 2024 00:15
@david-mcafee david-mcafee requested review from a team as code owners March 29, 2024 00:15
@david-mcafee david-mcafee requested a review from svidgen March 29, 2024 00:15
* if `flattenedResult`, result is an actual record:
*/
if (flattenedResult) {
// TODO: custom selection set. current selection set is default selection set only
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same TODO that can be found with the existing implementation in the try block

if (options?.selectionSet) {
return { data: flattenedResult, errors };
} else {
// TODO: refactor to avoid destructuring here
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same TODO that can be found with the existing implementation in the try block

@david-mcafee david-mcafee changed the title fix(data): update all operations to return null or empty array response on error fix(data): update all operations to return flattened response, null, or empty array response on error Apr 5, 2024
@david-mcafee david-mcafee requested a review from a team as a code owner April 5, 2024 21:19
@HuiSF HuiSF deleted the mcafd/fix-models-list-return-types branch September 27, 2024 16:25
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.

2 participants