Skip to content
This repository has been archived by the owner on Jan 27, 2021. It is now read-only.

GZIPContentEncodingFilter: Object header values #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

GZIPContentEncodingFilter: Object header values #16

wants to merge 1 commit into from

Conversation

mfulton26
Copy link

Response.header(String, Object) supports non-String header values so
GZIPContentEncodingFilter should too. Otherwise Response builders may
get a ClassCastException when attempting to use non-String response
header values.

@landonvg
Copy link

landonvg commented Apr 3, 2015

Should this change have a unit test?

@mfulton26
Copy link
Author

Probably but there are no existing unit tests for the GZIPContentEncodingFilter but that doesn't mean I can't add some. I will investigate.

@landonvg
Copy link

landonvg commented Apr 3, 2015

looks like there is the same issue on line 95; there you'll need to convert Object to String

@mfulton26
Copy link
Author

No casting takes place on line 95. ContainerRequest.getRequestHeaders()'s return type is MultivaluedMap<String, String>.

@landonvg
Copy link

landonvg commented Apr 3, 2015

Oh, right--that's the request header, not the response header.

@mfulton26
Copy link
Author

@mgajdos Perhaps you can answer a question for me. What are my options if I want to add a test class for this container filter but want to use a mocking framework such as Mockito? Can I pull that in as a Maven test-scope dependency?

@mfulton26
Copy link
Author

Unit test amended (failed with ClassCastException without the change). I didn't end up needing to use a mocking framework so no additional test dependencies have been added which makes this simpler.

Response.header(String, Object) supports non-String header values so
GZIPContentEncodingFilter should too. Otherwise Response builders may
get a ClassCastException when attempting to use non-String response
header values.
@mgajdos mgajdos added the OCA label Apr 20, 2015
@mfulton26
Copy link
Author

@mgajdos, what does the OCA label mean? Is it good or bad? i.e. Is there anything blocking this being reviewed and potentially merged? Thanks.

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

Successfully merging this pull request may close these issues.

3 participants