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

Add handling for empty content type in request header #2433

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Calgorr
Copy link

@Calgorr Calgorr commented Apr 14, 2023

Updated the code to handle the scenario when the content type in the request header is an empty string. Added a custom error to handle this scenario. No changes were made to other parts of the code.

@Calgorr
Copy link
Author

Calgorr commented Apr 14, 2023

so i was trying to build a rest api with echo and i did not know that i should set the Content-Type header in the http request so i was getting this error (unsupported media type) which was kind of misleading me as i was consistently changing the http body but the problem was that i did not know that i should include the Content-Type header
so after one hour i finally figured it out that i should set this header

by this little change hopefully no one gets stuck on the same problem that i got stuck on

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.

see notes and we are missing test(s) for this change

bind.go Show resolved Hide resolved
@Calgorr
Copy link
Author

Calgorr commented Apr 18, 2023

@aldas as you said it was not okay to return an error with 500 code as it was clients problem to set the content header so i fixed the problem and i added a test for this error

@Calgorr Calgorr requested a review from aldas April 19, 2023 05:25
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.

  • tests are failing. You can run test locally with make check

@@ -338,6 +338,7 @@ var (
ErrCookieNotFound = errors.New("cookie not found")
ErrInvalidCertOrKeyType = errors.New("invalid cert or key type, must be string or []byte")
ErrInvalidListenerNetwork = errors.New("invalid listener network")
ErrEmptyContentType = ErrUnsupportedMediaType.WithInternal(errors.New("missing content type header"))
Copy link
Contributor

Choose a reason for hiding this comment

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

please do not add new public error - I do not think this case is something that you would actively check. By making it public instead of just returning ErrUnsupportedMediaType.WithInternal(errors.New("missing content type header")) we are making contract library users that there is logic somewhere that returns that error and we can not change it. This is not probably this kind of situation where we should make such strong commitment.

Choose a reason for hiding this comment

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

Hi @aldas , Does this PR need some cleaning before becoming eligible for merge? I would like to take this, if possible.

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.

3 participants