• Bug
  • Status: Closed
  • 2 Major
  • Resolution: Fixed
  • ehcache-web
  • Reporter: edalquis
  • March 29, 2010
  • 1
  • Watchers: 2
  • July 15, 2010
  • April 29, 2010

Attachments

Description

CachingFilter uses GenericResponseWrapper to track information about the response that needs to be cached. It does not capture int and date headers that are set on the cached response. Also the PageInfo created from the GenericResponseWrapper tries to remove the “Content-Encoding” header from the set of cached response headers but it will never work as implemented since remove(String) is being called on a Collection<String[]>

The patch creates three Maps used to track the headers to cache using a simple Header bean to track the name and value of each. PageInfo also tracks the three different collections of headers and calls the appropriate addHeader, addIntHeader, or addDateHeader APIs. This change also reduces the need for use of custom SimpleDateFormat objects for date headers as the servlet spec provided setDateHeader simply takes a long value and does the formatting itself.

Please let me know if there are any issues with the patch. I’d be happy to refactor it further.

I believe I have an ICLA on file with Terracotta.

Comments

gluck 2010-03-30

Eric

I applied the patch and ran the tests with mvn -Ptest verify.

There are seven test failures. Can you please run the tests and take a look.

Failed tests: testContentTypeByHeader(net.sf.ehcache.constructs.web.filter.CachingFilterTest) testSetThenAddHeaders(net.sf.ehcache.constructs.web.filter.CachingFilterTest) testAddThenSetHeaders(net.sf.ehcache.constructs.web.filter.CachingFilterTest)

Tests in error: testUsedGunzipImplementationPerformance(net.sf.ehcache.constructs.web.PageInfoTest) testIfNotModifiedSinceCachingHeader(net.sf.ehcache.constructs.web.filter.SimpleCachingHeadersPageCachingFilterTest) testIfNotModifiedAndIfNoneMatchSinceCachingHeader(net.sf.ehcache.constructs.web.filter.SimpleCachingHeadersPageCachingFilterTest) testIfNoneMatchSinceAndIfNotModifiedCachingHeader(net.sf.ehcache.constructs.web.filter.SimpleCachingHeadersPageCachingFilterTest)

Tests run: 92, Failures: 3, Errors: 4, Skipped: 0

Geert Bevin 2010-03-30

From the description of the issue, this sounds like a good thing. I quickly scanned through the patch and while in general it looks fine, I’m not certain that I like the maintenance of three individual maps for this. One downside is that each time getHeaders() is called, it needs to reconstitute a new collection that aggregates all the headers together. It also seems weird that the ordering within each header TreeMap is maintained, which they’re split out in three. This means that there’s no global ordering at all. Can this be refactored to only use one map?

Eric Dalquist 2010-03-30

Are there docs on how to run the Orion test server for the tests? I get a lot of test failures locally on a clean checkout of ehcache-web.

GenericResponseWrapper.getHeaders is now deprecated and not actually called anywhere in the library. It is still there simply to ensure that the API does not break between patch releases.

I can look at a way to ensure ordering, I’m not sure that is required per the servlet spec though. The three Maps are used to avoid unnecessary casts when determining the type of Header being used. I could probably refactor Header a bit to provide an inexpensive Header type check on the class and then use a single LinkedHashSet. This will increase complexity quite a bit due to the need to compare headers in a case insensitive manner but keep the case of the first header for each name that is set.

Eric Dalquist 2010-03-30

This patch revision merges header tracking in GenericResponseWrapper and PageInfo into a single Map/List. Header ordering across header names is still not defined. Header ordering across types but within a single header name is maintained. From what I can find this meets the requirements laid out by the HTTP specification: http://stackoverflow.com/questions/750330/does-the-order-of-headers-in-an-http-response-ever-matter

I’m guessing that the CachingFilterTest and SimpleCachingHeadersPageCachingFilterTest tests are still in error. I need instructions on how to run the test HTTP server so I can actually run the unit tests.

Eric Dalquist 2010-03-30

Functionally the same as the last patch. Removed an unused class and fixed all of the checkstyle errors.

Fiona OShea 2010-03-30

Does Eric need to sign a contributors agreement? http://www.terracotta.org/confluence/display/devdocs/How+To+Become+A+Contributor

Eric Dalquist 2010-03-31

All unit tests now pass

Eric Dalquist 2010-04-19

Just wondering if there is any progress on this issue? I’d love to use ehcache 2 but can’t until this gets resolved.

gluck 2010-04-19

Unfortunately your contrib caught us a bit late in the release cycle. We manually tested against all the app servers.

We will look at this for the next release. In the meantime why don’t you just apply your patch to source and run with that?

The good news is that Himadri, out testing guy, has created automated testing of all the app servers and applied that. You will see that if you do an update. Using this we can release updates more easily.

I think we have your contrib agreement, right?

Eric Dalquist 2010-04-19

You should have a CLA on file for me.

Unfortunately the use of the library is for another open source project that depends on ehcache-web and we are planning on pushing this next release into the central maven repo so if we did a patched jar we would have to publish it under our own group id to central which wouldn’t be good at all. It looks like we’ll just wait for 2.0.2

gluck 2010-04-29

Last (in date) of the 4 patches applied.