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.

Sign up for GitHub

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

Jump to bottom

feat(api): simplify parsing and validation of http input params #3344

Merged
merged 9 commits into from Oct 19, 2022
Merged

feat(api): simplify parsing and validation of http input params #3344

merged 9 commits into from Oct 19, 2022

Conversation

mrekucci
Copy link
Contributor

@mrekucci mrekucci commented Sep 26, 2022 •
edited

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

Closes #3163 #3165 #3371

Open API Spec Version Changes (if applicable)

Swarm.yaml: minor
SwarmDebug.yaml: minor
SwarmCommon.yaml: minor

This change is Reviewable

@mrekucci mrekucci force-pushed the simplify-parsing-and-validation-of-http-input-params branch 11 times, most recently from 0d93a1b to 02b46de Compare September 30, 2022 14:46
@mrekucci mrekucci marked this pull request as ready for review September 30, 2022 16:50
This was referenced Sep 30, 2022
Closed
Closed
@mrekucci mrekucci force-pushed the simplify-parsing-and-validation-of-http-input-params branch from fde36e4 to 992ad1c Compare September 30, 2022 17:49
This was referenced Oct 3, 2022
Closed
Closed
aloknerurkar
aloknerurkar reviewed Oct 3, 2022
View reviewed changes
pkg/api/welcome_message.go Outdated
@@ -29,17 +29,19 @@ func (s *Service) getWelcomeMessageHandler(w http.ResponseWriter, r *http.Reques
}

func (s *Service) setWelcomeMessageHandler(w http.ResponseWriter, r *http.Request) {
logger := s.logger.WithName("post_wallet").Build()
Copy link
Contributor

Choose a reason for hiding this comment

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

copy-paste error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, fixed.

openapi/SwarmDebug.yaml
@@ -92,8 +92,8 @@ paths:
application/json:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think changes to SwarmDebug.yaml can be avoided as we should not be incrementing versions here when we plan to not support it going ahead.

Copy link
Contributor Author

@mrekucci mrekucci Oct 3, 2022 •
edited

Choose a reason for hiding this comment

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

Added the recommended semver changes to the PR description.

pkg/api/balances.go Outdated
balances, err := s.accounting.Balances()
if err != nil {
jsonhttp.InternalServerError(w, errCantBalances)
s.logger.Debug("balances: get balances failed", "error", err)
s.logger.Error(nil, "balances: get balances failed")
logger.Debug("balances: get balances failed", "error", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove "balances" from the msg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, done!

pkg/api/util.go Outdated
}

// mapStructure maps the input to the output values.
// The input is on of the following:
Copy link
Contributor

Choose a reason for hiding this comment

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

one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

notanatol
notanatol reviewed Oct 3, 2022
View reviewed changes
pkg/api/api.go
@@ -662,12 +752,12 @@ func equalASCIIFold(s, t string) bool {
// direct push to the network (default) a pushStamperPutter is returned.
// returns a function to wait on the errorgroup in case of a pushing stamper putter.
func (s *Service) newStamperPutter(r *http.Request) (storage.Storer, func() error, error) {
batch, err := requestPostageBatchId(r)
batch, err := requestPostageBatchId(r) // TODO: extrapolate the headers parsing to the handler level!
Copy link
Contributor

Choose a reason for hiding this comment

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

these TODOs are meant to be fixed in a future PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there is an existing issue for that: #3371

@mrekucci mrekucci force-pushed the simplify-parsing-and-validation-of-http-input-params branch from 2bedbed to 267e65b Compare October 3, 2022 12:48
@mrekucci mrekucci mentioned this pull request Oct 3, 2022
Closed
@mrekucci mrekucci force-pushed the simplify-parsing-and-validation-of-http-input-params branch from 25ea270 to 1e49a37 Compare October 3, 2022 13:33
@mrekucci mrekucci mentioned this pull request Oct 3, 2022
Merged
2 tasks
aloknerurkar
aloknerurkar approved these changes Oct 4, 2022
View reviewed changes
Copy link
Contributor

@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.

Looks really good!

@mrekucci mrekucci force-pushed the simplify-parsing-and-validation-of-http-input-params branch from 1e49a37 to 2fd3d46 Compare October 4, 2022 09:54
@mrekucci mrekucci force-pushed the simplify-parsing-and-validation-of-http-input-params branch from 2fd3d46 to 4a0b630 Compare October 4, 2022 11:16
@umerm-work
Copy link
Contributor

Really nice !

@ghost ghost mentioned this pull request Oct 9, 2022
Merged
4 tasks
vladopajic
vladopajic reviewed Oct 11, 2022
View reviewed changes
pkg/log/log.go
func MustParseVerbosityLevel(s string) Level {
l, err := ParseVerbosityLevel(s)
if err != nil {
panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

please handle error instead of panicing

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 Must pattern commonly used in the std lib (e.g. regexp.MustCompile). It is used in cases where the input has already been validated and simplifies initialization.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regardless of naming convention, having panic in code may lead to unexpected program termination. It is risky to be certain that some execution flows will not cause program to terminate.

Also, this rule is documented in coding style doc.

@mrekucci mrekucci force-pushed the simplify-parsing-and-validation-of-http-input-params branch from 7708749 to dbff8fb Compare October 18, 2022 00:47
@mrekucci mrekucci force-pushed the simplify-parsing-and-validation-of-http-input-params branch 6 times, most recently from d5e8513 to 25f9f1c Compare October 18, 2022 16:18
@mrekucci mrekucci force-pushed the simplify-parsing-and-validation-of-http-input-params branch from 25f9f1c to f9e5910 Compare October 18, 2022 16:53
@mrekucci mrekucci merged commit 4144cf2 into master Oct 19, 2022
@mrekucci mrekucci deleted the simplify-parsing-and-validation-of-http-input-params branch October 19, 2022 00:15
@aloknerurkar aloknerurkar added this to the 1.10.0 milestone Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vladopajic vladopajic vladopajic left review comments

@aloknerurkar aloknerurkar aloknerurkar approved these changes

@umerm-work umerm-work umerm-work approved these changes

@notanatol notanatol notanatol approved these changes

Assignees
No one assigned
Labels
pull-request
Projects
None yet
Milestone
1.10.0
Development

Successfully merging this pull request may close these issues.

Simplify HTTP API input parameters check
5 participants
@mrekucci @umerm-work @vladopajic @aloknerurkar @notanatol