cxf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Kulp <dk...@apache.org>
Subject Re: [Proposal] WS-Transfer/WS-Fragment implementation
Date Thu, 19 Feb 2015 19:09:53 GMT

> On Feb 18, 2015, at 1:12 PM, Erich Duda <dudaerich@gmail.com> wrote:
> 
> Hi,
> thank you for your comments. I fixed mentioned issues, see comments bellow.
> 
> Dňa 13.02.2015 o 21:23 Daniel Kulp napísal(a):
>>> On Feb 12, 2015, at 3:03 PM, dudae<dudaerich@gmail.com>  wrote:
>>> Do you check the implementation? Could you give me some feedback please?
>>> Thank you.
>> Just took a quick look at this… I completely forgot about this.
>> 
>> There are a couple of obvious things that “jump out” that are very quick/easy
to fix.  Mostly removing the @author tags (highly discouraged at Apache) and there are a few
files that need the Apache header added (the XML files).    There are also several warnings
when pulled into Eclipse, but those aren’t big deals at all.    The other issue would be
the use of the System.out for all the messages during the tests causing a lot of output on
the console.   Again, easy fix and I understand why it’s there during development.
> 
> Should I fixed all PMD warnings? For example, "A method should have only one exit point,
and that should be the last statement in the method". I think that more exit points (return
statements) do some methods more readable.

No.  The PMD that is run when you type “mvn” on the command line is the important one.
  Ideally, they’d both be the same but the current eclipse plugin has a bunch of issues.
 I have a few pull requests open with the PMD folks that should get the eclipse pmd plugin
to be more usable for CXF.


> 
>> The main thing that jumps out at me as being problematic is the use of the JAX-WS
handlers (org.apache.cxf.ws.transfer.shared.handlers).   Those cause problems as we have to
completely break streaming to build the SOAPMessage that is passed into those.   I’d very
strongly encourage flipping those to normal CXF  SOAP interceptors.   They seem to only operate
on SOAP headers so they can call the getHeaders method on the SOAPMessage passed into the
interceptor and pretty much allow the body to remain as is.  (likely streaming)   That said,
I’m not sure those are even needed.  CXF’s MAPCodec.java already gathers the headers that
have the ReferenceParameter attribute and adds them to the “To” EndpointReference.   You
may need to experiment a bit more to see if they are really getting through (and if not, figure
out why).    Likewise, on the client, you could directly add them to the client RequestContext
via the normal way of adding headers:
>> 
>> http://cxf.apache.org/faq.html#FAQ-HowcanIaddsoapheaderstotherequest/response?
>> 
>> and avoid the JAX-WS handler/interceptor entirely.
> 
> The ReferenceParameter is possible to send through the request context. When I tried
to send it through the standard headers, it isn't getting through because MAPCodec removes
all WS-Addressing headers (see discardMAPs method).

The discardMAPs method only discards headers that are in the WS-Addressing namespace.  It
shouldn’t discard the elements that are not in the ws-addressing namespace.   If I add a
factory.getFeatures().add(new LoggingFeature()) to your code (or use wireshark), I’m not
seeing the reference params being written out at all with the new code.   That kind of bothers
me as I believe they should.   Hmm….   Can you double check that the soap headers you are
expecting to be there are really there? 


> Few more CXF things:
>> In XLSTResourceTransformer and TransferTools and a few other places, you are creating
DocumentBuilderFactory/DocumentBuilder and a Transformer and such.   You really should use
the utility methods we have in StaxUtils, XMLUtils, and XSLTUtils for much of that.    We
try to make sure all the XML parsing and processing goes through those utilities so that we
can control various aspects to prevent various attacks (like entity expansion attacks).
> 
> I removed TransferTools. I use for
> - parsing, serializing XML - StaxUtils
> - creating elements - DOMUtils.createDocument().createElement
> - transforming - XSLTUtils
> - xpath - XPathUtils
> 
> However in FragmentDialectLanguageXPath10 I don't use the XPathUtils, because I need
to recognize, when the XPath throw the exception and when not. See https://www.java.net/node/681793

OK.   That works.   Looks much better.

I think if we just double check the reference params things, we’re likely good to go with
it.   

-- 
Daniel Kulp
dkulp@apache.org - http://dankulp.com/blog
Talend Community Coder - http://coders.talend.com


Mime
View raw message