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

http: support additional content type #903

Merged
merged 6 commits into from Feb 2, 2021

Conversation

@oncilla
Copy link
Contributor

@oncilla oncilla commented Jan 8, 2021

Support application/x-www-form-urlencoded as an additional content
type for AtomicLevel.ServeHTTP.

This is the default content type for curl -X POST.

With this change, interacting with the HTTP endpoint is a bit more
user friendly:

curl -X PUT localhost:8080/log/level -d level=debug

Additionally, the unit tests for the HTTP handler are transformed to
a table driven approach.

fixes #902


This change is Reviewable

Support `application/x-www-form-urlencoded` as an additional content
type for `AtomicLevel.ServeHTTP`.

This is the default content type for `curl -X POST`.

With this change, interacting with the HTTP endpoint is a bit more
user friendly:

```
curl -X PUT localhost:8080/log/level -d level=debug
```

Additionally, the unit tests for the HTTP handler are transformed to
a table driven approach.

fixes #902
@CLAassistant
Copy link

@CLAassistant CLAassistant commented Jan 8, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

@codecov codecov bot commented Jan 8, 2021

Codecov Report

Merging #903 (86ae38e) into master (f8ef926) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #903   +/-   ##
=======================================
  Coverage   98.22%   98.23%           
=======================================
  Files          44       44           
  Lines        1915     1925   +10     
=======================================
+ Hits         1881     1891   +10     
  Misses         27       27           
  Partials        7        7           
Impacted Files Coverage Δ
http_handler.go 100.00% <100.00%> (ø)

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 f8ef926...86ae38e. Read the comment docs.

Copy link
Contributor

@abhinav abhinav left a comment

Thanks for the change! The core logic looks mostly good. I have some suggestions around style.

// request could look like this:
//
// curl -X PUT localhost:8080/log/level -d level=debug
//
// For any other content type, the payload is expected to be JSON encoded and
// look like:
//
// {"level":"info"}
//
// An example curl request could look like this:
//
// curl -X PUT localhost:8080/log/level -H "Content-Type: application/json" -d '{"level":"debug"}'
Comment on lines 54 to 65

This comment has been minimized.

@abhinav

abhinav Jan 8, 2021
Contributor

This is great documentation, thank you!

}
if req.Level == nil {
return "Must specify a logging level."
requestedLvl, err := func(body io.Reader) (*zapcore.Level, error) {

This comment has been minimized.

@abhinav

abhinav Jan 8, 2021
Contributor

This appears to be an effort to reduce error handling duplication. That's a good call but can we perhaps move this to its own unexported method?

requestedLevel, err := lvl.decodePutRequest(r)

Additionally, since zapcore.Level is just an int8, we don't need to return its address:

func (lvl AtomicLevel) decodePutRequest(*http.Request) (zapcore.Level, error)

It's a pointer in payload to detect the absence of the level in the JSON payload. So we can validate it's non-nil on the JSON path and dereference it.

This comment has been minimized.

@oncilla

oncilla Jan 11, 2021
Author Contributor

Good point. I tried to follow the previous style, but your suggestion is more maintainable and readable IMO.

if err := l.UnmarshalText([]byte(lvl)); err != nil {
return nil, err
}
return &l, nil
Comment on lines 100 to 103

This comment has been minimized.

@abhinav

abhinav Jan 8, 2021
Contributor

Optionally, this can be shortened.

Suggested change
if err := l.UnmarshalText([]byte(lvl)); err != nil {
return nil, err
}
return &l, nil
err := l.UnmarshalText([]byte(lvl))
return &l, err

This comment has been minimized.

@oncilla

oncilla Jan 11, 2021
Author Contributor

I prefer being explicit here. Especially after switching to value type.

It is not immediately obvious whether the returned value is pre- or post-UnmarshalText without knowing that part of the spec by hart. (TBH I would need to look that up)

tests := map[string]struct {
Method string
ContentType string
Body string
ExpectedCode int
ExpectedLevel zapcore.Level
}{
Comment on lines 38 to 44

This comment has been minimized.

@abhinav

abhinav Jan 8, 2021
Contributor

Thanks for making this a table test. If you wouldn't mind, we have a preference for slice-based table tests rather than maps:

tests :=  []struct {
  desc string
  method string
  contentType string
  ...
}{
  ...
}

for _, tt := range tests {
  ..

(See also https://github.com/uber-go/guide/blob/master/style.md#test-tables)

ts := httptest.NewServer(lvl)
defer ts.Close()

req, err := http.NewRequest(test.Method, ts.URL, strings.NewReader(test.Body))
Comment on lines 114 to 117

This comment has been minimized.

@abhinav

abhinav Jan 8, 2021
Contributor

can we name the test server srv or server?

Suggested change
ts := httptest.NewServer(lvl)
defer ts.Close()
req, err := http.NewRequest(test.Method, ts.URL, strings.NewReader(test.Body))
server := httptest.NewServer(lvl)
defer server.Close()
req, err := http.NewRequest(test.Method, server.URL, strings.NewReader(test.Body))

This comment has been minimized.

@oncilla

oncilla Jan 11, 2021
Author Contributor

Done

},
}

for name, test := range tests {

This comment has been minimized.

@abhinav

abhinav Jan 8, 2021
Contributor

For table tests, consider renaming the test cases to tt.

for _, tt := range tests {

enc.Encode(errorResponse{Error: err.Error()})
return
}

lvl.SetLevel(*req.Level)
enc.Encode(req)

lvl.SetLevel(*requestedLvl)
enc.Encode(payload{Level: requestedLvl})
Comment on lines 117 to 121

This comment has been minimized.

@abhinav

abhinav Jan 8, 2021
Contributor

Do we want a JSON output even for non-JSON input?

Maybe this is okay and we can add a plain text output later if we need it and the client has an Accepts: text/plain?

CC @prashantv

This comment has been minimized.

@oncilla

oncilla Jan 11, 2021
Author Contributor

If we want to return non-JSON in the response, I would definitively go for content type negotiation through the Accepts header.

In any case, I would vote for this to be part of a follow-up PR if it is desired.

@oncilla
Copy link
Contributor Author

@oncilla oncilla commented Jan 10, 2021

Thanks for the swift feedback. I hope I get around to address the comments this week.

oncilla and others added 2 commits Jan 11, 2021
These methods do not have need of the atomic level. Turn them into
functions to avoid accidentally manipulating or accessing it.
if contentType == "application/x-www-form-urlencoded" {
return lvl.decodePutURL(body)
}
return lvl.decodePutJSON(body)
Comment on lines 98 to 101

This comment has been minimized.

@abhinav

abhinav Jan 15, 2021
Contributor

Minor: It looks like these methods could be top-level functions since they don't do anything with the AtomicLevel. I'm gonna change that.

@abhinav
Copy link
Contributor

@abhinav abhinav commented Jan 15, 2021

Sorry for the delay in reviewing. This looks great!
I'm going to get a second review on this before merging.
Thanks for the change!

Copy link
Contributor

@prashantv prashantv left a comment

Change looks good, although one change we can make to simplify, but also provide an easier way to set the level: use ParseForm() and FormValue(..) to get the level field if it exists. It would simplify the code, while also allowing URL parameters to be used. Thoughts?

@oncilla
Copy link
Contributor Author

@oncilla oncilla commented Jan 25, 2021

@prashantv nice. Did not know that exists.

Should I also add tests for the URL params encoding?

@prashantv
Copy link
Contributor

@prashantv prashantv commented Jan 26, 2021

Should I also add tests for the URL params encoding?

Yes, that would be great if you could add a test for passing the parameter in a URL (and mention it in the docs). Thanks!

@oncilla
Copy link
Contributor Author

@oncilla oncilla commented Jan 26, 2021

Done.

Copy link
Contributor

@prashantv prashantv left a comment

Thanks for the contribution!

@prashantv prashantv merged commit b274f65 into uber-go:master Feb 2, 2021
4 checks passed
4 checks passed
@travis-ci
Travis CI - Pull Request Build Passed
Details
@codecov
codecov/patch 100.00% of diff hit (target 98.22%)
Details
@codecov
codecov/project 98.23% (target 95.00%)
Details
license/cla Contributor License Agreement is signed.
Details
abhinav added a commit that referenced this pull request May 25, 2021
Support `application/x-www-form-urlencoded` as an additional content
type for `AtomicLevel.ServeHTTP`.

This is the default content type for `curl -X POST`.

With this change, interacting with the HTTP endpoint is a bit more
user friendly:

```
curl -X PUT localhost:8080/log/level -d level=debug
```

Additionally, the unit tests for the HTTP handler are transformed to
a table driven approach.

fixes #902

Co-authored-by: Abhinav Gupta <abg@uber.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

4 participants