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

Issue #4434 Implement context default request/response encodings. #4455

Merged

Conversation

janbartel
Copy link
Contributor

@janbartel janbartel commented Jan 6, 2020

Implement the new servlet spec web.xml settings of request-character-encoding/response-character-encoding and equivalent ServletContext methods.

Closes #4434

Signed-off-by: Jan Bartel <janb@webtide.com>
@janbartel janbartel added the Specification For all industry Specifications (IETF / Servlet / etc) label Jan 6, 2020
@janbartel janbartel requested review from joakime and removed request for gregw January 6, 2020 06:40
Signed-off-by: Jan Bartel <janb@webtide.com>
@janbartel janbartel requested a review from sbordet January 7, 2020 03:02
Signed-off-by: Jan Bartel <janb@webtide.com>
@sbordet
Copy link
Contributor

sbordet commented Jan 7, 2020

@janbartel besides the "default" discussion, the build failed?

…4434-default-request-response-encodings

Signed-off-by: Jan Bartel <janb@webtide.com>
@sbordet
Copy link
Contributor

sbordet commented Jan 9, 2020

@janbartel I still don't like "default" and I would like it to be removed.

Other than that, do we need changes in webdefault.xml, explicitly adding these 2 elements?

@janbartel
Copy link
Contributor Author

I don't understand your problem with having "default" in front of the name of the default request and response character encodings? That is what they are - defaults for the context. The servlet spec api is inconsistent: sometimes it uses "default" in method names eg getDefaultSessionTrackingModes() and web.xml elements eg default-context-path, sometimes it doesn't. However, the javadoc always makes clear that these are defaults and can be overridden by other means. I don't see why if the spec is inconsistent in naming we should be too in our own impl methods.

As to your question about webdefault.xml, I don't see why we would need to add to it?

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@sbordet
Copy link
Contributor

sbordet commented Jan 15, 2020

@janbartel it's not a problem, just I don't like it because it is only ever used in that one place for the encoding, all other places don't have "default" in it.

As to your question about webdefault.xml, I don't see why we would need to add to it?

I was asking whether we must provide a default or not - apparently not.
webdefault.xml was the only other place I thought could be affected by this change, so I asked.

@janbartel janbartel merged commit 86e3333 into jetty-10.0.x Jan 16, 2020
@janbartel janbartel deleted the jetty-10-0.x-4434-default-request-response-encodings branch January 16, 2020 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Specification For all industry Specifications (IETF / Servlet / etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement ServletContext.get/setRequestCharacterEncoding and ServletContext.get/setResponseCharacterEncoding
3 participants