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 support for Servlet API 2.5 #172

Closed
hanfak opened this issue Aug 21, 2017 · 6 comments
Closed

Add support for Servlet API 2.5 #172

hanfak opened this issue Aug 21, 2017 · 6 comments

Comments

@hanfak
Copy link

hanfak commented Aug 21, 2017

All versions of logbook depend on javax.servlet version 3.

Can you please add support for version 2.5 of javax.servlet?

We cannot easily upgrade the servlet version.

@whiskeysierra
Copy link
Collaborator

Can you provide a short list of changes between 2.5 and 3.0? I have never used either directly myself but rather via some frameworks.

@whiskeysierra whiskeysierra changed the title logbook filter for javax.servlet 2.5 Add support for Servlet API 2.5 Aug 21, 2017
@hanfak
Copy link
Author

hanfak commented Aug 21, 2017

Some things we noticed (but not exhaustive):

  • Need to call getParameterMap in the RemoteRequest BEFORE withBody is called in order to ensure the query string/form content is parsed ready for getParameterMap calls. However, this consumes the input stream. This issue seems not related to 2.5 vs 3 but was an existing logbook bug. Not sure how to solve this one...
  • No async (e.g. HttpServletRequest getDispatcherType, isAsyncStarted)
  • HttpServletResponseWrapper does not have getStatus, getHeaderNames, getHeaders

Also, it would be useful to keep LogbookFilter non final, so that it can be extended in order to provide a default constructor with Logbook built up in that constructor (e.g. to use it in a web.xml file).

@whiskeysierra
Copy link
Collaborator

Need to call getParameterMap in the RemoteRequest BEFORE withBody is called in order to ensure the query string/form content is parsed ready for getParameterMap calls. However, this consumes the input stream. This issue seems not related to 2.5 vs 3 but was an existing logbook bug. Not sure how to solve this one...

This is indeed an existing known issue. See #94 and #169.

No async (e.g. HttpServletRequest getDispatcherType, isAsyncStarted)
HttpServletResponseWrapper does not have getStatus, getHeaderNames, getHeaders

Not having a compile time dependency to those methods (and enum values) will make things "a bit" ugly but technically doable. Spring does it for years, with some success at least.

Also, it would be useful to keep LogbookFilter non final, so that it can be extended in order to provide a default constructor with Logbook built up in that constructor (e.g. to use it in a web.xml file).

This is not really related to 2.5 but it would be a general feature. Would you expect that this newly created default constructor uses all the defaults for logbook? Or would you expect some customization options? Feel free to open another issue for this feature in particular.

In general, I'd be thrilled if you could open a PR that changes the minimum requirement to 2.5 and also gets rid of the obvious issues (e.g. just uncomment/hardcode/ignore certain features/tests) just so we see the real size of the change we're talking about.

hanfak pushed a commit to hanfak/logbook that referenced this issue Aug 22, 2017
…ay of testing against 2.5 in the build yet, suggestions welcome :)
@hanfak
Copy link
Author

hanfak commented Oct 18, 2017

Any ideas on next steps? We have already pushed a commit.

@whiskeysierra
Copy link
Collaborator

I did not yet have the time to take a deeper look. Your PR is good first step. I'll try to take it as a foundation. I'll need to:

  • instruct travis to build it twice, for 2.5 and 3.0 repsectively
  • ensure code coverage

@whiskeysierra
Copy link
Collaborator

See comment in #182

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

No branches or pull requests

2 participants