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: api errors related to postage stamps #2037

Merged
merged 5 commits into from
Jun 10, 2021
Merged

fix: api errors related to postage stamps #2037

merged 5 commits into from
Jun 10, 2021

Conversation

aloknerurkar
Copy link
Contributor

@aloknerurkar aloknerurkar commented Jun 9, 2021

This PR attempts to solve #2026 in part.

This particular change tries to address the postage bucket full error which is annoying as the user does not understand that he cannot use the batch anymore.

We can create a separate errors.go file in pkg/api which could house all these error case handlers. This way we can edit the same file from time to time to add special case handling.

@acud @agazso @AuHau @Eknir


This change is Reviewable

@agazso
Copy link
Member

agazso commented Jun 9, 2021

Sending PSS message with the postage bucket full may lead to similar issue (see #1988).

@aloknerurkar do you plan to address that in this PR as well?

Copy link
Member

@zelig zelig left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @acud, @AuHau, and @Eknir)

Copy link
Contributor

@mrekucci mrekucci left a comment

Choose a reason for hiding this comment

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

Since the jsonhttp.PaymentRequired returns 402, all the affected endpoint definitions in openapi/Swarm.yaml should be also adjusted accordingly.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @acud, @AuHau, and @Eknir)

Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 4 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aloknerurkar, @AuHau, and @Eknir)


pkg/api/bzz.go, line 36 at r2 (raw file):

)

func mappedHTTPErr(w http.ResponseWriter, e error, defaultMsg interface{}) {

i think that having such a pattern would be useful if all middlewares would just return an error to an upper layer middleware that would then translate the actual errors into a response. but since we don't do this I would vote for explicitly handling the errors within the handlers

@aloknerurkar
Copy link
Contributor Author


pkg/api/bzz.go, line 36 at r2 (raw file):

Previously, acud (acud) wrote…

i think that having such a pattern would be useful if all middlewares would just return an error to an upper layer middleware that would then translate the actual errors into a response. but since we don't do this I would vote for explicitly handling the errors within the handlers

Done.

Copy link
Contributor Author

@aloknerurkar aloknerurkar left a comment

Choose a reason for hiding this comment

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

Dismissed @acud from a discussion.
Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @acud, @AuHau, @Eknir, and @zelig)

Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @AuHau, @Eknir, and @mrekucci)

Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @AuHau, @Eknir, and @mrekucci)

Copy link
Contributor

@mrekucci mrekucci left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 7 of 7 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @AuHau and @Eknir)

@aloknerurkar aloknerurkar merged commit 16551b8 into master Jun 10, 2021
@aloknerurkar aloknerurkar deleted the apiErrors.0 branch June 10, 2021 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants