cxf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Glynn, Eoghan" <eoghan.gl...@iona.com>
Subject RE: CXF-438 Patch; HTTP(S) Trust Decision
Date Tue, 20 Mar 2007 17:27:27 GMT


Hi Polar,

A few quick points on your patch ...

HTTPConduit ...

1. "//TODO: Revaluate whether we should really be enginnering for the
tests to succeed."

The alternateConnectionFactory is not "engineering for the tests to
succeed", its engineering so that the code is testable in isolation.
This is basic unit testing methodology. Not some trickery to ensure that
tests succeed.

2. "// TODO: Invesitgate whether the name is meaning full outside of
configuration"

I don't think personal reminders like this should be committed to the
code. Unless you intend someone else to do the investigation, in which
case you should open a JIRA instead.

3. " protoId.equals(HTTPS_PROTOCOL_ID)"

Please move the HTTPS check out of HTTPConduit, to the
HttpsURLConnectionFactory.

4. "// TODO: Question, why are we not getting a HttpURLConnection?"

I've already answered this question on this list.
 
5. " // TODO: Is this cast because of the "test" URL connection
factory?"

No the cast has nothing to do with testing. 

And please, enough of the TODOs already :) Seriously though, 25-ish
TODOs in the patch disrupts the readability of the code.

6. Does the configurer.configureBean() call for the MessageTrustDecider
really need to be done within the HTTPConduit code? Could this instead
just be pulled in as a result of the configurer.configureBean() call on
the HTTPConduit itself done within the HTTTransportFactory?


MessageTrustDecider ...

7. Tabs all over the place. This will break PMD/codestyle in the build.

8. MessageTrustDecider should probably be an interface, as it adds
little value as class, but prevents instances from extending some other
class (e.g. "GUIMessageTrustDecider extends Widget implements
MessageTrustDecider" would be impossible).

9. I agree with Dan that explicit establishTrust() params for the
endpointInfo, serviceInfo & serviceInfo is overkill. All these and more
can be accessed from the Exchange. Why not just pass this instead?

/Eoghan

> -----Original Message-----
> From: Polar Humenn [mailto:phumenn@iona.com] 
> Sent: 19 March 2007 16:29
> To: cxf-dev@incubator.apache.org
> Subject: CXF-438 Patch; HTTP(S) Trust Decision
> 
> Greetings,
> 
> I have submitted a patch for JIRA-CXF-438. This patch handles 
> the issue of making a Trust Decision in the 
> rt-transports-http module. It is attached to the JIRA issue. 
> I hope someone will review. Although I have applications that 
> test this, I will organize them into a system test shortly.
> 
> Cheers,
> -Polar
> 

Mime
View raw message