> -----Original Message-----
> From: Polar Humenn [mailto:[EMAIL PROTECTED]
> Sent: 20 March 2007 18:23
> To: [email protected]
> Subject: Re: CXF-438 Patch; HTTP(S) Trust Decision
>
> > 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.
The if statement is needed because the mocked up URLConnectionFactory is
specified as a ctor parameter, whereas the retrieval of the real
URLConnectionFactory (which we need to suppress in the test scenario)
must be deferred until after the config has been retrieved
post-construction. At one point, both of these tasks were accomplished
in the ctor. It was changed to the current mechanism to accommodate an
evolution of the config infrastructure. No rocket science involved :)
I don't have time for a big long discussion on testing methodology. But
here's a thought ... some unit tests must be better than none, right?
> 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.
Hmmm, here's a few thoughts on documenting "up the wazzzo", presumably
via in-code commentary.
Code is validated by tests. Comments are NOT.
When code is broken by a change somewhere, the tests fail and force us
fix the breakage.
However we get no warning on "comment rot", i.e where comments fall out
of date with respect to evolving code.
Similarly if the commentary is ambiguous, open to misinterpretation or
just plain wrong in the first place. On the other hand there can be no
ambiguity in the code.
I don't have time to get into a huge discussion about this, but briefly
consider the following ideas:
- Comment-rot is much worse than no comments at all.
- Unless the code is *really, really* complex, the intent is often
clearer to a competent programmer from the code itself, as opposed to
someone else's commentary.
Note that I'm NOT of the school that would ban comments altogether from
the code. Comments are a Good Thing *if* used judiciously. As a rule of
thumb, a warning light should go off if there are more lines of
commentary than executable code.
Now in the case in point, this discussion originated from a question you
asked last week ... why do we have a unused ctor param in HTTPConduit?
But this param was not in fact unused, as could easily have been seen
via the Eclipse 'open call hierarchy' for that ctor. Now I'd argue that
the overhead of viewing the call hierarchy and reading a comment is
about the same, but the former approach has the advantage that its
guaranteed to be a correct reflection of reality, whereas a comment may
be incorrect/ambiguous/out-of-date.
And lets not get in a rant about whether we should force anyone reading
the code to use Eclipse. I was a emacs-Luddite for long enough myself :)
So I don't care what IDE folks use. Even if their idea of an IDE is vi,
there they could still get the call hierarchy with a bit of:
find trunk -name "*.java" | xargs grep "new HTTPConduit(" ...
The point being that the code itself should be the definitive source of
all knowledge.
Just some ideas to consider, and possibly ignore if you disagree. Either
way, lets not get into another time-burning discussion on this.
> > 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.
I don't get your point.
If the SSLClientPolicy is configured, then the connection factory would
be a Http*s*URLConnection. You already check in the
Http*s*URLConnection.createConnection() whether the scheme is "https":
public URLConnection createConnection(Proxy proxy, URL url)
throws IOException {
+
+ if (!url.getProtocol().equals(HTTPS_URL_PROTOCOL_ID)) {
+ throw new IOException("Illegal Protocol "
+ + url.getProtocol()
+ + " for HTTPS URLConnection Factory.");
+ }
You also check this in the implementation of URLConnectionFactory
responsible for creating plain-text HttpURLConnections:
+ public URLConnection createConnection(Proxy proxy, URL url)
+ throws IOException {
+
+ if (!url.getProtocol().equals(HTTP_URL_PROTOCOL_ID)) {
+ throw new IOException("Illegal Protocol "
+ + url.getProtocol()
+ + " for HTTP URLConnection Factory.");
+ }
So why do you need to also do this check upfront in the
HTTPConduit.send() just before the URLConnection.createConnection() is
called? The effect is to needlessly introduce HTTPS logic into the HTTP
code for *no* benefit (as this check is done anyway in the relevant
URLConnectionFactory).
Also, is it deliberate that if the SSLClientPolicy is configured, but a
"http://"; URL is provided, this would no longer work? I think this would
work currently, and support the "semi-secure" client idea, i.e. use the
configured trust/keystores/ciphers etc. if the server is secure,
otherwise use plain-text HTTP. BTW I'm not sure that makes any sense,
but I know some other products do support this style of usage.
> Furthermore, this URLConnectionFactory should really be a
> HttpURLConnectionFactory emitting a HttpURLConnection or
> HttpsURLConnection.
Do you mean it should be called "HttpURLConnectionFactory"?
But its an inner class of HTTPTransportFactory that implements the
URLConnectionInterface for plain-text HttpURLConnections. Inner classes
are *anonymous* in Java. The following code:
new URLConnectionFactory() {
};
does NOT create a class called URLConnectionFactory. It creates an
*anonymous* class that implements the URLConnectionFactory interface.
And we already have a *separate* Http*s*URLConnectionFactory for
creating Http*s*URLConnection instances. This separation was the
motivation for introducing the URLConnectionFactory interface in the
first place.
> 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?
See the explanation as to the generic URLConnection usage that I posted
to the list in response to you question last week.
> > 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.
The intent is NOT to test the URLConnectionFactory! The idea is to
remove the real URLConnectionFactory from the mix, so that the rest of
the HTTPConduit code can be tested in isolation. This is called the mock
object pattern. It's basic tenet of unit testing.
> > 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?
It could matter for testing. How do you intend to unit test this logic?
It would probably be easier to simply mock up the return from a call to
something like:
EndpointInfo.getTraversedExtensor(new MessageTrustDeciderPolicy(),
MessageTrustDeciderPolicy.class);
Rather than mocking up a Bus instance, Configurer instance, and register
the latter as an extension of the former.
> Do you want the TrustDecider in the HTTPConduit
> Constructor then? I don't care.
Why not just pick it up in the same way as the other configuration
policies are picked up?
On a completely separate point, shouldn't MessageTrustDeciderPolicy (or
whatever you call it) be specified in the security schema, and just
JAXB-generated like the AuthorizationPolicy etc.?
> > 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.
Remember that Java is restricted to single inheritance of
implementation. So each class can only extend *one* other, but can
implement *any* number of interfaces. So a common rule of thumb is not
to waste the one shot at implementation inheritance just in order to
pick up something trivial.
/Eoghan
