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

feat(api): expose HTTP response from API errors #12835

Merged
merged 12 commits into from
Jan 30, 2024

Conversation

AllanZhengYP
Copy link
Contributor

@AllanZhengYP AllanZhengYP commented Jan 11, 2024

Description of changes

Expose the error.response accessor from the errors thrown by REST API handlers and GraphQL queries.

Since the errors thrown by both REST APIs and GraphQL API share a same base error, We have to export the base ApiError from the core package.

Here's the new error structure. Boxes in green are new.
Before
Untitled2 drawio

After
Untitled drawio

Issue #, if available

#12730

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.

Comment on lines 17 to +19
// TODO: Delete the following 2 lines after we change the build target to >= es2015
this.constructor = APIError;
Object.setPrototypeOf(this, APIError.prototype);
this.constructor = GraphQLApiError;
Object.setPrototypeOf(this, GraphQLApiError.prototype);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think the current build target is es2020 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, we have quite a few places with this and I'm going to follow up with a PR. The only thing I need to make sure is we don't use es5 for UMD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like the UMD uses the es6 class keyword. Will remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually not quite. Removing this will break the current Jest test. We have defer it to the Jest upgrade.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a new one for me. Got a link explaining what the issue is in more detail?

packages/core/src/errors/APIError.ts Outdated Show resolved Hide resolved
packages/core/src/errors/APIError.ts Show resolved Hide resolved
packages/core/src/errors/APIError.ts Show resolved Hide resolved
packages/api-rest/src/utils/serviceError.ts Outdated Show resolved Hide resolved
packages/api-rest/src/utils/serviceError.ts Outdated Show resolved Hide resolved
@AllanZhengYP AllanZhengYP force-pushed the feat-api-error branch 2 times, most recently from c1fdb0f to 2bf22dd Compare January 16, 2024 21:01
Co-authored-by: Hui Zhao <10602282+HuiSF@users.noreply.github.com>
@AllanZhengYP AllanZhengYP requested a review from a team as a code owner January 17, 2024 16:48
Copy link
Contributor

@ashika112 ashika112 left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Jan 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9f41cdb) 88.00% compared to head (192971b) 87.28%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12835      +/-   ##
==========================================
- Coverage   88.00%   87.28%   -0.72%     
==========================================
  Files         663      635      -28     
  Lines       18141    13446    -4695     
  Branches     3635     2462    -1173     
==========================================
- Hits        15965    11737    -4228     
+ Misses       2176     1709     -467     
Flag Coverage Δ
adapter-nextjs 98.64% <ø> (ø)
analytics 62.75% <ø> (ø)
api ∅ <ø> (∅)
api-graphql 87.71% <ø> (ø)
api-rest 95.95% <ø> (-0.33%) ⬇️
auth 86.78% <ø> (+0.06%) ⬆️
aws-amplify 100.00% <ø> (ø)
core 87.72% <ø> (-0.31%) ⬇️
datastore ?
datastore-storage-adapter 88.13% <ø> (ø)
geo 90.12% <ø> (ø)
interactions 96.10% <ø> (ø)
notifications 90.35% <ø> (ø)
predictions 86.00% <ø> (ø)
pubsub 94.70% <ø> (ø)
rtn-push-notification 100.00% <ø> (ø)
storage 91.28% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@jimblanc jimblanc left a comment

Choose a reason for hiding this comment

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

Very much appreciate the detailed PR description, thank you!

Comment on lines 17 to +19
// TODO: Delete the following 2 lines after we change the build target to >= es2015
this.constructor = APIError;
Object.setPrototypeOf(this, APIError.prototype);
this.constructor = GraphQLApiError;
Object.setPrototypeOf(this, GraphQLApiError.prototype);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a new one for me. Got a link explaining what the issue is in more detail?

@AllanZhengYP
Copy link
Contributor Author

Hey, @svidgen

Re: #12835 (comment), it was mainly because I need to make sure both api-rest and api-graphql package throws APIError . Not just api-graphql .
It also preserve the consistency of the error naming. api-rest package throws RestApiError , similarly, the api-graphql package throws GraphQLApiError .
Both of RestApiError and GraphQLApiError are internal, so it shouldn’t affect any usage.

@AllanZhengYP AllanZhengYP merged commit 38c1d56 into aws-amplify:main Jan 30, 2024
30 checks passed
cshfang added a commit that referenced this pull request Feb 5, 2024
* chore(auth): debounce refreshAuthTokens (#12845)

* chore: add debounce callback helper

* chore: add unit tests

* chore: debounce fetchAuthSession

* chore: fix unit test

* chore: fix bundle size limits

* chore: update debounce logic

* chore: update dedup logic

* chore: debounce refreshAuthTokens

* chore: fix bundle size

* chore: address feedback

* chore: fix unit test

* chore: address feedback

* chore: update yarn.lock

* chore: address feedbak

* fix(data): update DataStore to send correct Control Messages when starting (#12861)

* chore(release): Publish [ci skip]

 - @aws-amplify/adapter-nextjs@1.0.13
 - @aws-amplify/analytics@7.0.13
 - @aws-amplify/api@6.0.13
 - @aws-amplify/api-graphql@4.0.13
 - @aws-amplify/api-rest@4.0.13
 - @aws-amplify/auth@6.0.13
 - aws-amplify@6.0.13
 - @aws-amplify/core@6.0.13
 - @aws-amplify/datastore@5.0.13
 - @aws-amplify/datastore-storage-adapter@2.1.13
 - @aws-amplify/geo@3.0.13
 - @aws-amplify/interactions@6.0.13
 - @aws-amplify/notifications@2.0.13
 - @aws-amplify/predictions@6.0.13
 - @aws-amplify/pubsub@6.0.13
 - @aws-amplify/react-native@1.0.13
 - @aws-amplify/react-native-example@0.0.14
 - @aws-amplify/rtn-push-notification@1.2.13
 - @aws-amplify/rtn-web-browser@1.0.13
 - @aws-amplify/storage@6.0.13
 - tsc-compliance-test@0.1.13

* chore(release): update API docs [ci skip]

* chore: enable codecov (#12876)

* docs(auth,analytics): adding in-line docs for public apis (#12882)

* docs(auth,analytics): adding in-line docs for public apis

* fix: unblock the max length lint

* chore(react-native): use react-native 0.71.0 as the dev dep

* fix: Default branch resolution when running E2E tests (#12910)

* fix: Lambda auth config value (#12922)

* chore: renable caching the package list in GH actions

* chore: remove unused codecov package (has been deprecated)

* feat(api): expose HTTP response from API errors (#12835)

---------

Co-authored-by: Hui Zhao <10602282+HuiSF@users.noreply.github.com>
Co-authored-by: Jim Blanchard <jim.l.blanchard@gmail.com>

* chore: temporarily disable codecov GH action integration (#12928)

* ci: run flaky data e2es without retry (#12758)

* ci: run flaky data e2es without retry

* only run on chrome

* restore workflow

---------

Co-authored-by: Aaron S <94858815+stocaaro@users.noreply.github.com>

* fix(core): Amplify.configure dispatches Hub event with unparsed config object (#12930)

* chore(repo): use typescript 5.0.2 across workspace

* chore(repo): refactor tsconfig hierarchy

- move the base tsconfig into the root of the workspace
- unifying tsconfig tsconfig.build tsconfig.test and tsconfig.watch settings

* chore(repo): promote rollup dependencies into workspace

- remove unused build.js script

* chore(repo): add build:watch using rollup

* chore(repo): setup eslint

* chore(adapter-nextjs): migrate to eslint

* fix(repo): test-github-actions using js-yaml already removed API

* chore(repo): add Code Spell Checker to recommended extension list

* chore(repo): add formatOn actions to formattingToggle controlables

* fix: not appending notification configs

* chore(release): Publish [ci skip]

 - @aws-amplify/adapter-nextjs@1.0.14
 - @aws-amplify/analytics@7.0.14
 - @aws-amplify/api@6.0.14
 - @aws-amplify/api-graphql@4.0.14
 - @aws-amplify/api-rest@4.0.14
 - @aws-amplify/auth@6.0.14
 - aws-amplify@6.0.14
 - @aws-amplify/core@6.0.14
 - @aws-amplify/datastore@5.0.14
 - @aws-amplify/datastore-storage-adapter@2.1.14
 - @aws-amplify/geo@3.0.14
 - @aws-amplify/interactions@6.0.14
 - @aws-amplify/notifications@2.0.14
 - @aws-amplify/predictions@6.0.14
 - @aws-amplify/pubsub@6.0.14
 - @aws-amplify/react-native@1.0.14
 - @aws-amplify/react-native-example@0.0.15
 - @aws-amplify/rtn-push-notification@1.2.14
 - @aws-amplify/rtn-web-browser@1.0.14
 - @aws-amplify/storage@6.0.14
 - tsc-compliance-test@0.1.14

* Revert "chore(repo): use typescript 5.0.2 across workspace" (#12941)

* Revert "chore(repo): use typescript 5.0.2 across workspace"

This reverts commit e20782e.

* chore(api-graphql): temporarily install rollup to package

* fix(repo): rollup generated sourcemap has wrong src path (#12947)

* fix(datastore): Treat head as optional to avoid errors (#12936)

* fix(datastore): Treat head as optional to avoid errors

* Add testing and fix follow-on issue

* fix(api-graphql): wrong arguments for GET operation of a CPK model

* fix: Remove special e2e treatment for validated tests (#12946)

fix: Remove special integ treament for validated tests

* fix(datastore): Limit docs integ test to one browser to improve stability (#12937)

* fix(api-graphql): same results returned for queries on the same model with different selection set

- caused by the incomplete GraphQL documents caching

* Update branch to work with merged updates from main

---------

Co-authored-by: israx <70438514+israx@users.noreply.github.com>
Co-authored-by: David McAfee <mcafd@amazon.com>
Co-authored-by: Jim Blanchard <jim.l.blanchard@gmail.com>
Co-authored-by: aws-amplify-bot <aws@amazon.com>
Co-authored-by: Hui Zhao <10602282+HuiSF@users.noreply.github.com>
Co-authored-by: AllanZhengYP <zheallan@amazon.com>
Co-authored-by: Hui Zhao <zhohz@amazon.com>
Co-authored-by: Ivan Artemiev <29709626+iartemiev@users.noreply.github.com>
Co-authored-by: Aaron S <94858815+stocaaro@users.noreply.github.com>
Co-authored-by: ManojNB <manojnb95@gmail.com>
Co-authored-by: ManojNB <manojnb@amazon.com>
Co-authored-by: Francisco Rodriguez <frodriguez.cs@gmail.com>
cshfang added a commit that referenced this pull request Feb 5, 2024
* chore(auth): debounce refreshAuthTokens (#12845)

* chore: add debounce callback helper

* chore: add unit tests

* chore: debounce fetchAuthSession

* chore: fix unit test

* chore: fix bundle size limits

* chore: update debounce logic

* chore: update dedup logic

* chore: debounce refreshAuthTokens

* chore: fix bundle size

* chore: address feedback

* chore: fix unit test

* chore: address feedback

* chore: update yarn.lock

* chore: address feedbak

* fix(data): update DataStore to send correct Control Messages when starting (#12861)

* chore(release): Publish [ci skip]

 - @aws-amplify/adapter-nextjs@1.0.13
 - @aws-amplify/analytics@7.0.13
 - @aws-amplify/api@6.0.13
 - @aws-amplify/api-graphql@4.0.13
 - @aws-amplify/api-rest@4.0.13
 - @aws-amplify/auth@6.0.13
 - aws-amplify@6.0.13
 - @aws-amplify/core@6.0.13
 - @aws-amplify/datastore@5.0.13
 - @aws-amplify/datastore-storage-adapter@2.1.13
 - @aws-amplify/geo@3.0.13
 - @aws-amplify/interactions@6.0.13
 - @aws-amplify/notifications@2.0.13
 - @aws-amplify/predictions@6.0.13
 - @aws-amplify/pubsub@6.0.13
 - @aws-amplify/react-native@1.0.13
 - @aws-amplify/react-native-example@0.0.14
 - @aws-amplify/rtn-push-notification@1.2.13
 - @aws-amplify/rtn-web-browser@1.0.13
 - @aws-amplify/storage@6.0.13
 - tsc-compliance-test@0.1.13

* chore(release): update API docs [ci skip]

* chore: enable codecov (#12876)

* docs(auth,analytics): adding in-line docs for public apis (#12882)

* docs(auth,analytics): adding in-line docs for public apis

* fix: unblock the max length lint

* chore(react-native): use react-native 0.71.0 as the dev dep

* fix: Default branch resolution when running E2E tests (#12910)

* fix: Lambda auth config value (#12922)

* chore: renable caching the package list in GH actions

* chore: remove unused codecov package (has been deprecated)

* feat(api): expose HTTP response from API errors (#12835)

---------

Co-authored-by: Hui Zhao <10602282+HuiSF@users.noreply.github.com>
Co-authored-by: Jim Blanchard <jim.l.blanchard@gmail.com>

* chore: temporarily disable codecov GH action integration (#12928)

* ci: run flaky data e2es without retry (#12758)

* ci: run flaky data e2es without retry

* only run on chrome

* restore workflow

---------

Co-authored-by: Aaron S <94858815+stocaaro@users.noreply.github.com>

* fix(core): Amplify.configure dispatches Hub event with unparsed config object (#12930)

* chore(repo): use typescript 5.0.2 across workspace

* chore(repo): refactor tsconfig hierarchy

- move the base tsconfig into the root of the workspace
- unifying tsconfig tsconfig.build tsconfig.test and tsconfig.watch settings

* chore(repo): promote rollup dependencies into workspace

- remove unused build.js script

* chore(repo): add build:watch using rollup

* chore(repo): setup eslint

* chore(adapter-nextjs): migrate to eslint

* fix(repo): test-github-actions using js-yaml already removed API

* chore(repo): add Code Spell Checker to recommended extension list

* chore(repo): add formatOn actions to formattingToggle controlables

* fix: not appending notification configs

* chore(release): Publish [ci skip]

 - @aws-amplify/adapter-nextjs@1.0.14
 - @aws-amplify/analytics@7.0.14
 - @aws-amplify/api@6.0.14
 - @aws-amplify/api-graphql@4.0.14
 - @aws-amplify/api-rest@4.0.14
 - @aws-amplify/auth@6.0.14
 - aws-amplify@6.0.14
 - @aws-amplify/core@6.0.14
 - @aws-amplify/datastore@5.0.14
 - @aws-amplify/datastore-storage-adapter@2.1.14
 - @aws-amplify/geo@3.0.14
 - @aws-amplify/interactions@6.0.14
 - @aws-amplify/notifications@2.0.14
 - @aws-amplify/predictions@6.0.14
 - @aws-amplify/pubsub@6.0.14
 - @aws-amplify/react-native@1.0.14
 - @aws-amplify/react-native-example@0.0.15
 - @aws-amplify/rtn-push-notification@1.2.14
 - @aws-amplify/rtn-web-browser@1.0.14
 - @aws-amplify/storage@6.0.14
 - tsc-compliance-test@0.1.14

* Revert "chore(repo): use typescript 5.0.2 across workspace" (#12941)

* Revert "chore(repo): use typescript 5.0.2 across workspace"

This reverts commit e20782e.

* chore(api-graphql): temporarily install rollup to package

* fix(repo): rollup generated sourcemap has wrong src path (#12947)

* fix(datastore): Treat head as optional to avoid errors (#12936)

* fix(datastore): Treat head as optional to avoid errors

* Add testing and fix follow-on issue

* fix(api-graphql): wrong arguments for GET operation of a CPK model

* fix: Remove special e2e treatment for validated tests (#12946)

fix: Remove special integ treament for validated tests

* fix(datastore): Limit docs integ test to one browser to improve stability (#12937)

* fix(api-graphql): same results returned for queries on the same model with different selection set

- caused by the incomplete GraphQL documents caching

* Update branch to work with merged updates from main

---------

Co-authored-by: israx <70438514+israx@users.noreply.github.com>
Co-authored-by: David McAfee <mcafd@amazon.com>
Co-authored-by: Jim Blanchard <jim.l.blanchard@gmail.com>
Co-authored-by: aws-amplify-bot <aws@amazon.com>
Co-authored-by: Hui Zhao <10602282+HuiSF@users.noreply.github.com>
Co-authored-by: AllanZhengYP <zheallan@amazon.com>
Co-authored-by: Hui Zhao <zhohz@amazon.com>
Co-authored-by: Ivan Artemiev <29709626+iartemiev@users.noreply.github.com>
Co-authored-by: Aaron S <94858815+stocaaro@users.noreply.github.com>
Co-authored-by: ManojNB <manojnb95@gmail.com>
Co-authored-by: ManojNB <manojnb@amazon.com>
Co-authored-by: Francisco Rodriguez <frodriguez.cs@gmail.com>
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.

None yet

6 participants