Harmonize etree and encoding/xml

[fatlotus]

Following up on Andrew's comment on #10. Russell, stylistic preferences between etree and encoding/xml? (I too like keeping with the stdlib, but it's not my library.)

Anyway, if you do have an opinion one way or another, I'd be happy to start poking at things.

[russellhaering]

I'm flexible. I started with etree because it seemed like the best way to make XML signatures work, but encoding/xml does seem easier for Unmarshaling in general.

As long as we try to be consistent in our approach I'm ok with encoding/xml.

On Thu, Jul 7, 2016, 9:49 AM Jeremy Archer notifications@github.com wrote:

Following up on Andrew's comment on #10 https://github.com/russellhaering/gosaml2/pull/10. Russell, stylistic preferences between etree and encoding/xml? (I too like keeping with the stdlib, but it's not my library.)

Anyway, if you do have an opinion one way or another, I'd be happy to start poking at things.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/russellhaering/gosaml2/issues/11, or mute the thread https://github.com/notifications/unsubscribe/AAE-JUgEgB1NchSh6IDWLZXzmC36Fvkcks5qTS4OgaJpZM4JHSBk .

[andrewstuart]

So. This brings up a question that's been working its way into my mind. Namely, how closely should we adhere to the serialization format, versus trying to make the Go API cleaner (at the cost of serialization/deserialization complexity)?

A good example of where I'm wrestling with this is in this branch for metadata reading. As you can see from the test XML and from the Go Types I've started writing, there's quite a bit of cruft (as I would describe it) in the XML structure.

For example, <EntitiesDescriptor> contains many <EntityDescriptor> elements, and <IDPSSODescriptor> vs. <SPSSODescriptor> wrapper elements which have lots of repetition. Clearly this is an API built for extension-heavy languages.

What I've tentatively gone toward is only one []Entity type of length 1..N (you can have a bare EntityDescriptor in your metadata, but this obviously limits you to one).

Then with the IDP vs SP descriptor distinction, since type extension is not a Go thing, I've currently just used a well-known (and exported const) string identifier. To capture the subtype-specific configuration, I've been mentally leaning towards a Entity.TypeConfig interface{} struct field that can be typecast depending on the value of Entity.Type. This allows us to capture the difference minimally at least.

I think this also applies well to the conversation about marshaling strategies, since it would probably have implications for how we'd go about implementing a common strategy. This gets more true the more complex/polymorphic the XML structures are -- thus far they've been straightforward for anything I've built using stdlib unmarshaling.

So, before I go too far down this road, any thoughts?

[fatlotus]

Two questions from your question:

What do you mean by type extension? Do you mean something like inherence?

In that branch, could the duplicated fields be implemented by anonymous struct embedding? (So instead of a single []Entity, multiple IDP/SP-specific slices.)

[russellhaering]

I think we should favor simple interfaces, with the goal that it should be hard to use the library incorrectly. My only concern is making sure that in the future we can add more flexibility where reasonable, without breaking the existing API.

Maybe it would make sense to have a lower-level interface that is closely aligned with the XML structure, and wrap it in a higher level interface intended to serve most common use cases?

[andrewstuart]

@fatlotus, by "type extension," I do mean inheritance. There are tons of references in the spec to "BaseSomething" and "AbstractType" etc. And I'm rather glad that Go doesn't directly support that. It does make for some interesting creative challenges though, trying to design the API.

As far as struct embedding, I have at least experimented (if it works well, perhaps it will stick) with a few types that share the same fields via embedding.

I'm rather on the fence between []Entity (for simplicity) and a couple stronger IdP/SP types (to benefit from type checking). I'd imagine that ultimately, given the complexity of the spec, we'd probably benefit from having separate types so we can work with the type system and keep things a tad simpler (e.g. no unnecessary nil fields that would be non-nil for a sibling type).

@russellhaering I definitely like the idea of keeping the data structured very closely to the original XML, and providing methods that simplify the common use cases. I'll try to make sure I aim for that with the metadata parsing (and hopefully some SP metadata generation too in short order).

I'll do a little refactoring and see if it cleans things up. I'm guessing it will at least clean up the Unmarshal code and make it a bit more declarative if I can separate out the IdP and SP types.