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

feat: Bind data using headers as source #1866

Merged
merged 3 commits into from
May 25, 2021
Merged

Conversation

noseglid
Copy link
Contributor

@noseglid noseglid commented May 6, 2021

Currently, echo supports binding data from query, path or body.
Sometimes we need to read bind data from headers. It would be nice to
automatically bind those using the bindData func, which is already
well prepared to accept http.Header.

I didn't add this to the Bind func, so this will not happen
automatically. Main reason is backwards compatability. It might be
confusing if variables are bound from headers when upgrading, and might
even have become a security issue as pointed out in #1670.

Currently, echo supports binding data from query, path or body.
Sometimes we need to read bind data from headers. It would be nice to
automatically bind those using the `bindData` func, which is already
well prepared to accept `http.Header`.

I didn't add this to the `Bind` func, so this will not happen
automatically. Main reason is backwards compatability. It might be
confusing if variables are bound from headers when upgrading, and might
even have become a security issue as pointed out in labstack#1670.
@noseglid
Copy link
Contributor Author

noseglid commented May 6, 2021

I realize we should add documentation over this seeing as this is a new feature and all. But I wanted to get the PR up here to see if this is something you'd be willing to accept. And if so, we can discuss next steps.

bind.go Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 13, 2021

Codecov Report

Merging #1866 (11d6bf8) into master (6430665) will increase coverage by 0.64%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1866      +/-   ##
==========================================
+ Coverage   89.57%   90.22%   +0.64%     
==========================================
  Files          31       31              
  Lines        2735     2773      +38     
==========================================
+ Hits         2450     2502      +52     
+ Misses        184      173      -11     
+ Partials      101       98       -3     
Impacted Files Coverage Δ
bind.go 89.41% <100.00%> (+0.52%) ⬆️
middleware/timeout.go 100.00% <0.00%> (ø)
middleware/jwt.go 79.80% <0.00%> (+1.91%) ⬆️
middleware/static.go 70.00% <0.00%> (+2.83%) ⬆️
middleware/key_auth.go 96.55% <0.00%> (+29.88%) ⬆️

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 6430665...11d6bf8. Read the comment docs.

Copy link
Contributor

@aldas aldas left a comment

Choose a reason for hiding this comment

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

LGTM

@noseglid
Copy link
Contributor Author

Anything else you expect from me in this?

@lammel
Copy link
Contributor

lammel commented May 20, 2021

Anything else you expect from me in this?

Could you open a PR in https://github.com/labstack/echox/pulls to update the documentation please.
Nice work. We'll merge when the docs PR is linked.

@noseglid
Copy link
Contributor Author

labstack/echox#210

@lammel lammel merged commit 7846e3f into labstack:master May 25, 2021
@noseglid noseglid deleted the bind-headers branch May 29, 2021 07:30
@liaohongxing
Copy link

func hello(c echo.Context) error {
	c.Request().Header.Set("id", "1")

	input := new(struct {
		ID int `header:"id"`
	})

	if err := c.Bind(input); err != nil {
		return err
	}

	fmt.Println(input)

	return c.JSON(http.StatusOK, input)
}

result: {"ID":0} ,c.Bind lacks support

@aldas
Copy link
Contributor

aldas commented Jul 14, 2021

Yes, c.Bind() does not bind header. To bind headers you would need to use

		if err := (&echo.DefaultBinder{}).BindHeaders(c, input); err != nil {
			return err
		}

This is intentional as original author points out #1866 (comment)

@tunhuit
Copy link

tunhuit commented Jul 15, 2021

Should we add this feature to Bind() in Echo v5 or next release? It should have some notice about breaking compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants