Skip to content

Conversation

@slockij
Copy link

@slockij slockij commented Sep 22, 2022

  • added check for custom unmarshal of json, text or binary in form binding

@appleboy appleboy added this to the v1.9 milestone Oct 11, 2022
appleboy
appleboy previously approved these changes Oct 11, 2022
@appleboy
Copy link
Member

Lint error.

@slockij slockij force-pushed the form-mapping-with-custom-unmarshal branch from 2952371 to 74734ca Compare October 11, 2022 07:49
@slockij slockij requested a review from appleboy October 11, 2022 07:50
@appleboy
Copy link
Member

@slockij Please rebase the master branch.

@slockij slockij force-pushed the form-mapping-with-custom-unmarshal branch from 74734ca to c59af23 Compare October 12, 2022 07:02
@appleboy appleboy requested a review from thinkerou October 16, 2022 01:36
@appleboy
Copy link
Member

@thinkerou help to review.

@codecov
Copy link

codecov bot commented Oct 16, 2022

Codecov Report

Merging #3335 (c59af23) into master (de1c4ec) will decrease coverage by 0.46%.
The diff coverage is 100.00%.

❗ Current head c59af23 differs from pull request most recent head 26c932b. Consider uploading reports for the commit 26c932b to get more accurate results

@@            Coverage Diff             @@
##           master    #3335      +/-   ##
==========================================
- Coverage   98.63%   98.18%   -0.46%     
==========================================
  Files          42       42              
  Lines        3151     3136      -15     
==========================================
- Hits         3108     3079      -29     
- Misses         29       42      +13     
- Partials       14       15       +1     
Flag Coverage Δ
98.18% <100.00%> (-0.46%) ⬇️
go-1.17 98.18% <100.00%> (?)
go-1.18 98.18% <100.00%> (?)
go-1.20 ?
macos-latest 98.18% <100.00%> (-0.46%) ⬇️
ubuntu-latest 98.18% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
binding/form_mapping.go 100.00% <100.00%> (ø)
render/toml.go 0.00% <0.00%> (-100.00%) ⬇️
context.go 97.45% <0.00%> (-0.52%) ⬇️
response_writer.go 93.47% <0.00%> (-0.40%) ⬇️
recovery.go 98.07% <0.00%> (-0.04%) ⬇️
gin.go 99.18% <0.00%> (-0.01%) ⬇️
tree.go 100.00% <0.00%> (ø)
routergroup.go 100.00% <0.00%> (ø)
binding/toml.go 100.00% <0.00%> (ø)
test_helpers.go 100.00% <0.00%> (ø)
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@slockij
Copy link
Author

slockij commented Nov 21, 2022

guys if I can help here let me know.

@thinkerou thinkerou modified the milestones: v1.9, v1.10 Jan 17, 2023
@slockij slockij force-pushed the form-mapping-with-custom-unmarshal branch from c59af23 to 26c932b Compare March 1, 2023 13:05
@appleboy appleboy modified the milestones: v1.10, v1.11 Mar 21, 2024
```

```console
$ curl "localhost:8085/[email protected]"
Copy link
Contributor

Choose a reason for hiding this comment

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

How come request query parameters are read into a json unmarshaller? What happens when json is posted in curl body?

}

type jsonUnmarshaler interface {
UnmarshalJSON([]byte) error
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason to use multiple different interfaces? Given that these will always execute in a specific order. JSON then TEXT then BINARY. If all 3 are defined only JSON one will be executed. So in this case only 1 interface would be enough to define the unmarshaller override behaviour. E.g. func Unmarshal([]byte) error

assert.Equal(t, "test", s.Email.Name)
assert.Equal(t, "example.org", s.Email.Host)

err = mappingByPtr(&s, formSource{"Email": {`not an email`}}, "form")
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally fuzzing would be a separate test. Right now it's not testing 1 unit of code. Also helps with readability.

@appleboy appleboy modified the milestones: v1.11, v1.12 Jun 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants