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

Add support of the inheritance with generics. #99

Merged
merged 3 commits into from
Nov 19, 2018
Merged

Add support of the inheritance with generics. #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
23961d4
Add support of the inheritance with generics.
Oct 17, 2018
fd6f197
Fix remarks.
Oct 18, 2018
8687416
Write the discriminator of the child classes. Remove previously added…
Oct 18, 2018
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
7 changes: 4 additions & 3 deletions java8/src/main/java/com/dslplatform/json/processor/CompiledJsonAnnotationProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment
try {
JavaFileObject converterFile = processingEnv.getFiler().createSourceFile(classNamePath, structInfo.element);
try (Writer writer = converterFile.openWriter()) {
buildCode(writer, entry.getKey(), structInfo, structs, typeSupport, unknownTypes != UnknownTypes.ERROR);
buildCode(writer, entry.getKey(), structInfo, structs, typeSupport, unknownTypes != UnknownTypes.ERROR, processingEnv);
generatedFiles.put(classNamePath, structInfo);
originatingElements.add(structInfo.element);
} catch (IOException e) {
Expand Down Expand Up @@ -376,10 +376,11 @@ private static void buildCode(
final StructInfo si,
final Map<String, StructInfo> structs,
final TypeSupport typeSupport,
final boolean allowUnknown) throws IOException {
final boolean allowUnknown,
final ProcessingEnvironment processingEnv) throws IOException {
final Context context = new Context(code, InlinedConverters, Defaults, structs, typeSupport, allowUnknown);
final EnumTemplate enumTemplate = new EnumTemplate(context);
final ConverterTemplate converterTemplate = new ConverterTemplate(context, enumTemplate);
final ConverterTemplate converterTemplate = new ConverterTemplate(context, enumTemplate, processingEnv);

final String generateFullClassName = findConverterName(si);
final int dotIndex = generateFullClassName.lastIndexOf('.');
Expand Down
58 changes: 51 additions & 7 deletions java8/src/main/java/com/dslplatform/json/processor/ConverterTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,23 @@

import com.dslplatform.json.CompiledJson;

import javax.annotation.processing.ProcessingEnvironment;
import javax.lang.model.element.Element;
import javax.lang.model.element.ExecutableElement;
import javax.lang.model.element.TypeElement;
import javax.lang.model.element.TypeParameterElement;
import javax.lang.model.element.VariableElement;
import javax.lang.model.type.ArrayType;
import javax.lang.model.type.DeclaredType;
import javax.lang.model.type.TypeKind;
import javax.lang.model.type.TypeMirror;
import java.io.IOException;
import java.io.Writer;
import java.util.ArrayDeque;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Queue;

import static com.dslplatform.json.processor.CompiledJsonAnnotationProcessor.findConverterName;
import static com.dslplatform.json.processor.Context.nonGenericObject;
Expand All @@ -24,11 +30,13 @@ class ConverterTemplate {
private final Writer code;
private final Context context;
private final EnumTemplate enumTemplate;
private final ProcessingEnvironment processingEnv;
Copy link
Member

Choose a reason for hiding this comment

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

You are not using this anymore. Please clean it up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will do it.


ConverterTemplate(Context context, EnumTemplate enumTemplate) {
ConverterTemplate(Context context, EnumTemplate enumTemplate, ProcessingEnvironment processingEnv) {
this.code = context.code;
this.context = context;
this.enumTemplate = enumTemplate;
this.processingEnv = processingEnv;
}

private boolean isStaticEnum(AttributeInfo attr) {
Expand Down Expand Up @@ -112,7 +120,7 @@ private void asFormatConverter(final StructInfo si, final String name, final Str
for (AttributeInfo attr : si.attributes.values()) {
String typeName = attr.type.toString();
boolean hasConverter = context.inlinedConverters.containsKey(typeName);
StructInfo target = context.structs.get(attr.typeName);
StructInfo target = context.structs.get(attr.typeName);
if (attr.converter == null && (target == null || target.converter == null) && !hasConverter && !isStaticEnum(attr) && !attr.isJsonObject) {
List<String> types = attr.collectionContent(context.typeSupport, context.structs);
if (target != null && attr.isEnum(context.structs)) {
Expand All @@ -123,7 +131,11 @@ private void asFormatConverter(final StructInfo si, final String name, final Str
if (attr.isArray) {
content = ((ArrayType) attr.type).getComponentType().toString();
} else {
content = attr.typeName;
if (si.isParameterized) {
content = attr.typeName;
} else {
content = attr.genericType != null ? attr.genericType.toString() : "java.lang.Object";
}
}
} else {
content = types.get(0);
Expand Down Expand Up @@ -227,7 +239,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) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you pretty please fix your alignments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, of course.

type = typeForGeneric(attr.type, attr.typeVariablesIndex);
} else {
type = attr.genericType != null ? attr.genericType.toString() + ".class" : "java.lang.Object";
}
}

code.append("\t\t\tjava.lang.reflect.Type manifest_").append(attr.name).append(" = ").append(type).append(";\n");
Expand Down Expand Up @@ -304,7 +320,7 @@ private void buildGenericType(TypeMirror type, Map<String, Integer> typeVariable
void emptyObject(final StructInfo si, String className) throws IOException {
asFormatConverter(si, "ObjectFormatConverter", className, true);
List<AttributeInfo> sortedAttributes = sortedAttributes(si);
writeObject(className, sortedAttributes);
writeObject(si, className, sortedAttributes);
code.append("\t\tpublic ").append(className).append(" bind(final com.dslplatform.json.JsonReader reader, final ");
code.append(className).append(" instance) throws java.io.IOException {\n");
code.append("\t\t\tif (reader.last() != '{') throw new java.io.IOException(\"Expecting '{' \" + reader.positionDescription() + \". Found \" + (char) reader.last());\n");
Expand Down Expand Up @@ -388,7 +404,7 @@ void emptyObject(final StructInfo si, String className) throws IOException {
void fromObject(final StructInfo si, final String className) throws IOException {
asFormatConverter(si, "ObjectFormatConverter", className, false);
List<AttributeInfo> sortedAttributes = sortedAttributes(si);
writeObject(className, sortedAttributes);
writeObject(si, className, sortedAttributes);
code.append("\t\tpublic ").append(className).append(" read(final com.dslplatform.json.JsonReader reader) throws java.io.IOException {\n");
code.append("\t\t\tif (reader.wasNull()) return null;\n");
code.append("\t\t\telse if (reader.last() != '{') throw new java.io.IOException(\"Expecting '{' \" + reader.positionDescription() + \". Found \" + (char) reader.last());\n");
Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

String discriminator = "";
if (si.isParameterized || (!si.deserializeDiscriminator.isEmpty())) {
discriminator = si.deserializeDiscriminator.isEmpty() ? "$type" : si.deserializeDiscriminator;
} else {
boolean hasDiscriminator = false;
for (AttributeInfo attr : si.attributes.values()) {
if (attr.genericType != null) {
hasDiscriminator = true;
break;
}
}
if (hasDiscriminator) {
discriminator = "$type";
}
}
if ((!discriminator.isEmpty()) && (!si.attributes.containsKey(discriminator))) {
String deserializeName = si.deserializeName.isEmpty() ? si.binaryName.replaceAll("\\$", ".") : si.deserializeName;
code.append("\t\t\twriter.writeAscii(\"\\\"").append(discriminator).append("\\\":\\\"").append(deserializeName).append("\\\"");
if (!si.attributes.isEmpty()) {
code.append(",");
}
code.append("\");\n");
}
}

private void writeObject(final StructInfo si, final String className, List<AttributeInfo> sortedAttributes) throws IOException {
boolean isFirst = true;
for (AttributeInfo attr : sortedAttributes) {
String prefix = isFirst ? "" : ",";
Expand All @@ -446,6 +488,7 @@ private void writeObject(final String className, List<AttributeInfo> sortedAttri
code.append("\t\t}\n");
code.append("\t\tpublic void writeContentFull(final com.dslplatform.json.JsonWriter writer, final ");
code.append(className).append(" instance) {\n");
writeDiscriminator(si);
for (AttributeInfo attr : sortedAttributes) {
code.append("\t\t\twriter.writeAscii(quoted_").append(attr.name).append(");\n");
writeProperty(attr, false);
Expand All @@ -454,6 +497,7 @@ private void writeObject(final String className, List<AttributeInfo> sortedAttri
code.append("\t\tpublic boolean writeContentMinimal(final com.dslplatform.json.JsonWriter writer, final ");
code.append(className).append(" instance) {\n");
code.append("\t\t\tboolean hasWritten = false;\n");
writeDiscriminator(si);
for (AttributeInfo attr : sortedAttributes) {
String typeName = attr.type.toString();
String defaultValue = context.getDefault(typeName);
Expand Down
6 changes: 3 additions & 3 deletions library/src/main/java/com/dslplatform/json/DslJson.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ public Settings<TContext> resolveBinder(ConverterFactory<? extends JsonReader.Bi
public Settings<TContext> includeServiceLoader() {
return includeServiceLoader(Thread.currentThread().getContextClassLoader());
}

/**
* Load converters using provided `ClassLoader` instance
* Will scan through `META-INF/services/com.dslplatform.json.Configuration` file and register implementation during startup.
Expand Down Expand Up @@ -2781,8 +2781,8 @@ public boolean serialize(final JsonWriter writer, final Type manifest, @Nullable
* In most cases JSON is serialized into target `OutputStream`.
* This method will reuse thread local instance of `JsonWriter` and serialize JSON into it.
*
* @param value instance to serialize
* @param stream where to write resulting JSON
* @param value instance to serialize
* @param stream where to write resulting JSON
* @throws IOException error when unable to serialize instance
*/
public final void serialize(@Nullable final Object value, final OutputStream stream) throws IOException {
Expand Down
27 changes: 27 additions & 0 deletions library/src/main/java/com/dslplatform/json/processor/Analysis.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,11 +697,38 @@ public void findRelatedReferences() {
}
}
}
findGenericAttributes(info);
path.pop();
}
} while (total != structs.size());
}

private void findGenericAttributes(StructInfo si) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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?

Queue<TypeMirror> queue = new ArrayDeque<TypeMirror>(types.directSupertypes(si.element.asType()));
Map<String, TypeMirror> genericAttributes = new HashMap<String, TypeMirror>();
while (!queue.isEmpty()) {
TypeMirror mirror = queue.poll();
if (mirror instanceof DeclaredType) {
DeclaredType declaredType = (DeclaredType) mirror;
Element element = declaredType.asElement();
if (element instanceof TypeElement) {
List<? extends TypeMirror> typeArguments = declaredType.getTypeArguments();
List<? extends TypeParameterElement> typeParameters = ((TypeElement) element).getTypeParameters();
for (int i = 0; i < typeParameters.size(); i++) {
genericAttributes.put(typeParameters.get(i).toString(), typeArguments.get(i));
}
queue.addAll(types.directSupertypes(mirror));
}
}
}
for (AttributeInfo attr : si.attributes.values()) {
TypeMirror genericType = genericAttributes.get(attr.typeName);
if (genericType != null) {
attr.genericType = genericType;
}
}
}

public static String objectName(final String type) {
return "int".equals(type) ? "java.lang.Integer"
: "long".equals(type) ? "java.lang.Long"
Expand Down
1 change: 1 addition & 0 deletions library/src/main/java/com/dslplatform/json/processor/AttributeInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ public class AttributeInfo {
public final boolean isGeneric;
public final Map<String, Integer> typeVariablesIndex;
public final boolean containsStructOwnerType;
public TypeMirror genericType;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


public AttributeInfo(
String name,
Expand Down
Loading