Qs after reviewing Java dynamic metadata resolvers

[Brent Putman]

On 8/31/16 1:44 PM, Cantor, Scott wrote:
> I've filed a couple of bugs after reviewing the code,

I'll look at the OSJ-171 and OSJ-172 ones and respond in the issues. 
Those two seem pretty minor.


> I'm guessing maybe this is already known, but it seems like this design doesn't accomodate artifact resolution if the transformation of the URI is configured into the resolver "once" at wiring time, and then only works against an EntityIDCriterion.
>
> There are a number of ways it could be enhanced I suppose, but I also found it a little strange that the Function interface it's using is String->String vs. CriteriaSet->String. The latter would accomodate an ArtifactSourceIDCriterion for example, and then a Function chain could be used to look for supported Criterion and handle the different cases. As it is, I'm not sure how it would work.

Yeah, I think I didn't have the artifact use case in mind when I
originally did this work.  Was that always in the MDQ spec - as in
specifically mentioned - or was it added later?  I probably just
overlooked it.

In any case, I agree the current impl is not correct for that use case. 
However, I think it's trivial to fix.  The
FunctionDrivenDynamicHTTPMetadataResolver is the (trivial) concrete
class here, and all it basically does is provide an impl of the
superclass abstract method:

String buildRequestURL(CriteriaSet criteria)

So the input there already is a CriteriaSet.  The current String->String
function is just an impl detail.  So it would be trivial to fix things
for this case:  Just add a new Function<CriteriaSet, String> property
and if configured use that in preference to the existing String->String
one.  Or something like that.  Details TBD.  Maybe deprecate the
String->String one.

And/or of course we could add a new similarly trivial concrete subclass
that has only a CriteriaSet->String function.

Fixing up the Spring custom parsing to use either of those approaches in
the IdP doesn't seem that bad, it's all "under the covers" kind of stuff.

>
> Mostly I'm reviewing the code to see where the best place to add file-backed support would be, because we need to get that into 3.3.


Read on, but I think it's AbstractDynamicMetadataResolver (NOT the
HTTP-specific subclass).


>
> Does the HttpClient support file URIs by any random chance? Probably not I'm guessing.

Not that I'm aware.

>
> I guess there are a few ways to do this:
>
> - extend AbstractDynamic[HTTP]MetadataResolver to support failing into backup files

That's basically what I had in my mind to do.   The code to save and
load metadata to/from a file is not complicated.


> - create a new resolver that's strictly file-based (largely duplicating some of the existing class hierarchy underneath AbstractDynamicHTTPMetadataResolver) and force the deployer to chain to that as a backup

Well, as far as providing a totally file-based dynamic resolver (the
folder o' metadata idea), my plan was for a relatively trivial concrete
subclass of AbstractDynamicMetadataResolver.  Note that this class is
where all of the "dynamic" bits live.  That was deliberate, making it
easy to create new dynamic resolvers that are not tied to HTTP.  The
AbstractDynamicHTTPMetadataResolver  just has the HTTP transport bits only.

As we've previously discussed, I think the only trick for the file-based
one is devising the appropriate strategy for turning entityIDs into
usable file names.  And providing a end-user tool to let them
convert/rename an EntityDescriptor using the same strategy.  Or
something fancier, like an ingest queue processed by a background (drop
in the file, it gets processed and moved to the runtime metadata dir
with the correct name).

> - pull the transport aspect of this out of the class hierarchy and deprecate what's there now

Offhand, at the moment, I don't personally see a need to do this, given
the relative ease of adding backing file support to
AbstractDynamicMetadataResolver, which then covers all the dynamic
resolver cases.

But if there's a good reason, or something I'm missing, of course let's
discuss.

>
> I favor the latter. Basically, I think instead of baking HTTP-awareness into a metadata resolver,

To re-iterate: The HTTP awareness is *not* in the dynamic resolver
code.  We already have a transport/source-agnostic dynamic resolver base
class, the AbstractDynamicMetadataResolver.  That was deliberate.



>  we should plug in a new interface that handles metadata acquisition, and that interface can take over the HTTP client case, the existing or extended entityID -> URL transformation job, and include additional support for backing up to files, and simply reading from files.

You may remember that early on in v3 Chad had proposed something that
sounds very much like that - a 2-part metadata resolver that separated
the "source" of the metadata from the "resolving" from it.  And in the
end we rejected it because there was something about it that didn't
work.  I'd have to go look at my email archive to swap in exactly what
it was, but I think in fact it was you who originally pointed out the
problem(s).  Maybe that's not the same thing as you are proposing here,
though.

But that in general sounds like a much more complicated design. 
Reading/writing files isn't that hard, and I think (albeit not having
gotten into it yet) that that is easily done in
AbstractDynamicMetadataResolver.  If your reasoning is that you want to
factor out the file processing code so it can be reused for the separate
use cases of the backup files *and* the true file-based dynamic resolver
- I think we could achieve the same thing via either support classes, or
even some sort of file load/save interface+impl component that is
plugged into both the AbstractDynamicMetadataResolver for the backing
file use, and the concrete file-based dynamic resolver.  The latter
component approach is kind of what I had been thinking, although I
hadn't gotten to anything concrete yet.



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://shibboleth.net/pipermail/dev/attachments/20160901/ec24f51e/attachment.html>