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

[CXF-7491] make Stax & XSLT interceptors Charset-aware #309

Open
wants to merge 5 commits into
base: 3.1.x-fixes
Choose a base branch
from

Conversation

cchepelov
Copy link

per https://issues.apache.org/jira/browse/CXF-7491
this implements "solution 3" described there; forking the Stax & XSLT transform interceptors to make them charset-aware, not blind "always UTF-8" as previously

  • updates the tests to cover for this

@sberyozkin
Copy link
Contributor

Thanks for this effort. Can you consider modifying the existing interceptors to make them charset-aware instead of creating a new set of interceptors ? In reality some users will continue using the old ones, and we will face the problems with syncing the fixes across the 2 sets of interceptors

@cchepelov
Copy link
Author

Thanks for the feedback @sberyozkin

As noted in the JIRA ticket, I hesitated between simply updating the existing interceptors and forking/deprecating. The reason why I went with the latter is in order to avoid making a breaking interface change for users of subclasses of these interceptors, which seemed to be an excessive surprise in a possible 3.1.(x+1) release. In the patch as it is now, the features are updated to use the new interceptors.

Do you feel I overestimated the inconvenience to such subclassers, at the expense of leaving (effectively) dead/worse code around ? I'll gladly switch to the break-but-improve-in-place strategy if this is preferred.

@sberyozkin
Copy link
Contributor

I'm not sure if users actually have ever tried to extend these classes, we can ask at the CXF users for example if someone has tried it...Can you please try to do another PR but based on updating the protected methods, or may be you can find a way to pass that extra parameter as a new message property, example, Message.PAYLOAD_ENCODING, or similar ? thanks

@cchepelov cchepelov force-pushed the cxf-7491_nonutf8-interceptor branch from 56dd9e3 to 31514b5 Compare November 6, 2017 02:18
@cchepelov
Copy link
Author

@sberyozkin many apologies for not coming back earlier

I've updated the PR to switch back to updating the protected methods, which bets hardly anyone extended the classes in question (+ some checkstyle tidying up)

I'm not sure adding a message property would work, since the whole point of the patch is to read the existing property and add an "encoding" arguments to internal methods to pass it on, but I might misread.

@cchepelov cchepelov changed the title [CXF-7491] Fork Stax & XSLT interceptors to make them Charset-aware [CXF-7491] make Stax & XSLT interceptors Charset-aware Nov 6, 2017
@sberyozkin
Copy link
Contributor

Np, sorry for a delay too. I see you check a message contextual property in many places and if it is not set - default to UTF-8 and finally pass it as a method parameter. What I suggested that, instead of updating all the methods which need this encoding property - set it as a new Message property and get the interceptors check the current Message. In fact, why don't interceptor methods which need this encoding property simply check as in
https://github.com/apache/cxf/pull/309/files#diff-7b1f19d5162a1a32027e223ec72228eeR61

may be also have this code encapsulated in MessageUtils ?

@cchepelov cchepelov force-pushed the cxf-7491_nonutf8-interceptor branch from 8e59039 to 58de618 Compare December 1, 2017 16:07
@cchepelov
Copy link
Author

Hi @sberyozkin — good advice. Now the inheritance interface is (almost) untouched. (almost) as there remains a corner case which according to IntelliJ should be private not protected anyway.

All rebased up to current 3.1.x-fixes branch

Thanks for the review!

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.

2 participants