sshaw / ddex

DDEX metadata serialization for Ruby
https://metadatagui.com
53 stars 38 forks source link

strip leading slash #2

Closed germs12 closed 10 years ago

germs12 commented 10 years ago

While working with SONY Music DDEX files I encountered an error where a leading slash ("/") was causing the version validation to fail. Remove any leading slashes when checking version.

MessageSchemaVersionId="/ern/341"

vs

MessageSchemaVersionId="ern/341"
sshaw commented 10 years ago

Hummmm yes, I was worried about this when using MessageSchemaVersionId to determine the version. The DDEX schemas define this element as a string, so who's to say some form of spec + version will be always be used?

Have you seen many/any DDEX files with a MessageSchemaVersionId attribute that differs from the spec + version format? Maybe I'll just strip any non-word chars? E.g., ver.downcase.tr "^[a-z0-9]", ""

I think I need to make this configurable globally or add this as an option. Thoughts?

germs12 commented 10 years ago

I just got off the phone with Sony and didn't have any comment when I brought up the leading slash. I would guess you could simply remove it when you look at the version and not make it an option at all. They were excited to hear about you and your library and plan on spreading the word of its existence within their company. Congrats!

I'm not sure why the tests are failing, but if there is something I can do to make it work please let me know. Thanks for your hard work, I will continue to use the library and make it more usable when ever possible. Thanks!

sshaw commented 10 years ago

Thanks for your support. I'm glad to have someone using this. I haven't yet used it in production yet so it's great to get some feedback. I'm curious what -if any- high level methods can be added to the classes to simplify processing; update? for example.

I've raised the MessageSchemaVersionId issue on the DDEX forums. We'll see what they have to say but I think some form of normalization/customization is the way to go regardless.

If you're unlucky enough (like me) to have to deal with XML then I would suggest checking out the library that I used to generate most of this, jaxb2ruby. It's not released yet, but seems to be working for the most common cases.

Not sure why the specs are failing, I'll have a look this weekend, but one problem I see is that if the attribute is not set you'd be calling gsub on nil. Nevertheless don't worry, as I said before , some broad form of normalization is needed.

sshaw commented 10 years ago

My question in the DDEX forum goes unanswered...

I have been thinking about/working on this and wondering if customization of the MessageSchemaVersionId is a sufficient solution. For example, you can now say:

DDEX.read(doc, :version => "ern/34")
# or
DDEX::ERN.config["V34"][:message_schema_version_id] = "/ern/34"
DDEX.read(doc)

Of course this isn't good if you don't know the version ahead of time or if 1 out of N files have the extra slash. My only concern about normalization is that if one does want a custom version id like "/ern/34" and says so via config above the code would normalize it away, resulting in another UnknownVersionError.

Thoughts?

germs12 commented 10 years ago

I think the document should be the sole definer of the version and you should loosen the check by stripping leading and tailing slashes. There is no technical difference between /ern/34 and ern/34, so why make it something that trips up users of the library?

If there was a difference between the two, then it should be something the user can modify, but since it's just an extra slash, it seems meaningless and adding configuration only increases complexity. I tend to think things should be as simple as possible. Allowing for a manual override adds a lot more code then a single gsub to replace the leading slash.

My 2 cents.