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.

Sign up for GitHub

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

Jump to bottom

Support custom data type (de)serialization with EFactory #26

Closed
wants to merge 1 commit into from
Closed

Support custom data type (de)serialization with EFactory #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/main/java/org/eclipse/emfcloud/jackson/databind/deser/EDataTypeDeserializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import java.io.IOException;

import org.eclipse.emf.ecore.EDataType;
import org.eclipse.emf.ecore.EEnum;
import org.eclipse.emf.ecore.util.EcoreUtil;
import org.eclipse.emfcloud.jackson.databind.EMFContext;

Expand All @@ -26,6 +25,11 @@

public class EDataTypeDeserializer extends JsonDeserializer<Object> {

public static boolean isJavaLangType(final EDataType dataType) {
String instanceClassName = dataType.getInstanceClassName();
return instanceClassName.startsWith("java.lang.") || instanceClassName.indexOf('.') < 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a rather greedy approach since we cannot really guarantee that we support all java.lang.* types.
For instance, the EcoreUtil.createFromString call will fail hard if you do not provide the necessary create/convert function in the factory.

Also having this check as a static method makes it impossible to override this in any sub-class.

Copy link
Author

@hallvard hallvard Mar 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you define a data type, it is your duty (contract to be fulfilled) to implement the create/convert methods if you need to support serialisation. That's part of the job when using EMF. Hence, IMO (as detailed below) we should be even more greedy. I'm not sure why the original implementation doesn't rely on EFactory in general, I'd assume that all pre-defined data types in Ecore support serialization and just should work. However, the tests would need to be re-written, since there are assumptions there of the specific format that may not match Ecore's. Some cases won't work, e.g. Object[] is handled not by a general serialization of Objects, but using JSON-specific logic, if I understand it correctly. I would start by reverting the logic, use EFactory in all cases where the data type has the serializable flag set and then see if there are special cases that could need special handling, e.g. when a data type wraps an array of something that can be serialized.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to make certain data types easier to support, we can also define a separate Emfjson model with pre-defined data types, so anyone can import it and use its data types. This can be cleaner than hard-coding the logic in the serialization class.
An orthogonal option is to introduce some annotations on the data type, for controlling how emfjson-specific logic should be used for serialization.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are also not sure why the original implementation did not rely on EFactory in general but I think it is the right way forward. However, as to not break backwards compatibility, we should use an EAnnotation to allow switching between strategies as there might be some customizations we are not aware of. There is already little support for annotations in JsonAnnotations.

}

@Override
public Object deserialize(final JsonParser jp, final DeserializationContext ctxt) throws IOException {
final EDataType dataType = EMFContext.getDataType(ctxt);
Expand All @@ -35,7 +39,7 @@ public Object deserialize(final JsonParser jp, final DeserializationContext ctxt
}
Class<?> type = dataType.getInstanceClass();

if (type == null || dataType instanceof EEnum || EJAVA_CLASS.equals(dataType)
if (type == null || (!isJavaLangType(dataType)) || EJAVA_CLASS.equals(dataType)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you removed the EEnum check for this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't really matter, as I guess the default handling of EFactory and current emfjson implementation should be the same. I think the main decision should be made elsewhere and this class always use the EFactory.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the EAnnotation approach this check should be replaced by checking the annotation to see which strategy should be used.

|| EJAVA_OBJECT.equals(dataType)) {
return EcoreUtil.createFromString(dataType, jp.getText());
}
Expand Down
36 changes: 33 additions & 3 deletions src/main/java/org/eclipse/emfcloud/jackson/databind/property/EObjectFeatureProperty.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,27 @@

import java.io.IOException;

import com.fasterxml.jackson.core.JsonParseException;
import org.eclipse.emf.ecore.EAttribute;
import org.eclipse.emf.ecore.EDataType;
import org.eclipse.emf.ecore.EObject;
import org.eclipse.emf.ecore.EStructuralFeature;
import org.eclipse.emf.ecore.EcorePackage;
import org.eclipse.emf.ecore.resource.Resource;
import org.eclipse.emfcloud.jackson.databind.EMFContext;
import org.eclipse.emfcloud.jackson.databind.deser.EDataTypeDeserializer;
import org.eclipse.emfcloud.jackson.databind.deser.ReferenceEntries;
import org.eclipse.emfcloud.jackson.databind.deser.ReferenceEntry;
import org.eclipse.emfcloud.jackson.databind.ser.EDataTypeSerializer;
import org.eclipse.emfcloud.jackson.databind.type.FeatureKind;

import com.fasterxml.jackson.core.JsonGenerator;
import com.fasterxml.jackson.core.JsonParseException;
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonToken;
import com.fasterxml.jackson.databind.DeserializationContext;
import com.fasterxml.jackson.databind.JavaType;
import com.fasterxml.jackson.databind.JsonDeserializer;
import com.fasterxml.jackson.databind.JsonMappingException;
import com.fasterxml.jackson.databind.JsonSerializer;
import com.fasterxml.jackson.databind.SerializerProvider;
import com.fasterxml.jackson.databind.ser.impl.UnknownSerializer;
Expand All @@ -52,12 +57,28 @@ public EObjectFeatureProperty(final EStructuralFeature feature, final JavaType t
this.defaultValues = OPTION_SERIALIZE_DEFAULT_VALUE.enabledIn(features);
}

private static final EDataTypeDeserializer EDATA_TYPE_DESERIALIZER = new EDataTypeDeserializer();

private boolean shouldUseEFactory(final EDataType dataType) {
return dataType.isSerializable() && dataType.getEPackage() != EcorePackage.eINSTANCE
&& (!EDataTypeDeserializer.isJavaLangType(dataType));
}

private JsonDeserializer<Object> findValueDeserializer(final DeserializationContext ctxt)
throws JsonMappingException {
if ((!feature.isMany()) && feature instanceof EAttribute
&& shouldUseEFactory(((EAttribute) feature).getEAttributeType())) {
return EDATA_TYPE_DESERIALIZER;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to avoid using static instances of concrete classes like this as it makes it impossible for adopters to avoid it's usage. At the moment deserializers are registered through the EMFDeserializers class so I think we should use the registration mechanism to retrieve the deserializer we want:

ctxt.findContextualValueDeserializer(ctxt.getTypeFactory().constructType(EcoreType.DataType.class), null)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: Sounds like a good idea!

}
return ctxt.findContextualValueDeserializer(javaType, null);
}

@Override
@SuppressWarnings({ "checkstyle:cyclomaticComplexity", "checkstyle:fallThrough" })
public void deserializeAndSet(final JsonParser jp, final EObject current, final DeserializationContext ctxt,
final Resource resource)
throws IOException {
final JsonDeserializer<Object> deserializer = ctxt.findContextualValueDeserializer(javaType, null);
final JsonDeserializer<Object> deserializer = findValueDeserializer(ctxt);
JsonToken token = null;

if (jp.getCurrentToken() == JsonToken.FIELD_NAME) {
Expand Down Expand Up @@ -120,11 +141,20 @@ public void deserializeAndSet(final JsonParser jp, final EObject current, final
}
}

private static final EDataTypeSerializer EDATA_TYPE_SERIALIZER = new EDataTypeSerializer();

private JsonSerializer<Object> findValueSerializer(final SerializerProvider provider) throws JsonMappingException {
if (feature instanceof EAttribute && shouldUseEFactory(((EAttribute) feature).getEAttributeType())) {
return EDATA_TYPE_SERIALIZER;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to avoid using static instances of concrete classes like this as it makes it impossible for adopters to avoid it's usage. At the moment serializers are registered through the EMFSerializers class so I think we should use the registration mechanism to retrieve the serializer we want:

provider.findValueSerializer(EcoreType.DataType.class);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good idea!

}
return provider.findValueSerializer(javaType);
}

@Override
public void serialize(final EObject bean, final JsonGenerator jg, final SerializerProvider provider)
throws IOException {
if (serializer == null) {
serializer = provider.findValueSerializer(javaType);
serializer = findValueSerializer(provider);
}

EMFContext.setParent(provider, bean);
Expand Down
33 changes: 33 additions & 0 deletions src/test/java/org/eclipse/emfcloud/jackson/tests/ValueTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.io.IOException;
import java.math.BigDecimal;
import java.math.BigInteger;
import java.time.LocalDate;
import java.util.Calendar;
import java.util.Date;
import java.util.List;
Expand Down Expand Up @@ -401,4 +402,36 @@ public void testSaveObjectTypeValue() {

assertThat(result.get("objectType").isNumber()).isTrue();
}

@Test
public void testLocalDateValue() {
Resource resource = resourceSet.createResource(URI.createURI("tests/test.json"));

ETypes valueObject = ModelFactory.eINSTANCE.createETypes();
valueObject.setELocalDate(LocalDate.of(2021, 9, 13));
resource.getContents().add(valueObject);

JsonNode result = mapper.valueToTree(resource);

assertEquals("2021-09-13", result.get("eLocalDate").asText());
}

@Test
public void testLoadLocalDateValue() throws IOException {
JsonNode data = mapper.createObjectNode()
.put("eClass", "http://www.emfjson.org/jackson/model#//ETypes")
.put("eLocalDate", "2021-09-13");

Resource resource = resourceSet.createResource(URI.createURI("tests/test.json"));
resource.load(new ByteArrayInputStream(mapper.writeValueAsBytes(data)), null);

assertEquals(1, resource.getContents().size());

EObject root = resource.getContents().get(0);
assertEquals(ModelPackage.Literals.ETYPES, root.eClass());

LocalDate value = ((ETypes) root).getELocalDate();

assertEquals(LocalDate.of(2021, 9, 13), value);
}
}
10 changes: 10 additions & 0 deletions src/test/resources/model/model.xcore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ package org.eclipse.emfcloud.jackson.junit.model
import org.eclipse.emf.ecore.EByteArray
import org.eclipse.emf.ecore.EFeatureMapEntry
import java.util.Map
import java.time.format.DateTimeFormatter
import java.time.LocalDate

class User {
id String userId
Expand Down Expand Up @@ -62,6 +64,7 @@ class ETypes {
contains StringMap[*] stringMapValues
contains DataTypeMap[*] dataTypeMapValues
unique URI[] uris
ELocalDate eLocalDate
}

class StringMap wraps Map.Entry {
Expand Down Expand Up @@ -96,6 +99,13 @@ type URI wraps org.eclipse.emf.common.util.URI
type UserType wraps String
type ObjectType wraps Object
type ObjectArrayType wraps Object[]
type ELocalDate wraps java.time.LocalDate
convert {
if (it === null) "" else DateTimeFormatter.ISO_LOCAL_DATE.format(it);
}
create {
if (it === null || it.trim().empty) null else LocalDate.parse(it, DateTimeFormatter.ISO_LOCAL_DATE);
}

class PrimaryObject {
String name
Expand Down