Mailing-List: contact dev-help@cxf.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cxf.apache.org
Content-Type: text/plain; charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 8.2 \(2070.6\))
Subject: Re: [Proposal] WS-Transfer/WS-Fragment implementation
From: Daniel Kulp <dkulp@apache.org>
In-Reply-To: <54E4D611.2000300@gmail.com>
Date: Thu, 19 Feb 2015 14:09:53 -0500
Content-Transfer-Encoding: quoted-printable
Message-Id: <2866E03F-B2D9-4B1D-A6C2-C20A6B605508@apache.org>
References: <1415711832549-5750975.post@n5.nabble.com>
 <1417120280405-5751577.post@n5.nabble.com>
 <1423771411108-5754317.post@n5.nabble.com>
 <C3839D91-9DC6-4ADA-8BB6-943E89FD3917@apache.org>
 <54E4D611.2000300@gmail.com>
To: dev@cxf.apache.org


> On Feb 18, 2015, at 1:12 PM, Erich Duda <dudaerich@gmail.com> wrote:
>=20
> Hi,
> thank you for your comments. I fixed mentioned issues, see comments =
bellow.
>=20
> D=C5=88a 13.02.2015 o 21:23 Daniel Kulp nap=C3=ADsal(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=E2=80=A6 I completely forgot about =
this.
>>=20
>> There are a couple of obvious things that =E2=80=9Cjump out=E2=80=9D =
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=E2=80=99t 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=E2=80=99s there during development.
>=20
> 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 =E2=80=9Cmvn=E2=80=9D on the =
command line is the important one.   Ideally, they=E2=80=99d 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.


>=20
>> 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=E2=80=99d 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=E2=80=99m not =
sure those are even needed.  CXF=E2=80=99s MAPCodec.java already gathers =
the headers that have the ReferenceParameter attribute and adds them to =
the =E2=80=9CTo=E2=80=9D 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:
>>=20
>> =
http://cxf.apache.org/faq.html#FAQ-HowcanIaddsoapheaderstotherequest/respo=
nse?
>>=20
>> and avoid the JAX-WS handler/interceptor entirely.
>=20
> 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=E2=80=99t 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=E2=80=99m 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=E2=80=A6.   Can you double check that the soap =
headers you are expecting to be there are really there?=20


> 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).
>=20
> I removed TransferTools. I use for
> - parsing, serializing XML - StaxUtils
> - creating elements - DOMUtils.createDocument().createElement
> - transforming - XSLTUtils
> - xpath - XPathUtils
>=20
> 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=E2=80=99re=
 likely good to go with it.  =20

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