Add support of the inheritance with generics. #99

ma1uta · 2018-10-17T20:02:51Z

This also fixes the #97.

zapov · 2018-10-18T06:58:59Z

java8/src/main/java/com/dslplatform/json/processor/ConverterTemplate.java

@@ -109,10 +117,32 @@ private void asFormatConverter(final StructInfo si, final String name, final Str
 			code.append("\t\tprivate final java.lang.reflect.Type[] actualTypes;\n");
 		}

+		Map<String, TypeMirror> genericAttributes = new HashMap<>();


It would be more consistent to do this analysis in Analysis.java (in analyze() method)

I see, I will move this analysis to the Analysis class.

zapov · 2018-10-18T06:59:47Z

java8/src/main/java/com/dslplatform/json/processor/ConverterTemplate.java

@@ -109,10 +117,32 @@ private void asFormatConverter(final StructInfo si, final String name, final Str
 			code.append("\t\tprivate final java.lang.reflect.Type[] actualTypes;\n");
 		}

+		Map<String, TypeMirror> genericAttributes = new HashMap<>();
+		for (AttributeInfo attr : si.attributes.values()) {
+			if (!si.isParameterized && attr.isGeneric) {


We can handle generics later.... but in that case at least leave a TODO comment

We can lost the actual generic values later. I will double check this code.

zapov · 2018-10-18T07:00:33Z

java8/src/main/java/com/dslplatform/json/processor/ConverterTemplate.java

@@ -227,7 +265,11 @@ private void asFormatConverter(final StructInfo si, final String name, final Str
 					if (attr.isArray) {
 						type = typeForGeneric(((ArrayType) attr.type).getComponentType(), attr.typeVariablesIndex);
 					} else {
-						type = typeForGeneric(attr.type, attr.typeVariablesIndex);
+					    if (si.isParameterized) {


Can you pretty please fix your alignments?

Yes, of course.

zapov · 2018-10-18T07:09:16Z

library/src/main/java/com/dslplatform/json/DslJson.java

+	 * @param stream 		where to write resulting JSON
+	 * @throws IOException error when unable to serialize instance
+	 */
+	public final void serialize(@Nullable final Object value, @Nullable Class<?> typeManifest, final OutputStream stream) throws IOException {


If we are gonna have such method, it should accept a Type instead of Class and it should not be nullable

I guess the method is useful (even if there are already too many methods here...)

I kind of am not a fan of such method (since you can send in a different object from the signature),
but it is useful.
What might make more sense it to have a generic signature if we are passing in a Class

I think we can avoid this method. Also I found a bug (?) if deserialize a child class the resulting json will be without discriminator.

ma1uta · 2018-10-18T15:58:51Z

Fix remarks. Please don't merge this commit, I'll try to fix the bug with serialization and remove a new method in the DslJson.

… method to serialize.

ma1uta · 2018-10-18T20:05:25Z

I've fixed the bug with missing descriptor. Could you please code review changes again?

zapov · 2018-10-19T08:23:40Z

java8/src/main/java/com/dslplatform/json/processor/ConverterTemplate.java

@@ -24,11 +30,13 @@
 	private final Writer code;
 	private final Context context;
 	private final EnumTemplate enumTemplate;
+	private final ProcessingEnvironment processingEnv;


You are not using this anymore. Please clean it up

Ok, I will do it.

zapov · 2018-10-19T08:27:34Z

java8/src/main/java/com/dslplatform/json/processor/ConverterTemplate.java

@@ -425,7 +441,33 @@ void fromObject(final StructInfo si, final String className) throws IOException
 		code.append("\t}\n");
 	}

-	private void writeObject(final String className, List<AttributeInfo> sortedAttributes) throws IOException {
+	private void writeDiscriminator(final StructInfo si) throws IOException {


This doesn't look right.
There is the MixinFormatter which adds discriminator.
Discriminator makes sense only on base/abstract classes and interfaces.
If you are serializing concrete class it should be serialized without the discriminator.
If you are serializing interface/abstract it should be serialized with the discriminator.

You mentioned some issue which you found? Which test covers that specific issue and this generated code?

I thought a bit about what you are trying to do here and I think I understand.
It makes sense to always serialize discriminator when it's defined on an object.
The current deserializeDiscriminator was kind of useless on non abstract things anyway, and this would make it useful.
In that regard it would be better if I called it just discriminator (it's still new so I'm fine with making that breaking change).
But in that case, discriminator should be written outside of writeContentFull, since that is reused by the mixin thing.

Yes, might be the MixinFormatter is better place where we can write deserializer.

MixinFormatter already does the expected thing. If we want to have this new feature to always put discriminator even on classes, you should do it outside of writeContent method, eg, here: https://github.com/ngs-doo/dsl-json/blob/master/java8/src/main/java/com/dslplatform/json/processor/ConverterTemplate.java#L442 and https://github.com/ngs-doo/dsl-json/blob/master/java8/src/main/java/com/dslplatform/json/processor/ConverterTemplate.java#L599

zapov · 2018-10-19T08:29:27Z

library/src/main/java/com/dslplatform/json/processor/Analysis.java

 				path.pop();
 			}
 		} while (total != structs.size());
 	}

+	private void findGenericAttributes(StructInfo si) {


This looks like it can be done when we register the attribute, instead of later changing the attribute with genericType info

Might be better if we avoid the genericType and fix the type (?) attribute which should point to the correct class. The problem is the concrete class may be lost if we go deep into parent. This method in general is a workaround, for example declaredType.getTypeArguments() return correct class. If we do Element element = declaredType.asElement() the element will lost the information about this class and return the parameter value T.

@varahash added the support for generics so he might know better
I know of two main cases:

where we need runtime info to fully construct the correct type (in which case we pass in some additional type info to the converter and do it in runtime)

where we have all the info at compile time

Here is the conversion for the runtime case: https://github.com/ngs-doo/dsl-json/blob/master/java8/src/main/java/com/dslplatform/json/processor/ConverterTemplate.java#L274
But compile time case should be just a type?

zapov · 2018-10-19T08:29:57Z

library/src/main/java/com/dslplatform/json/processor/AttributeInfo.java

@@ -35,6 +35,7 @@
 	public final boolean isGeneric;
 	public final Map<String, Integer> typeVariablesIndex;
 	public final boolean containsStructOwnerType;
+	public TypeMirror genericType;


Ideally it should be final, unless there is some important reason it cant be

Please see my previous comment. I don't know the code well and might be we can remove this parameter.

zapov · 2018-10-20T05:52:22Z

tests-java8/src/test/java/com/dslplatform/json/ComplexGenericsTest.java

+        assertTrue(child.getBoolValue());
+
+        ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
+        dslJson.serialize(child, outputStream);


The previous test where you explicitly specified the type was the expected behavior.
This doesn't look like the expected behavior to me unless you explicitly add that discriminator also to the class annotation

zapov · 2018-10-21T13:40:46Z

btw. This is really close to done (with some behavior changes)
I can merge it as is and fix remaining stuff next weekend if that will help

ma1uta · 2018-10-24T05:14:53Z

Sorry, I was busy. I have time to wait and we can do it correct. I will try to find time to fix remarks and improve the code.

zapov · 2018-10-24T06:59:49Z

Np. We are not in hurry :)

zapov · 2018-11-17T09:12:51Z

@ma1uta do you have any ETA when you will be able to finish this?

ma1uta · 2018-12-17T12:18:18Z

Sorry, unfortunately I was off these days.
Thanks for changes.

zapov · 2018-12-18T10:04:03Z

Np. Tnx for the initial effort

Add support of the inheritance with generics.

23961d4

zapov reviewed Oct 18, 2018

View reviewed changes

Fix remarks.

fd6f197

Write the discriminator of the child classes. Remove previously added…

8687416

… method to serialize.

zapov reviewed Oct 19, 2018

View reviewed changes

zapov reviewed Oct 20, 2018

View reviewed changes

zapov mentioned this pull request Nov 17, 2018

Don't allocate memory when ensuring capacity #102

Merged

zapov merged commit 8687416 into ngs-doo:master Nov 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support of the inheritance with generics. #99

Add support of the inheritance with generics. #99

ma1uta commented Oct 17, 2018

zapov Oct 18, 2018

ma1uta Oct 18, 2018

zapov Oct 18, 2018

ma1uta Oct 18, 2018

zapov Oct 18, 2018

ma1uta Oct 18, 2018

zapov Oct 18, 2018

ma1uta Oct 18, 2018

ma1uta commented Oct 18, 2018

ma1uta commented Oct 18, 2018

zapov Oct 19, 2018

ma1uta Oct 24, 2018

zapov Oct 19, 2018

zapov Oct 20, 2018

ma1uta Oct 24, 2018

zapov Oct 24, 2018

zapov Oct 19, 2018

ma1uta Oct 24, 2018

zapov Oct 24, 2018

zapov Oct 19, 2018

ma1uta Oct 24, 2018

zapov Oct 20, 2018

ma1uta Oct 24, 2018

zapov commented Oct 21, 2018

ma1uta commented Oct 24, 2018

zapov commented Oct 24, 2018

zapov commented Nov 17, 2018

ma1uta commented Dec 17, 2018

zapov commented Dec 18, 2018

Add support of the inheritance with generics. #99

Add support of the inheritance with generics. #99

Conversation

ma1uta commented Oct 17, 2018

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

Choose a reason for hiding this comment

ma1uta commented Oct 18, 2018

ma1uta commented Oct 18, 2018

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

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

zapov commented Oct 21, 2018

ma1uta commented Oct 24, 2018

zapov commented Oct 24, 2018

zapov commented Nov 17, 2018

ma1uta commented Dec 17, 2018

zapov commented Dec 18, 2018