CX/Papryus allows invalid characters in names causing weird downstream effects.

[emammoser]

Issue and tracking information

Developer's time Estimated effort to fix (hours):

Developer's Actual time spent on fix (hours)

Issue reporter to provide a detailed description of the issue in the space below

Original Problem

I created a new message definition and then added pub/sub ports for it on two components. I inadvertently typed a space character at the beginning of the port name.

CX took that as another character in the port name and happily output it in the generated output.

In the component's idl file, it just resulted in another space between the port type and name which is not obvious and does not affect the compiled code. In the component's taginfo file, it also puts the space into the generated class name for the facet class representing that port.

Note: in this case it happened to be a space. It could also have been some other illegal character as well, so any solution needs to take that into account as well.

Suggested Resolution

Use a whitelist of allowed characters for any input that is an identifier, or a type name.

Typically, these can only be:

Upper and lowercase letters. numbers underscores. Important:

There are inputs where other characters are valid and need to be allowed.  Some examples (I'm sure there are others):

Anything that is a value of something. Some examples: attribute values for components, connectors, containers, nodes in a deployment plan constant values. Don't touch the connector default value bindings. They would all be values. Basically: The name of something should use the white list. The value of something should not use the white list.

[eposse]

I've been able to reproduce it. What's more, if you put a space inside the name of the port (or other entities, e.g. "test DataPub"), this propagates to the IDL as well, resulting in illegal IDL syntax. Interestingly, the descriptors generator throws an error, but then completes the generation apparently fixing the name (or just avoiding regeneration).

I think we'll have to do a meta-model update to incorporate the required validation.

A question: should we implement model migration for older models that replaces illegal names by legal ones (e.g. replace non-allowed characters with white-listed characters)?

And a second question: the white list can be fixed (hard-coded) or do you want to be able to specify it? If so, should it be a workspace preference, or something specified in each model?

[emammoser]

No, I do not think we want to have any model migration step for this. As long as the new validation catches these issues user's can fix the issues themselves when they fail validation.

[eposse]

I've added the required validation to streams/v2. Would you like to test it? The build is available in this link.

[emammoser]

I downloaded the test build and verified it on my end. Can we also have this validation work applied to the v3 stream? Thanks

[eposse]

The build for v3 is here.

I've tested it and it works as well. Do you want to test it or should I merge the PRs and close the issue?

[emammoser]

You can merge it in since I tested v2 and am happy with the results.

[eposse]

Will do.

[emammoser]

We have found two issues with this change:

1. When using our BasicPubSub example, the naming constraint gets disabled because of a null pointer exception:

   java.lang.NullPointerException at com.zeligsoft.domain.dds4ccm.constraints.java.ValidNameConstraint.validate(ValidNameConstraint.java:56) at com.zeligsoft.base.validation.provider.java.JavaConstraint.validate(JavaConstraint.java:65) at org.eclipse.emf.validation.internal.service.AbstractValidator.evaluateConstraints(AbstractValidator.java:241) at org.eclipse.emf.validation.internal.service.BatchValidator.validate(BatchValidator.java:264) at org.eclipse.emf.validation.internal.service.BatchValidator.validate(BatchValidator.java:211) at org.eclipse.emf.validation.internal.service.BatchValidator.doValidate(BatchValidator.java:149) at org.eclipse.emf.validation.internal.service.AbstractValidator.validate(AbstractValidator.java:147) at org.eclipse.emf.validation.internal.service.AbstractValidator.validate(AbstractValidator.java:126)

2. We have model elements with periods "." in their name like the "nl.remedy.it.DnCX11.RegisterNaming" element under our components which would fail this when the naming constraint doesn't get disabled.

[eposse]

I see. I will add periods to the set of valid name characters. It's strange that this would result in an NPE though. I'll see if I can reproduce it.

[eposse]

I found out the case where the NPE arises: when you delete the element's name in the UML tab of the Properties view. Note that this is not the case in the CX tab of the Properties view which just sets the name to the empty string.

I will consider that case in the validation rule as well.

[eposse]

A question: can a name begin with a period?

[emammoser]

I think model elements can begin with a period (and use periods). But values that generate to IDL and taginfo.xml files shouldn't have periods anywhere. So any component names, attribute names, data type/message names.

[eposse]

I'm not sure if we are on the same page here, because, for some elements, like ports, attributes and components, their names are used in the IDL as identifiers, and IDL identifiers cannot include periods (according to the latest specs), so allowing periods there would yield invalid IDL.

Similarly, this results in periods in the taginfo.xml files, in the 'name' and '*_name' fields. I don't know if this is allowed, but I imagine that probably not.

Also, allowing periods in, say port names, results in those names used in generated CDPs as well. I don't know if that should be allowed or not, but as before, I imagine that probably shouldn't be allowed.

We have model elements with periods "." in their name like the "nl.remedy.it.DnCX11.RegisterNaming" element under our components which would fail this when the naming constraint doesn't get disabled.

I've seen this name in a Property, although not a CX Attribute. You've mentioned that the elements whose name should not have periods are components, attributes, types, messages. I imagine ports as well, and interfaces, correct? Any others? Should we impose some constraints on other elements (like the Property above)? Keep in mind that models can contain allot of "Named Elements" whose name is null or empty, such as connectors.

[emammoser]

Its hard to say because I do not know the exact syntax allowed in all cases. What I am trying to say is that if the element eventually generates to IDL, the periods should not be allowed. This includes attributes, ports, and components.

As far as elements that only generate to taginfo.xml files, I am not sure if periods would cause issues, but the only thing we can do is not expressly forbid periods for these elements and see if anything goes wrong.

CDP files are not a concern in my opinion as CDP files do not seem to have issues with periods. So elements that only generate information in CDP files do not need this restriction as far as I know.

[eposse]

Ok, I'll go through the IDL generator and check what are all the cases.

[eposse]

Ok, I think I have all cases covered now. Here's a link for the v2 build: Fix for Issue #477.

Let me know if that works for you. In the meantime, I'll start porting it to v3.

[emammoser]

I have verified this build on our BPS example as well as a larger internal model. I am good to close this issue after the PRs are merged