HttpClient parameterization

[Brent Putman]

Realized I hadn't responded yet.  At the moment, I'll try and be pithy
and just hit the highlights, rather going into all the details.


On 2/8/17 10:01 AM, Rod Widdowson wrote:
> It turns out that the code structure is more complex than I remembered which may make this suggestion just too Byzantine (I was
> already worried that it might be), but I'll write this up anyway.

I don't think the general idea is really all that Byzantine.  I think
this is (or close to) a common formal software engineering design
pattern (whose names I always forget and/or confuse).


> If a particular usage pattern (for instance the DynamicMetadataProvider) needs a different parameterization then we derive a new
> class from the base DefaultParameterClass and change just those values that we care about.

This is really the main issue that jumps out for me.  As the main
example here: The dynamic provider needs "non-default defaults" and so
you make a separate class that extends the based "default defaults"
params class.  That's fine.  Since the usage though is really specific
to the internal HttpClient produced for the dynamic metadata case
(inside the parser), the most logical place to put that extension class
is as an inner class of the parser...at least to me.  So that kinda
just moves the defaults from static constant field values directly on
the parser to values in an inner class of the parser.  (I guess could
be a separate top-level class, but I'd still naturally want to put it
in the same package as the parsers.)  Either way, not horrible ... but
playing devil's advocate: how much does that really buy us?

Unless you think the dynamic metadata defaults would be elsewhere, but
I'm not seeing another logical place.


> The complication comes in that there are similar class hierarchies for InMemoryCaching clients and for FileSystemCaching Trees (they
> fork in java-support).

This is the other issue that jumps out for me.  You really need (based
on the current variations) 3 "default defaults" base classes, with the
2 caching ones derived from the non-caching one.  Then for say the
dynamic provider, you actually need 3 "non-default defaults"
extensions, and you set the correct one based on the type of caching
specified.  It probably works, but does start to get a little messy.


I may have other comments later, but those were the 2 main things.  So
I'm not totally against it. Just trying to weigh the additional
complexity against the benefits.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://shibboleth.net/pipermail/dev/attachments/20170217/296355c6/attachment.html>