Reverse attribute mapping..

[Cantor, Scott]

On 9/24/13 2:50 PM, "Tom Zeller" <tzeller at dragonacea.biz> wrote:

>I must be missing something. From what I can tell, the proposal is to
>rename net.shibboleth.idp.Attribute to net.shibboleth.idp.IdPAttribute
>so that org.opensaml.saml.saml2.core.Attribute does not have to be
>fully qualified. Am I correct ?

That's a benefit, but isn't the main justification.

>Again, y'all are too smart for me to be saying this, so I must be
>dense, but there are multiple kinds of sessions in the IdP, so
>IdPSession and SPSession or ServiceSession further qualify the kind of
>session.

That's kind of debateable. There's one kind of session that happens to
contain something used to track sessions with the SPs. Using Session and
ServiceSession would not have bothered me that much, but I probably would
have used Session and ServiceRecord or something like that. But if they
both are called sessions, I didn't think using IdPSession was a bad idea,
as I don't think IdPAttribute is a bad idea.

I definitely believe very strongly that calling the resolver's product
"Attribute" is a bad idea. The code is just many times harder to follow
than the actual logic itself is if the name were different.

> There are only a handful of classes in the v3 IdP which start
>with "IdP", IdPSession and IdpInitiatedSsoRequest*, so I do not
>understand what you mean by consistent.

Consistent in that classes referring to things that appear multiple times
in somewhat different senses ought to have separate names, not just
separate packages.

I would note that I screwed this up in the SP too, and have regretted it
ever since.

>I think that changes to the IdPv3 APIs, even as unreleased, should get
>a new email thread or JIRA issue and time for me to think about it.

I think we have to have a little bit of gradient because there are
sections of the code that are literally meaningless at this stage, nothing
but half-written ideas. The session code for example was totally wrong to
start with, and it's no more correct now, it's just a work in progress.

>If the reason for the refactoring is solely improved readability of
>the attribute mapper, then I suggest that the refactoring is
>inappropriate for the perceived benefit, which is subjective.

It's not just for the mapper, but for the whole code base where it
appears. I don't see why readability isn't a sufficient justification for
changing the API now before we release anything and can't change it.

I'm fine leaving it as an open issue to take up later, but part of the
reason to do it sooner is so that code review is easier now.

In my opinion, it's just plain bad, both in the SP and IdP. Only the
latter is fixable at the moment.

-- Scott