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:[EMAIL PROTECTED]
Sent: 19 March 2007 16:29
To: [email protected]
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
