[22] Support Metamodel migration, POJO datatype and feature serialization ordering #21

vrichard12 · 2023-11-06T15:04:12Z

Addresses the following subjects:

Handling the renaming of EAttribute
Renaming of EAttribute is handled by the ExtendedMetaData.getElement() API. Examples of such a migration is provided in the org.eclipse.sirius.emfjson.tests.internal.unit.load.ExtendedMetaDataAttributesLoadTests test class.
Handling a type change or a value change of an EAttribute
Changing the type or the value of an EAttribute is handled with the JsonHelper.setValue() API. Exemple of such migrations are provided in the org.eclipse.sirius.emfjson.tests.internal.unit.load.JsonHelperDataLoadTests test class.
Support for Serialization / deserialization of POJO EDataType
Custom data types with instance type set to a POJO class are serialized to / deserialized from json. The corresponding tests are :
org.eclipse.sirius.emfjson.tests.internal.unit.load.DataTypeLoadTests.testLoadSingleValueAttributePojoDataType()
org.eclipse.sirius.emfjson.tests.internal.unit.load.DataTypeLoadTests.testLoadMultiValuedAttributePojoDataType()
org.eclipse.sirius.emfjson.tests.internal.unit.save.DataTypeSaveTests.testSaveSingleValueAttributePojoDataType()
org.eclipse.sirius.emfjson.tests.internal.unit.save.DataTypeSaveTests.testSaveMultiValuedAttributePojoDataType()
A comparator option on the JsonResource to determine the order in which features are serialized
org.eclipse.sirius.emfjson.tests.internal.unit.save.SerializeOptionsTests.testSerializationWithFeatureOrderComparator()

vrichard12 · 2023-11-06T15:21:50Z

fixes #22

sbegaudeau · 2023-11-14T09:58:34Z

bundles/org.eclipse.sirius.emfjson/META-INF/MANIFEST.MF

@@ -19,5 +19,5 @@ Import-Package: javax.xml.bind;version="0.0.0",
 org.eclipse.emf.ecore.resource;version="0.0.0",
 org.eclipse.emf.ecore.resource.impl;version="0.0.0",
 org.eclipse.emf.ecore.util;version="0.0.0"
+Require-Bundle: javax.xml.bind


Why are you removing and then adding back java.xml.bind and why remove its version?

I didn't remove it, I added it. It was missing and it was blocking the build.

No, you are wrong here, you literally removed it in the first commit of this pull request:

Before adding it back as I said but without the version number:

So I'm trying to understand both changes, I suspect that the first one could be removed and the second one too.

sbegaudeau · 2023-11-14T10:09:17Z

...e.sirius.emfjson/src/main/java/org/eclipse/sirius/emfjson/utils/GsonEObjectDeserializer.java

+    private static final List<EDataType> ECORE_PACKAGE_DATA_TYPES = Arrays.asList(EcorePackage.eINSTANCE.getEString(), EcorePackage.eINSTANCE.getEBoolean(), EcorePackage.eINSTANCE.getEBooleanObject(),
+            EcorePackage.eINSTANCE.getEInt(), EcorePackage.eINSTANCE.getEIntegerObject(), EcorePackage.eINSTANCE.getEBigDecimal(), EcorePackage.eINSTANCE.getEBigInteger(),
+            EcorePackage.eINSTANCE.getEByte(), EcorePackage.eINSTANCE.getEByteArray(), EcorePackage.eINSTANCE.getEByteObject(), EcorePackage.eINSTANCE.getEChar(),
+            EcorePackage.eINSTANCE.getECharacterObject(), EcorePackage.eINSTANCE.getEDate(), EcorePackage.eINSTANCE.getEDouble(), EcorePackage.eINSTANCE.getEDoubleObject(),
+            EcorePackage.eINSTANCE.getEFloat(), EcorePackage.eINSTANCE.getEFloatObject(), EcorePackage.eINSTANCE.getELong(), EcorePackage.eINSTANCE.getELongObject(),
+            EcorePackage.eINSTANCE.getEShort(), EcorePackage.eINSTANCE.getEShortObject());
+
+    /**
+     * Tests if the given {@link EDataType} is a POJO.
+     *
+     * @param dataType
+     *            A {@link EDataType} to test
+     * @return true if the given {@link EDataType} is not one of the {@link EcorePackage} DataTypes nor an
+     *         {@link EEnum}.
+     */
+    private boolean isPojo(EDataType dataType) {
+        return !ECORE_PACKAGE_DATA_TYPES.contains(dataType) && //
+                dataType.eClass() != EcorePackage.eINSTANCE.getEEnum() && //
+                dataType.getInstanceClass() != null;
+    }


Why would we consider that if something is not from Ecore then we should not leverage the method EcoreUtil.createFromString? Is there anything like that in XMIResource? If not, I don't understand the need for an ad hoc serialization system in our code.

EcoreUtil.createFromString does not handle json format. I don't see your point.
What would anything be like in XMIResource?

In XMI resource, you would have to implement the expected methods from EMF and you would not return XML data structure this way. But if done properly, your meta-model could work for every kind of resources, XMI, Json, binary, etc

sbegaudeau

This pull request mixes commits which look good from afar (I have not tested nor really reviewed them) and a couple of commits which are a bit odd for me at the moment. Such a big PR mixing various concerns is quite slow to integrate.

For the content that seems odd to me, I don't see why we would need a custom serialization mechanism on top of the EMF one nor why we should change the current behavior for datatypes.

vrichard12 · 2023-11-16T10:25:34Z

As I said in the associated issue #22, before this evolution, the POJO serialization felt back to a toString() call, producing this kind of string: org.obeonetwork.some.package.SomeType@147c1776. What we want is to be able to handle a json object structure in order to be fully compliant with the json format.
We need a custom serialization mechanism on top of the EMF one to handle the Json format. That's the purpose of JsonResourceImpl. All we do here is add the support for a corner case that wasn't handled: custom datatypes with an instance type name referring to a POJO.

sbegaudeau · 2024-01-11T07:00:36Z

I've merged 90% of the commits of this pull request as part of the pr #31 and performed the release of EMFJson 2.3.6. This way, we can continue our discussion regarding the last commit since I still have some issues with the code.

vrichard12 · 2024-01-29T16:41:19Z

Hi Stéphane,

To get everyone in agreement, I've set up an option to specify which POJO should be considered by Json serialization/deserialization (OPTION_IS_EDATATYPE_SERIALIZABLE_IN_JSON_TESTER).

You'll notice that I've also avoided calling super.getElement() in the BasicExtendedMetaData subclasses to avoid falling into the XMI-specific processing that has been implemented in BasicExtendedMetaData.

Finally, I had to correct a behavior in GsonEObjectSerializer.serializeEDataType() in the case of a single-valued attribute. When the stringValue was null, the code created a JsonPrimitive(""). This poses a problem when serializing POJO type attributes, where it cannot be serialized as the empty string. When the default value of an attribute is null, and the value of the attribute in the model to be serialized is also null, nothing should be serialized.

lfasani · 2024-01-31T07:49:13Z

...g.eclipse.sirius.emfjson/src/main/java/org/eclipse/sirius/emfjson/resource/JsonResource.java

@@ -425,6 +428,12 @@ interface IEObjectHandler {
     */
    Object OPTION_SAVE_FEATURES_ORDER_COMPARATOR = "OPTION_SAVE_FEATURES_ORDER_COMPARATOR"; //$NON-NLS-1$

+    /**
+     * Specify a {@link Function} returning {@link Boolean#TRUE} if a given {@link EDataType} should be serialized


good remark from Gionatta. Prefer a Predicate that is designed for this need.

Definitely yes! Thank you for the remark. I fixed that.

lfasani · 2024-01-31T07:50:52Z

...e.sirius.emfjson/src/main/java/org/eclipse/sirius/emfjson/utils/GsonEObjectDeserializer.java

+     * @param eDataType
+     * @return true if the given {@link EDataType} should be deserialized from a Json tree.
+     */
+    private boolean isEDataTypeSerializableInJson(EDataType eDataType) {


isEDataTypeSerializableInJson -> isSerializeEDataTypeInJson

lfasani · 2024-01-31T07:53:50Z

...pse.sirius.emfjson/src/main/java/org/eclipse/sirius/emfjson/utils/GsonEObjectSerializer.java

+            if (this.isEDataTypeSerializableInJson(eAttribute.getEAttributeType())) {
+                jsonElement = new Gson().toJsonTree(value);
+            } else {
+                if (stringValue == null) {


prefer checking null first like line 1046. It avoid letting the given Predicate manage this case (if any)

Yes you're right, the code will be cleaner like that. I fix that right now.

lfasani · 2024-01-31T07:54:03Z

...pse.sirius.emfjson/src/main/java/org/eclipse/sirius/emfjson/utils/GsonEObjectSerializer.java

+    private boolean isEDataTypeSerializableInJson(EDataType eDataType) {
+        @SuppressWarnings("unchecked")


isEDataTypeSerializableInJson -> isSerializeEDataTypeInJson

The naming "isSerializeEDataTypeInJson" suggests that the given EDataType is serializable and that, based on this assumption, tests whether the "Json" serialization is a valid target for serialization. I find this naming incorrect. I prefer to keep "isEDataTypeSerializableInJson", which says exactly what the test does.

Actually you're right on one point. The test should not say "can" but "should". I propose to rename it "shouldEDataTypeBeSerializedInJson" which reflect the real intent.

I fixed the code accordingly.

gion86 · 2024-05-17T07:04:40Z

...pse.sirius.emfjson/src/main/java/org/eclipse/sirius/emfjson/utils/GsonEObjectSerializer.java

            String stringValue = eFactoryInstance.convertToString(eAttribute.getEAttributeType(), value);
-
            if (stringValue == null) {
-                stringValue = ""; //$NON-NLS-1$
+                jsonElement = JsonNull.INSTANCE;
+            } else if (this.shouldEDataTypeBeSerializedInJson(eAttribute.getEAttributeType())) {
+                jsonElement = new Gson().toJsonTree(value);
+            } else {
+                jsonElement = new JsonPrimitive(stringValue);
            }


I think that shouldEDataTypeBeSerializedInJson() should be called first, like:

if (this.shouldEDataTypeBeSerializedInJson(eAttribute.getEAttributeType())) { jsonElement = new Gson().toJsonTree(value); } else { String stringValue = eFactoryInstance.convertToString(eAttribute.getEAttributeType(), value); if (stringValue == null) { jsonElement = JsonNull.INSTANCE; } else { jsonElement = new JsonPrimitive(stringValue); } }

It's the same beahviour of the eAttribute.isMany() case above.

Bug: eclipse-sirius#30

Signed-off-by: Stéphane Bégaudeau <stephane.begaudeau@obeo.fr>

vrichard12 changed the title ~~Support Metamodel migration, POJO datatype and feature serialization ordering~~ Support Metamodel migration, POJO datatype and feature serialization ordering (fixes #22) Nov 6, 2023

gcoutable linked an issue Nov 6, 2023 that may be closed by this pull request

Support Metamodel migration, POJO datatype and feature serialization ordering #22

Closed

gcoutable changed the title ~~Support Metamodel migration, POJO datatype and feature serialization ordering (fixes #22)~~ [22] Support Metamodel migration, POJO datatype and feature serialization ordering Nov 6, 2023

gcoutable added the enhancement New feature or request label Nov 6, 2023

sbegaudeau reviewed Nov 14, 2023

View reviewed changes

sbegaudeau requested changes Nov 14, 2023

View reviewed changes

vrichard12 force-pushed the migration-pojodatatype-featureordering branch from 4505bc8 to 22f04b9 Compare November 22, 2023 16:18

lfasani assigned lfasani and vrichard12 and unassigned lfasani Dec 13, 2023

sbegaudeau force-pushed the migration-pojodatatype-featureordering branch from 22f04b9 to 63ed710 Compare January 11, 2024 06:50

vrichard12 force-pushed the migration-pojodatatype-featureordering branch 4 times, most recently from 9ee6461 to 17dc1c4 Compare January 29, 2024 16:16

lfasani requested changes Jan 31, 2024

View reviewed changes

vrichard12 force-pushed the migration-pojodatatype-featureordering branch 2 times, most recently from 4a427b6 to 4eb40c1 Compare January 31, 2024 09:37

lfasani approved these changes Jan 31, 2024

View reviewed changes

lredor requested a review from sbegaudeau March 5, 2024 14:54

gion86 reviewed May 17, 2024

View reviewed changes

[30] Add support for Serialization / deserialization of POJO

36e85fa

Bug: eclipse-sirius#30

sbegaudeau force-pushed the migration-pojodatatype-featureordering branch from 4eb40c1 to 36e85fa Compare May 24, 2024 13:05

sbegaudeau linked an issue May 24, 2024 that may be closed by this pull request

Add the ability to save values as custom JSON data structure #30

Closed

sbegaudeau added this to the 2024.7.0 milestone May 24, 2024

sbegaudeau approved these changes May 24, 2024

View reviewed changes

[releng] Bump version to 2.3.9

53d6044

Signed-off-by: Stéphane Bégaudeau <stephane.begaudeau@obeo.fr>

sbegaudeau merged commit e13d502 into eclipse-sirius:master May 24, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[22] Support Metamodel migration, POJO datatype and feature serialization ordering #21

[22] Support Metamodel migration, POJO datatype and feature serialization ordering #21

vrichard12 commented Nov 6, 2023 •

edited

Loading

vrichard12 commented Nov 6, 2023

sbegaudeau Nov 14, 2023

vrichard12 Nov 16, 2023

sbegaudeau Jan 10, 2024

sbegaudeau Nov 14, 2023

vrichard12 Nov 16, 2023

sbegaudeau Jan 10, 2024

sbegaudeau left a comment

vrichard12 commented Nov 16, 2023

sbegaudeau commented Jan 11, 2024

vrichard12 commented Jan 29, 2024

lfasani Jan 31, 2024

vrichard12 Jan 31, 2024

lfasani Jan 31, 2024

lfasani Jan 31, 2024

vrichard12 Jan 31, 2024

vrichard12 Jan 31, 2024

lfasani Jan 31, 2024

vrichard12 Jan 31, 2024 •

edited

Loading

vrichard12 Jan 31, 2024

vrichard12 Jan 31, 2024

gion86 May 17, 2024

		private boolean isEDataTypeSerializableInJson(EDataType eDataType) {
		@SuppressWarnings("unchecked")

[22] Support Metamodel migration, POJO datatype and feature serialization ordering #21

[22] Support Metamodel migration, POJO datatype and feature serialization ordering #21

Conversation

vrichard12 commented Nov 6, 2023 • edited Loading

vrichard12 commented Nov 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbegaudeau left a comment

Choose a reason for hiding this comment

vrichard12 commented Nov 16, 2023

sbegaudeau commented Jan 11, 2024

vrichard12 commented Jan 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrichard12 Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrichard12 commented Nov 6, 2023 •

edited

Loading

vrichard12 Jan 31, 2024 •

edited

Loading