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

chore: validation checks bzz and stewardship #3170

Merged
merged 11 commits into from
Aug 23, 2022

Conversation

umerm-work
Copy link
Contributor

@umerm-work umerm-work commented Aug 11, 2022

Checklist

  • I have read the coding guide
  • My change requires a documentation update and I have done it
  • I have added tests to cover my changes.
  • I have filled out the description and linked the related issues

Description

if the reference is incorrect (not 64 or 128 hex characters), return 400
if the reference is correct but not found, then return 404
Remove bzz patch endpoint as it is deprecated.

Closes #1803

Motivation and context (Optional)

Related Issue (Optional)

Screenshots (if appropriate):


This change is Reviewable

@notanatol notanatol force-pushed the reupload-input-validation branch from 821859e to 117a45e Compare August 11, 2022 13:53
@umerm-work umerm-work self-assigned this Aug 12, 2022
@umerm-work umerm-work marked this pull request as ready for review August 15, 2022 10:01
@umerm-work umerm-work added the ready for review The PR is ready to be reviewed label Aug 15, 2022
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.

Please reflect the API changes also in the openapi specifications.

@umerm-work
Copy link
Contributor Author

Done

pkg/api/stewardship.go Outdated Show resolved Hide resolved
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.

Reviewed 5 of 6 files at r1, 2 of 2 files at r3, 5 of 5 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: 14 of 15 files reviewed, 2 unresolved discussions (waiting on @umerm-work)


pkg/api/stewardship.go line 36 at r5 (raw file):

		s.logger.Debug("stewardship put: service unavailable", "string", nameOrHex, "error", err)
		s.logger.Error(nil, "stewardship put: service unavailable")
		jsonhttp.ServiceUnavailable(w, nil)

This error code can be misleading because it is not clear from the response what service is not available. We also do not try to be specific in the response code. I would suggest returning a simple code 500 with the following message, "stewardship put: resolver service unavailable".

Copy link
Contributor Author

@umerm-work umerm-work left a comment

Choose a reason for hiding this comment

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

Reviewable status: 14 of 15 files reviewed, 1 unresolved discussion (waiting on @agazso and @umerm-work)


pkg/api/stewardship.go line 41 at r5 (raw file):

Previously, agazso (Attila Gazso) wrote…

Why do we use a quite obscure HTTP error number (422 Unprocessable Entity) here when there is a simple well-known alternative to it (400 Bad Request)?

As far as I understand we don't even use 422 correctly and that may be also because it's obscure.

The 422 (Unprocessable Content) status code indicates that the server understands the content type of the request content (hence a 415 (Unsupported Media Type) status code is inappropriate), and the syntax of the request content is correct, but it was unable to process the contained instructions. For example, this status code can be sent if an XML request content contains well-formed (i.e., syntactically correct), but semantically erroneous XML instructions.
https://httpwg.org/specs/rfc9110.html#status.422

Done.

@umerm-work umerm-work requested review from mrekucci and agazso August 19, 2022 10:15
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.

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @agazso)

@umerm-work umerm-work added this to the 1.7.1 milestone Aug 23, 2022
@umerm-work umerm-work merged commit ba6f2bf into master Aug 23, 2022
@umerm-work umerm-work deleted the reupload-input-validation branch August 23, 2022 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull-request ready for review The PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve input validation for BZZ Reupload
5 participants