EMF Validation issue with EDataType

[StephFo]

Hi,
I'm using EMF framework for 20 years.
Today, a developper came to me with a basic validation issue.

The Model to reproduce is quite basic:
EClass Person with attributes

• name:EString
• age:AgeRange

where AgeRange is a EDataType based on integer with minInclusive and maxInclusive values set to 1-99.

Based on EMF generated Editor, we can create Person model instances.

If you enter the PropertySheetPage cell editor for age attribute, an hint is displayed in the status line reporting age has to be between 1-99: perfect so far.

But if you don't edit this age attribute and only fill name value, right clicking to valide the person object, the validation is OK as successfully valid against the model which is not correct since age is undefined and valued with a default value set to 0.
I did not find a way to change EDEFAULT generated value for Integer.MIN_VALUE for instance instead of 0.

I did a try through java code to use Diagnostician.INSTANCE with the same result.

In fact, EObjectValidator implementation (involved in Diagnostician::validate chain) checks data conformity with a method named EObjectValidator ::validate_DataValueConforms that first checks if the attribute (feature) is set.
Since age attribute is unset i.e. value == 0, validate_DataValueConforms returns true to report valid.

Is it the expected behavior in such situation ?

The only way I've found to workaround this problem, is to go to generated code to modify the generated eIsSet() method to return systematically true for age attribute.

But in my context, the generated code is not in GIT, it is produced during pipeline build.

Does anyone have experience this problem ?
Does anyone have an idea to workaround the problem without changing EMF model generated code ?

Regards,

Stéphane

[merks]

You haven't mentioned it, but I assume that the Lower Bound property of the age attribute is 0 (the default) so you've declared age to be valid for the case that the age is not set at all. Change Lower Bound to 1 and then the age must be set so this will kick in when it's not set:

org.eclipse.emf/plugins/org.eclipse.emf.ecore/src/org/eclipse/emf/ecore/util/EObjectValidator.java

Lines 719 to 737 in 47ea84d

	else if (eStructuralFeature.isRequired())
	{
	if (eStructuralFeature.isUnsettable() ? !eObject.eIsSet(eStructuralFeature) : eObject.eGet(eStructuralFeature, false) == null)
	{
	result = false;
	if (diagnostics != null)
	{
	diagnostics.add
	(createDiagnostic
	(Diagnostic.ERROR,
	DIAGNOSTIC_SOURCE,
	EOBJECT__EVERY_MULTIPCITY_CONFORMS,
	"_UI_RequiredFeatureMustBeSet_diagnostic",
	new Object [] { getFeatureLabel(eStructuralFeature, context), getObjectLabel(eObject, context) },
	new Object [] { eObject, eStructuralFeature },
	context));
	}
	}
	}

You probably need to make age unsettable as well given it's primitive. And then, once it's been set, the validation of the value relative to the data type will kick in:

org.eclipse.emf/plugins/org.eclipse.emf.ecore/src/org/eclipse/emf/ecore/util/EObjectValidator.java

Lines 829 to 835 in 47ea84d

	protected boolean validate_DataValueConforms
	(EObject eObject, EAttribute eAttribute, DiagnosticChain diagnostics, Map<Object, Object> context)
	{
	if (!eObject.eIsSet(eAttribute))
	{
	return true;
	}

[StephFo]

Hi Ed,
Thank you for your quick reply.
Please see my comments below, to me there is a bug as explained in the implementation.

Thank you very much for your support.

Stéphane

You haven't mentioned it, but I assume that the Lower Bound property of the age attribute is 0 (the default) so you've declared age to be valid for the case that the age is not set at all. Change Lower Bound to 1 and then the age must be set so this will kick in when it's not set:

Age attribute has its Lower Bound set to 1 as required, as shown in the screenshot

At L720 of EOBjectValidator as you spotted, it should enter the if to complain the attribute is required.
Since Age attribute is based on primitive EDataType, the call to eObject.eGet(eStructuralFeature, false) == null) always returns a non null object. Here age is based on int then an Integer set to 0 is returned.
Indeed, eGet() call will delegate to generated switch case that selects getAge() implementation that is returning the age attribute's value which is 0 because unset.

I can suggest this implementation to replace the if block at L720 like this (you can optimize it if needed)
Do you think it is valuable to raise a ticket to integrate this fix ?

if (eStructuralFeature.isRequired()) {
			if (eStructuralFeature.isUnsettable()) {
				if (!eObject.eIsSet(eStructuralFeature)) {
					result = false;
				}
			} else {
				// EDataType primitive-based specific use-case because
				// eObject.eGet(eStructuralFeature, false) still returns a non null object
				final EClassifier eType = eStructuralFeature.getEType();
				if ((eType instanceof EDataType) && !eObject.eIsSet(eStructuralFeature)) {
					result = false;
				} else if (eObject.eGet(eStructuralFeature, false) == null) {
					result = false;
				}
			}
			if ((result == false) && (diagnostics != null)) {
				diagnostics.add(createDiagnostic(Diagnostic.ERROR, DIAGNOSTIC_SOURCE, EOBJECT__EVERY_MULTIPCITY_CONFORMS, "_UI_RequiredFeatureMustBeSet_diagnostic",
						new Object[] { getFeatureLabel(eStructuralFeature, context), getObjectLabel(eObject, context) }, new Object[] { eObject, eStructuralFeature }, context));
			}

org.eclipse.emf/plugins/org.eclipse.emf.ecore/src/org/eclipse/emf/ecore/util/EObjectValidator.java

Lines 719 to 737 in 47ea84d

	else if (eStructuralFeature.isRequired())
	{
	if (eStructuralFeature.isUnsettable() ? !eObject.eIsSet(eStructuralFeature) : eObject.eGet(eStructuralFeature, false) == null)
	{
	result = false;
	if (diagnostics != null)
	{
	diagnostics.add
	(createDiagnostic
	(Diagnostic.ERROR,
	DIAGNOSTIC_SOURCE,
	EOBJECT__EVERY_MULTIPCITY_CONFORMS,
	"_UI_RequiredFeatureMustBeSet_diagnostic",
	new Object [] { getFeatureLabel(eStructuralFeature, context), getObjectLabel(eObject, context) },
	new Object [] { eObject, eStructuralFeature },
	context));
	}
	}
	}

You probably need to make age unsettable as well given it's primitive. And then, once it's been set, the validation of the value relative to the data type will kick in:

Doing that way it is not really convenient and error prone to retrieve in a big model all usage of such a EDatatype based on primitive type. To me the error is more linked to previous check about the fact the attribute is required due to Lower bound set to 1 than a bad data in it.

org.eclipse.emf/plugins/org.eclipse.emf.ecore/src/org/eclipse/emf/ecore/util/EObjectValidator.java

Lines 829 to 835 in 47ea84d

	protected boolean validate_DataValueConforms
	(EObject eObject, EAttribute eAttribute, DiagnosticChain diagnostics, Map<Object, Object> context)
	{
	if (!eObject.eIsSet(eAttribute))
	{
	return true;
	}

[merks]

Given EMF's age and stability something you consider a fix is nevertheless a change in very long established behavior and is therefore more likely to cause new problems than it is to make many people a little bit happier:

On the one hand, this logic would effectively kick in for all EAttributes:

				if ((eType instanceof EDataType) && !eObject.eIsSet(eStructuralFeature)) {
					result = false;

On the other hand, it's really very easy to change unsettable to true for your model to achieve the same effect as would be achieved by changing the logic in the validator for every EAttribute for everyone:

In the end, it is valid to not be set and given the model instance cannot distinguish the difference between being set to 0 versus being not set at all (and defaulting to 0 or whatever the default is defined to be; in both cases the value will not be serialized nor copied), making the attribute unsettable disambiguates that state such that explicitly setting the value to 0 would result in a validation error that 0 is outside of the valid range while not setting it at all would result in a validation error that the value must be set to something.

[StephFo]

Given EMF's age and stability something you consider a fix is nevertheless a change in very long established behavior and is therefore more likely to cause new problems than it is to make many people a little bit happier:

On the one hand, this logic would effectively kick in for all EAttributes:

				if ((eType instanceof EDataType) && !eObject.eIsSet(eStructuralFeature)) {
					result = false;

On the other hand, it's really very easy to change unsettable to true for your model to achieve the same effect as would be achieved by changing the logic in the validator for every EAttribute for everyone:

Not so easy when your model is about 100++ Class with many attributes referring to primitive EDataTypes.

I'm with you about consequences of a minor change in EMF EValidator implementation even if I'm sure you have many unit tests to check for non regressions.

On my side, we've decided to not go to 'unsettable' set to true manually for now but we added a second step after Diagnostician.INSTANCE.validate(...) call.
This second step runs a second loop to only check EAttributes with a Lower Bound > 0 and that are unset. Of course it is not available in generated EMF Editor see my comment below.

In the end, it is valid to not be set **and** given the model instance cannot distinguish the difference between being set to 0 versus being not set at all (and defaulting to 0 or whatever the default is defined to be; in both cases the value will not be serialized nor copied), making the attribute unsettable disambiguates that state such that explicitly setting the value to 0 would result in a validation error that 0 is outside of the valid range while not setting it at all would result in a validation error that the value must be set to something.

I'm using EMF framework to generate XML content shared with other partners that are producing / consuming XML content with different implementations most of time JAXB for java language.

Then we extended XmlResource, to force the generation of the full model content without the standard optimisations I mean default enum values or default boolean values are not serialized ...
In my situation, XML serialization contains age attribute with the 0 value serialized. I can be producer or consumer of such a model.
As you mentioned making the attribute unsettable disambiguates the state. It is the best approach in my use case.

Then, I will take your advice by creating a basic tool, that walks other our Ecore model to automatically set to true, unsettable attribute as soon as an EAttribute has a Lower Bound > 0.

This way, I will be able to remove the extra validation loop, and EMF Editor will validate correctly as well .

Thank you very much Ed for your time and advices.

Stéphane

[merks]

BTW, I was thinking today again about your comment.

I wondered if you are using this:

org.eclipse.emf/plugins/org.eclipse.emf.ecore.xmi/src/org/eclipse/emf/ecore/xmi/XMLResource.java

Lines 198 to 203 in 47ea84d

	/**
	* Keep default content ( e.g. default attributes). This applies to saving and converting contents to DOM.
	* By default the default content is discarded.
	* To save the default content, set this option to <code>Boolean.TRUE</code>.
	*/
	String OPTION_KEEP_DEFAULT_CONTENT = "KEEP_DEFAULT_CONTENT";

It also occurred to me that if you defined the default of the attribute in your model to be a valid value, e.g., the same value as minInclusive, that maybe that's also a good or even better approach. Just a thought...