Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serialization: Boolean TypeConverter doesn't conform to XML standard #54

Open
majoreffort opened this issue Dec 10, 2019 · 3 comments
Open
Assignees
Labels

Comments

@majoreffort
Copy link

When serializing a model using NMF, boolean values are serialized upper case "True"/"False". As a result, deserialization using other tools/libraries fails. One prominent example would be the default EMF Deserializer.

According to the XML schema specification, only "true", "false" or "1" and "0" are legal values (see https://w3.org/TR/xmlschema-2/#boolean). I don't know if this is an issue for other datatypes as well.
I think NMF should adhere to the specification, at least when serializing.

The .NET framework already provides a static class XmlConvert with several useful functions (XmlConvert.ToString with multiple overloads for serialization, XmlConvert.ToBoolean, XmlConvert.ToInt32 etc. for deserialization) which could be used. Maybe we could wrap these XmlConverts in a TypeConverter and use them instead of the System.ComponentModel ones.

I would be willing to provide a pull request once a decision is made.

@georghinkel
Copy link
Contributor

Yes, that would definitely make sense. After all, we already have a custom type converter for DateTime in order to get that serialized in the ISO-Format (also for compatibility with EMF), we could do the very same thing with booleans as well or even wrap all the conversion code into a single converter.

I do not have a strong opinion regarding whether to bundle the conversion in a single TypeConverter or use different ones, I guess both approaches have their own Advantages and disadvantages.

Regarding the decision whether "true/false" or "1/0", I would say that following whatever XmlConvert class does is good, I guess they will use "true/false".

@majoreffort
Copy link
Author

I tend towards different TypeConverters to avoid constant runtime checks of the type. We only ever instanciate them once anyways.

Regarding deserialization I propose we don't use XmlConvert, as it would break every existing NMF XML/XMI (being very strict with which values are legal). I guess the "normal" Convert would do just fine, it basically delegates to [type].Parse, which seems to be able to parse basically everything.

Are there any tests covering serialization specifically?

@georghinkel
Copy link
Contributor

Yes, runtime type checks are the strong reason in favor of having separate type converters. The reason against would be reusability but probably this is a quite weak argument given that you mostly know which types you are using.

There are some tests that serialize and deserialize the railway model from the TTC 2015 Railway benchmark. However, the serialization is probably the least tested part of NMF at all, given that it is also the oldest. It bases on code I wrote when I was in my second year of studying computer science and has been refactored multiple times since.

@georghinkel georghinkel self-assigned this Jan 2, 2020
@georghinkel georghinkel added the bug label Jan 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants