cxf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Polar Humenn <phum...@iona.com>
Subject Re: CXF-438 Patch; HTTP(S) Trust Decision
Date Tue, 20 Mar 2007 18:23:28 GMT
Glynn, Eoghan wrote:
> 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.
>
>   

Well, I don't understand why we have *if* statements in the code just to 
support the testing of the code. But maybe I can consider it 
"instrumentation". However, in any case, this explicit use of it should 
be documented up the wazzzo to prevent the "Huh??? what is *this* used 
for?" that wastes time in discovery.

> 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.
>   

Okay. Fair enough.

> 3. " protoId.equals(HTTPS_PROTOCOL_ID)"
>
> Please move the HTTPS check out of HTTPConduit, to the
> HttpsURLConnectionFactory.
>
>   
Aside from the fact that check won't work if the URL is indeed "https" 
at that point.
Furthermore, this URLConnectionFactory should really be a 
HttpURLConnectionFactory emitting a HttpURLConnection or HttpsURLConnection.

Although, I can see otherwise that if the application can change out the 
ENDPOINT_ADDRESS with something like "ftp://span.com/file1" then we have 
a problem.

I don't know where to cut the cheese on this one? Is it an HTTP Conduit, 
or not?

> 4. "// TODO: Question, why are we not getting a HttpURLConnection?"
>
> I've already answered this question on this list.
>   

I really believe this has to do with the "testing" URLConnection 
factory.  If it doesn't please give me a suitable reason why.

If we are indeed not dealing with HTTP(S) here, then I believe we really 
need to change the name of the Conduit. URLConduit?

> 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.
>
>   
whatever. Okay.

> 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?
>
>   
It's "declarative" right? It's either there to get picked up or not. 
This kind of stuff is all over the place. What does it matter? Do you 
want the TrustDecider in the HTTPConduit Constructor then? I don't care. 
I just did it in the same place the HTTPConduit "configures" itself, 
figuring that it was a logical place to go. Sorry.

> MessageTrustDecider ...
>
> 7. Tabs all over the place. This will break PMD/codestyle in the build.
>
>   
Sorry, that's an Eclipse problem that I thought I had fixed, apparently 
not. My bad.

> 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).
>
>   

Well, I had thought about it, originally I did have it as an interface, 
but then I thought about it some more and decided it an abstract for the 
better. Since it is a "Configurable" "Spring" "Bean" I decided that 
since its class has to be instantiated by Spring anyway it was only 
going to hold as well just carry ONLY the things that we are supposed to 
get for it. I think being an abstract class with a default logical name 
implementation is a perfect way to go.

> 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?
>
>   
Yeah, I agree, understanding more of the message oriented nature instead 
of the RPC oriented architecture moving there really isn't any 
justification to go that far. I'll just give them the Message, which has 
got everything it may care to use.

Cheers,
-Polar
> /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