Support for immutable and private fields/properties

In addition to better encapsulation and increased safety, this allows LoganSquare to support Kotlin data classes. For each fields, marked for parsing from JSON, there must be a matching constructor parameter in the corresponding position. The names of parameters are not validated, instead the number and types of parameters are relied on for safety. Each model uses either setter/field injection (old behavior) or constructor injection (new behavior); mixing setters and constructor parameters within same model is not supported. Calling `parseField` on JsonMappers of models with constructor injection produces UnsupportedOperationException. The impact on existing code should be minimal — the new parsing strategy gets used only when previous version of LoganSquare would produce an error: e.g. if there are final final or private fields without setters. See pull request for full description of feature
bluelinelabs · Jul 5, 2017 · 3f2f0b3 · 3f2f0b3
1 parent a063552
commit 3f2f0b3
Show file tree

Hide file tree

Showing 12 changed files with 490 additions and 52 deletions.
diff --git a/processor/src/main/java/com/bluelinelabs/logansquare/processor/JsonAnnotationProcessor.java b/processor/src/main/java/com/bluelinelabs/logansquare/processor/JsonAnnotationProcessor.java
@@ -1,6 +1,9 @@
 package com.bluelinelabs.logansquare.processor;
 
 import com.bluelinelabs.logansquare.processor.processor.Processor;
+import com.squareup.javapoet.ClassName;
+import com.squareup.javapoet.ParameterizedTypeName;
+import com.squareup.javapoet.TypeName;
 
 import java.io.IOException;
 import java.io.PrintWriter;
@@ -20,6 +23,7 @@
 import javax.lang.model.element.TypeElement;
 import javax.lang.model.util.Elements;
 import javax.lang.model.util.Types;
+import javax.tools.Diagnostic;
 import javax.tools.JavaFileObject;
 
 import static javax.tools.Diagnostic.Kind.ERROR;
@@ -70,6 +74,13 @@ public boolean process(Set<? extends TypeElement> elements, RoundEnvironment env
                 if (!jsonObjectHolder.fileCreated) {
                     jsonObjectHolder.fileCreated = true;
 
+                    if (jsonObjectHolder.hasParentClass() && hasUnsupportedInheritance(jsonObjectHolder)) {
+                        // The interactions between models with constructor injection and
+                        // and their subclasses can get really weird. For now let's simply
+                        // disallow inheritance from them
+                        continue;
+                    }
+
                     try {
                         JavaFileObject jfo = mFiler.createSourceFile(fqcn);
                         Writer writer = jfo.openWriter();
@@ -91,6 +102,34 @@ public boolean process(Set<? extends TypeElement> elements, RoundEnvironment env
         }
     }
 
+    private boolean hasUnsupportedInheritance(JsonObjectHolder objectHolder) {
+        TypeName parentType = objectHolder.parentTypeName;
+
+        ClassName parentClass = parentType instanceof ClassName
+                        ? (ClassName) parentType
+                        : ((ParameterizedTypeName) parentType).rawType;
+
+        JsonObjectHolder parentHolder =
+                mJsonObjectMap.get(TypeUtils.getInjectedFQCN(parentClass));
+
+        if (parentHolder == null || !parentHolder.needConstructorInjection) {
+            return false;
+        }
+
+        TypeName badType = objectHolder.objectTypeName;
+
+        TypeName perpetratorType = badType instanceof ClassName
+                ? (ClassName) badType
+                : ((ParameterizedTypeName) badType).rawType;
+
+        TypeElement badTypeElement = mElementUtils.getTypeElement(perpetratorType.toString());
+
+        processingEnv.getMessager().printMessage(Diagnostic.Kind.ERROR,
+                "Subclassing from models with constructor injection is not supported", badTypeElement);
+
+        return true;
+    }
+
     private void error(String message, Object... args) {
         processingEnv.getMessager().printMessage(ERROR, String.format(message, args));
     }

diff --git a/processor/src/main/java/com/bluelinelabs/logansquare/processor/JsonFieldHolder.java b/processor/src/main/java/com/bluelinelabs/logansquare/processor/JsonFieldHolder.java
@@ -6,26 +6,37 @@
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Set;
 
 import javax.lang.model.element.Element;
 import javax.lang.model.element.ExecutableElement;
+import javax.lang.model.element.Modifier;
 import javax.lang.model.element.TypeElement;
 import javax.lang.model.type.TypeKind;
 import javax.lang.model.type.TypeMirror;
 import javax.lang.model.util.ElementFilter;
 import javax.lang.model.util.Elements;
 import javax.lang.model.util.Types;
 
+import static javax.lang.model.element.Modifier.FINAL;
+import static javax.lang.model.element.Modifier.PRIVATE;
+
 public class JsonFieldHolder {
 
     public String[] fieldName;
     public String setterMethod;
     public String getterMethod;
     public boolean shouldParse;
     public boolean shouldSerialize;
+    public boolean needConstructorArg;
     public Type type;
 
     public String fill(Element element, Elements elements, Types types, String[] fieldNames, TypeMirror typeConverterType, JsonObjectHolder objectHolder, boolean shouldParse, boolean shouldSerialize) {
+        if (shouldParse && hasNoMatchingSetter(element, elements)) {
+            this.needConstructorArg = true;
+            objectHolder.needConstructorInjection = true;
+        }
+
         if (fieldNames == null || fieldNames.length == 0) {
             String defaultFieldName = element.getSimpleName().toString();
 
@@ -49,6 +60,12 @@ public String fill(Element element, Elements elements, Types types, String[] fie
         return ensureValidType(type, element);
     }
 
+    private static boolean hasNoMatchingSetter(Element element, Elements elements) {
+        final Set<Modifier> modifiers = element.getModifiers();
+
+        return modifiers.contains(FINAL) || modifiers.contains(PRIVATE) && TextUtils.isEmpty(JsonFieldHolder.getSetter(element, elements));
+    }
+
     private String ensureValidType(Type type, Element element) {
         if (type == null) {
             return "Type could not be determined for " + element.toString();

diff --git a/processor/src/main/java/com/bluelinelabs/logansquare/processor/JsonObjectHolder.java b/processor/src/main/java/com/bluelinelabs/logansquare/processor/JsonObjectHolder.java
@@ -31,9 +31,12 @@ public class JsonObjectHolder {
     public String onCompleteCallback;
     public String preSerializeCallback;
 
+    public boolean needConstructorInjection;
+
     // Using a TreeMap now to keep the entries sorted. This ensures that code is
     // always written the exact same way, no matter which JDK you're using.
     public final Map<String, JsonFieldHolder> fieldMap = new TreeMap<>();
+
     public boolean fileCreated;
 
     public boolean hasParentClass() {

diff --git a/processor/src/main/java/com/bluelinelabs/logansquare/processor/ObjectMapperInjector.java b/processor/src/main/java/com/bluelinelabs/logansquare/processor/ObjectMapperInjector.java
@@ -12,15 +12,7 @@
 import com.fasterxml.jackson.core.JsonGenerator;
 import com.fasterxml.jackson.core.JsonParser;
 import com.fasterxml.jackson.core.JsonToken;
-import com.squareup.javapoet.AnnotationSpec;
-import com.squareup.javapoet.ClassName;
-import com.squareup.javapoet.FieldSpec;
-import com.squareup.javapoet.JavaFile;
-import com.squareup.javapoet.MethodSpec;
-import com.squareup.javapoet.ParameterizedTypeName;
-import com.squareup.javapoet.TypeName;
-import com.squareup.javapoet.TypeSpec;
-import com.squareup.javapoet.TypeVariableName;
+import com.squareup.javapoet.*;
 
 import java.io.IOException;
 import java.util.ArrayList;
@@ -177,7 +169,77 @@ private MethodSpec getParseMethod() {
                 .addParameter(JsonParser.class, JSON_PARSER_VARIABLE_NAME)
                 .addException(IOException.class);
 
-        if (!mJsonObjectHolder.isAbstractClass) {
+        if (mJsonObjectHolder.isAbstractClass) {
+            builder.addStatement("return null");
+
+            return builder.build();
+        }
+
+        CodeBlock.Builder onCompleteCall = CodeBlock.builder();
+
+        if (!TextUtils.isEmpty(mJsonObjectHolder.onCompleteCallback)) {
+            onCompleteCall.addStatement("instance.$L()", mJsonObjectHolder.onCompleteCallback);
+        }
+
+        if (mJsonObjectHolder.needConstructorInjection) {
+            for (Map.Entry<String, JsonFieldHolder> field : mJsonObjectHolder.fieldMap.entrySet()) {
+                JsonFieldHolder fieldInfo = field.getValue();
+
+                if (!fieldInfo.shouldParse) {
+                    continue;
+                }
+
+                TypeName fieldType = fieldInfo.type.getTypeName();
+
+                if (fieldType.isPrimitive()) {
+                    builder.addStatement("$T $N = null", fieldType.box(), field.getKey());
+                } else {
+                    builder.addStatement("$T $N = null", fieldType, field.getKey());
+                }
+            }
+
+            builder.beginControlFlow("if ($L.getCurrentToken() == null)", JSON_PARSER_VARIABLE_NAME)
+                    .addStatement("$L.nextToken()", JSON_PARSER_VARIABLE_NAME)
+                    .endControlFlow()
+                    .beginControlFlow("if ($L.getCurrentToken() != $T.START_OBJECT)", JSON_PARSER_VARIABLE_NAME, JsonToken.class)
+                    .addStatement("$L.skipChildren()", JSON_PARSER_VARIABLE_NAME)
+                    .addStatement("return null")
+                    .endControlFlow()
+                    .beginControlFlow("while ($L.nextToken() != $T.END_OBJECT)", JSON_PARSER_VARIABLE_NAME, JsonToken.class)
+                    .addStatement("String fieldName = $L.getCurrentName()", JSON_PARSER_VARIABLE_NAME)
+                    .addStatement("$L.nextToken()", JSON_PARSER_VARIABLE_NAME);
+
+            int entryCount = 0;
+
+            final StringBuilder argsLiteral = new StringBuilder();
+
+            for (Map.Entry<String, JsonFieldHolder> field : mJsonObjectHolder.fieldMap.entrySet()) {
+
+                JsonFieldHolder fieldInfo = field.getValue();
+
+                if (fieldInfo.shouldParse) {
+                    if (entryCount == 0) {
+                        argsLiteral.append(field.getKey());
+                    } else {
+                        argsLiteral.append(", ").append(field.getKey());
+                    }
+
+                    emitFieldSetter(builder, fieldInfo, entryCount == 0, "$L = $L", field.getKey());
+
+                    ++entryCount;
+                }
+            }
+
+            if (entryCount > 0) {
+                builder.endControlFlow();
+            }
+
+            builder.addStatement("$L.skipChildren()", JSON_PARSER_VARIABLE_NAME)
+                    .endControlFlow();
+
+            builder.addCode(onCompleteCall.build())
+                    .addStatement("return new $T($L)", mJsonObjectHolder.objectTypeName, argsLiteral);
+        } else {
             builder.addStatement("$T instance = new $T()", mJsonObjectHolder.objectTypeName, mJsonObjectHolder.objectTypeName)
                     .beginControlFlow("if ($L.getCurrentToken() == null)", JSON_PARSER_VARIABLE_NAME)
                     .addStatement("$L.nextToken()", JSON_PARSER_VARIABLE_NAME)
@@ -193,13 +255,8 @@ private MethodSpec getParseMethod() {
                     .addStatement("$L.skipChildren()", JSON_PARSER_VARIABLE_NAME)
                     .endControlFlow();
 
-            if (!TextUtils.isEmpty(mJsonObjectHolder.onCompleteCallback)) {
-                builder.addStatement("instance.$L()", mJsonObjectHolder.onCompleteCallback);
-            }
-
-            builder.addStatement("return instance");
-        } else {
-            builder.addStatement("return null");
+            builder.addCode(onCompleteCall.build())
+                    .addStatement("return instance");
         }
 
         return builder.build();
@@ -214,6 +271,11 @@ private MethodSpec getParseFieldMethod() {
                 .addParameter(JsonParser.class, JSON_PARSER_VARIABLE_NAME)
                 .addException(IOException.class);
 
+        if (mJsonObjectHolder.needConstructorInjection) {
+            builder.addStatement("throw new $T($S)", UnsupportedOperationException.class, "The class is using constructor injection, individual fields can not be parsed");
+            return builder.build();
+        }
+
         int parseFieldLines = addParseFieldLines(builder);
 
         if (mJsonObjectHolder.hasParentClass()) {
@@ -288,38 +350,10 @@ private int addParseFieldLines(MethodSpec.Builder builder) {
             JsonFieldHolder fieldHolder = entry.getValue();
 
             if (fieldHolder.shouldParse) {
-                List<Object> args = new ArrayList<>();
-                StringBuilder ifStatement = new StringBuilder();
-                boolean isFirst = true;
-                for (String fieldName : fieldHolder.fieldName) {
-                    if (isFirst) {
-                        isFirst = false;
-                    } else {
-                        ifStatement.append(" || ");
-                    }
-                    ifStatement.append("$S.equals(fieldName)");
-                    args.add(fieldName);
-                }
-
-                if (entryCount == 0) {
-                    builder.beginControlFlow("if (" + ifStatement.toString() + ")", args.toArray(new Object[args.size()]));
-                } else {
-                    builder.nextControlFlow("else if (" + ifStatement.toString() + ")", args.toArray(new Object[args.size()]));
-                }
-
-                String setter;
-                Object[] stringFormatArgs;
                 if (fieldHolder.hasSetter()) {
-                    setter = "instance.$L($L)";
-                    stringFormatArgs = new Object[] { fieldHolder.setterMethod };
+                    emitFieldSetter(builder, fieldHolder, entryCount == 0, "instance.$L($L)", fieldHolder.setterMethod);
                 } else {
-                    setter = "instance.$L = $L";
-                    stringFormatArgs = new Object[] { entry.getKey() };
-                }
-
-                if (fieldHolder.type != null) {
-                    setFieldHolderJsonMapperVariableName(fieldHolder.type);
-                    fieldHolder.type.parse(builder, 1, setter, stringFormatArgs);
+                    emitFieldSetter(builder, fieldHolder, entryCount == 0, "instance.$L = $L", entry.getKey());
                 }
 
                 entryCount++;
@@ -328,6 +362,36 @@ private int addParseFieldLines(MethodSpec.Builder builder) {
         return entryCount;
     }
 
+    private void emitFieldSetter(MethodSpec.Builder builder,
+                                 JsonFieldHolder fieldHolder,
+                                 boolean first,
+                                 String setter,
+                                 Object... stringFormatArgs) {
+        List<Object> args = new ArrayList<>();
+        StringBuilder ifStatement = new StringBuilder();
+        boolean isFirst = true;
+        for (String fieldName : fieldHolder.fieldName) {
+            if (isFirst) {
+                isFirst = false;
+            } else {
+                ifStatement.append(" || ");
+            }
+            ifStatement.append("$S.equals(fieldName)");
+            args.add(fieldName);
+        }
+
+        if (first) {
+            builder.beginControlFlow("if (" + ifStatement.toString() + ")", args.toArray(new Object[args.size()]));
+        } else {
+            builder.nextControlFlow("else if (" + ifStatement.toString() + ")", args.toArray(new Object[args.size()]));
+        }
+
+        if (fieldHolder.type != null) {
+            setFieldHolderJsonMapperVariableName(fieldHolder.type);
+            fieldHolder.type.parse(builder, 1, setter, stringFormatArgs);
+        }
+    }
+
     private void addUsedJsonMapperVariables(TypeSpec.Builder builder) {
         Set<ClassNameObjectMapper> usedJsonObjectMappers = new HashSet<>();
 

diff --git a/...or/src/main/java/com/bluelinelabs/logansquare/processor/processor/JsonFieldProcessor.java b/...or/src/main/java/com/bluelinelabs/logansquare/processor/processor/JsonFieldProcessor.java
@@ -63,11 +63,14 @@ private void processJsonFieldAnnotation(Element element, Map<String, JsonObjectH
         TypeElement enclosingElement = (TypeElement)element.getEnclosingElement();
 
         JsonObjectHolder objectHolder = jsonObjectMap.get(TypeUtils.getInjectedFQCN(enclosingElement, elements));
-        JsonFieldHolder fieldHolder = objectHolder.fieldMap.get(element.getSimpleName().toString());
+
+        String propertyName = element.getSimpleName().toString();
+
+        JsonFieldHolder fieldHolder = objectHolder.fieldMap.get(propertyName);
 
         if (fieldHolder == null) {
             fieldHolder = new JsonFieldHolder();
-            objectHolder.fieldMap.put(element.getSimpleName().toString(), fieldHolder);
+            objectHolder.fieldMap.put(propertyName, fieldHolder);
         }
 
         JsonField annotation = element.getAnnotation(JsonField.class);
@@ -102,8 +105,8 @@ private boolean isJsonFieldFieldAnnotationValid(Element element, Elements elemen
             return false;
         }
 
-        if (element.getModifiers().contains(PRIVATE) && (TextUtils.isEmpty(JsonFieldHolder.getGetter(element, elements)) || TextUtils.isEmpty(JsonFieldHolder.getSetter(element, elements)))) {
-            error(element, "@%s annotation can only be used on private fields if both getter and setter are present.", JsonField.class.getSimpleName());
+        if (element.getModifiers().contains(PRIVATE) && (TextUtils.isEmpty(JsonFieldHolder.getGetter(element, elements)))) {
+            error(element, "@%s annotation can only be used on private fields if a getter is present.", JsonField.class.getSimpleName());
             return false;
         }
 

diff --git a/processor/src/test/java/com/bluelinelabs/logansquare/processor/ConstructorInjectionTest.java b/processor/src/test/java/com/bluelinelabs/logansquare/processor/ConstructorInjectionTest.java
@@ -0,0 +1,29 @@
+package com.bluelinelabs.logansquare.processor;
+
+import com.google.testing.compile.JavaFileObjects;
+import org.junit.Test;
+
+import static com.google.common.truth.Truth.ASSERT;
+import static com.google.testing.compile.JavaSourceSubjectFactory.javaSource;
+
+public class ConstructorInjectionTest {
+    @Test
+    public void testPrivateFields() {
+        ASSERT.about(javaSource())
+                .that(JavaFileObjects.forResource("model/good/PrivateFieldsWithoutSettersModel.java"))
+                .processedWith(new JsonAnnotationProcessor())
+                .compilesWithoutError()
+                .and()
+                .generatesSources(JavaFileObjects.forResource("generated/PrivateFieldsWithoutSettersModel$$JsonObjectMapper.java"));
+    }
+
+    @Test
+    public void testImmutableFields() {
+        ASSERT.about(javaSource())
+                .that(JavaFileObjects.forResource("model/good/ImmutableFieldsModel.java"))
+                .processedWith(new JsonAnnotationProcessor())
+                .compilesWithoutError()
+                .and()
+                .generatesSources(JavaFileObjects.forResource("generated/ImmutableFieldsModel$$JsonObjectMapper.java"));
+    }
+}