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

Better error responses #720

Merged
merged 32 commits into from Oct 1, 2019

Conversation

@shreyasthiagaraj
Copy link
Collaborator

commented Sep 3, 2019

As a Blockstack developer I'd like to get better error codes when operations fail. This PR implements #442 in Typescript as a starting point.

  • Fixes #436 with a DecryptionFailedError
  • Adds a proper ERROR_CODE to all instances of BlockstackException
  • #413 by adding a URL to exception messages with a pre-filled URL that points to blockstack.org/bugs with some HTTP parameters that auto-fill the bug form.
  • Fixes #665 by returning HTTP response code + error description for storage methods.
  • Includes json/string information returned from Gaia in error responses.
  • Fixes #601 by returning error instead of null when file is not found.
@moxiegirl

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

@shreyasthiagaraj I believe @timstackblock wants the bugs filed in GitHub not the forum --- which is where your PR is going to send them. Tim, I may have misunderstood you.

@yknl yknl added this to the DX Q3 Sprint 2 milestone Sep 3, 2019
@yknl yknl requested review from zone117x and yknl Sep 3, 2019
@shreyasthiagaraj

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 9, 2019

@moxiegirl I don't understand. My PR will send bugs to the forum?

@moxiegirl

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

@moxiegirl I don't understand. My PR will send bugs to the forum?

Yes, @shreyasthiagaraj . if you take the link you refer to in the description (blockstack.org/bugs) and in the code, the link redirects to this: https://forum.blockstack.org which is the forum.

src/storage/hub.ts Outdated Show resolved Hide resolved
src/errors.ts Outdated Show resolved Hide resolved
src/encryption/ec.ts Outdated Show resolved Hide resolved
Copy link
Member

left a comment

Looking good so far! I left some comments for potential changes & fixes

@yknl yknl modified the milestones: DX Q3 Sprint 2, DX Q3 Sprint 3 Sep 16, 2019
@shreyasthiagaraj shreyasthiagaraj changed the title [WIP] Better error codes (typescript) Better error codes (typescript) Sep 17, 2019
@shreyasthiagaraj shreyasthiagaraj changed the title Better error codes (typescript) Better error responses Sep 17, 2019
src/storage/hub.ts Outdated Show resolved Hide resolved
src/storage/hub.ts Outdated Show resolved Hide resolved
@yknl yknl referenced this pull request Sep 17, 2019
@shreyasthiagaraj

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 22, 2019

@zone117x I fixed 1 test that was obviously the result of my changes, but there are 10 more falling that seem totally unrelated. Is this a known issue, or should all tests currently be passing? Thanks!

src/storage/hub.ts Outdated Show resolved Hide resolved
@codecov

This comment has been minimized.

Copy link

commented Sep 24, 2019

Codecov Report

Merging #720 into develop will increase coverage by 5.84%.
The diff coverage is 72.61%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #720      +/-   ##
===========================================
+ Coverage    69.16%   75.01%   +5.84%     
===========================================
  Files           57       57              
  Lines         3357     2914     -443     
  Branches       602      222     -380     
===========================================
- Hits          2322     2186     -136     
+ Misses         748      701      -47     
+ Partials       287       27     -260
Impacted Files Coverage Δ
src/storage/hub.ts 75% <100%> (-0.68%) ⬇️
src/utils.ts 55.04% <63.15%> (+7.98%) ⬆️
src/errors.ts 74.44% <69.04%> (-6.92%) ⬇️
src/encryption/ec.ts 94.28% <80%> (+1.23%) ⬆️
src/storage/index.ts 77.47% <86.66%> (+3.39%) ⬆️
src/logger.ts 73.68% <0%> (-6.32%) ⬇️
src/profiles/profileSchemas/creativework.ts 30.76% <0%> (-4.95%) ⬇️
src/profiles/profileSchemas/organization.ts 30.76% <0%> (-4.95%) ⬇️
src/profiles/services/serviceUtils.ts 77.77% <0%> (-2.23%) ⬇️
src/profiles/services/github.ts 84.61% <0%> (-1.1%) ⬇️
... and 42 more

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 c2ac2c6...2d8cc2b. Read the comment docs.

@shreyasthiagaraj

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 24, 2019

Fixed all failing tests, and modified the getFile throw on 404 test to check some of the values that the newly introduced GaiaHubError should contain. Let me know if it looks good, thanks!

src/storage/index.ts Outdated Show resolved Hide resolved
@zone117x

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

This looks great! We should note in the changelog of breaking change: getFile throws error on 404 rather than returning null.

Adding @timstackblock as reviewer to perform the regular smoke tests.

We should also test something like getFile('bogus123_not_exists') results in a useful console log message.

@zone117x zone117x requested a review from timstackblock Sep 25, 2019
Copy link
Member

left a comment

Left a last comment where a line of code can be removed. Other than that, code looks good.

@yknl
yknl approved these changes Oct 1, 2019
Copy link
Collaborator

left a comment

This looks good to me 👍

@shreyasthiagaraj shreyasthiagaraj merged commit 679ebda into develop Oct 1, 2019
2 checks passed
2 checks passed
ci/circleci Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.