Add support for forcing Moshi to construct instances using a specified constructor #266

cbruegg · 2017-03-06T20:31:33Z

This PR implements a method to force Moshi to construct instances of specific types using a constructor annotation. The main use case is Kotlin, but I imagine this could apply to Java code as well. Consider the following example:

data class Foo(val bar: Bar) {
  @delegate:Transient val fooBar by lazy { doSomethingWith(bar) }
}

Currently, Moshi supports deserializing a JSON into this type by unsafely creating the object, bypassing the constructor. A workaround is the Kotlin no-arg plugin introduced in Kotlin 1.0.6 which generates an empty constructor for annotated classes, but unfortunately both ways are problematic, as they don't initialize the hidden delegate instance for the lazy property.

This problem is solved by the PR. The following modification is required in the code:

data class Foo @JsonConstructor constructor(val bar: Bar) {
  @delegate:Transient val fooBar by lazy { doSomethingWith(bar) }
}

This change will tell Moshi to use the primary constructor to construct the object. The way Moshi generates values for the parameters of the constructor after this PR is pretty straight-forward:

Primitive types and their associated reference types get the default value, such as 0 for int and Integer, 0.0 for double and Double and so on.
Strings get "".
Interfaces get a proxy instance that simply throws on method calls.
Enums get the first value in the values field.
Other types are recursively constructed using ClassFactory.get(...).newInstance().

To reduce cost, ClassFactory instances are cached. The reason why parameters of reference types aren't simply set to null is that constructors may check for parameters to be non-null. Kotlin does this by default.

Afterwards, Moshi proceeds as normal by deserializing the JSON values into the appropriate fields of the newly created instance. However, since the real constructor was called, the delegate is initialized correctly.

NightlyNexus

very non-comprehensive pass.

Thanks for the PR and description!

This is very interesting.
Different default values for String, interfaces, and Enums is rather surprising behavior.
Definitely needs tests, but I'm hesitant to review/ask for changes because I'm not sure this is something Moshi will support in this way.

To clarify, the goal here is to support Kotlin's lazy, right?

NightlyNexus · 2017-03-06T21:18:33Z

moshi/src/main/java/com/squareup/moshi/ClassFactory.java

+                  args[j] = Proxy.newProxyInstance(getClass().getClassLoader(), new Class[]{parameterType}, new InvocationHandler() {
+                    @Override
+                    public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
+                      throw new IllegalAccessException("Trying to call a method on a " +


UnsupportedOperationException, perhaps?
Needs tests, too.

NightlyNexus · 2017-03-06T21:18:36Z

moshi/src/main/java/com/squareup/moshi/ClassFactory.java

-        @Override public String toString() {
+
+        @Override
+        public String toString() {


nit: Revert style change.

NightlyNexus · 2017-03-06T21:18:40Z

moshi/src/main/java/com/squareup/moshi/ClassFactory.java

+      return (ClassFactory<T>) unsafeFactories.get(rawType);
+    }
+
+    // Try to find a constructor with the JsonConstructor annotation


nit: full stop period in comments.

NightlyNexus · 2017-03-06T21:18:45Z

moshi/src/main/java/com/squareup/moshi/ClassFactory.java

+   * This can't be a {@link LinkedHashTreeMap} since {@link Class}
+   * does not implement {@link Comparable}.
+   */
+  private static Map<Class<?>, ClassFactory<?>> unsafeFactories = new HashMap<>();


LinkedHashMap.
This should probably be cached somewhere more appropriate, like the Moshi instance.
But, even moreover, I don't think it actually needs to be cached. The Factory is supposed to do work to create the Adapter; the created Adapter is still fast. This is also an independent change. Revert?

The adapter may not always be fast without caching due to the recursive fallback call to ClassFactory.get(parameterType).newInstance(). It's your call on this though, I don't really care too much about the caching since many real-world entities consist of primitives and Strings only anyway.

NightlyNexus · 2017-03-06T21:20:55Z

moshi/src/main/java/com/squareup/moshi/ClassFactory.java

+            T newInstance() throws InvocationTargetException, IllegalAccessException, InstantiationException {
+              Class<?>[] parameterTypes = constructor.getParameterTypes();
+              Object[] args = new Object[parameterTypes.length];
+              for (int j = 0; j < parameterTypes.length; j++) {


nit: conventional i instead of j?

NightlyNexus · 2017-03-06T21:34:18Z

moshi/src/main/java/com/squareup/moshi/ClassFactory.java

-        @Override public String toString() {
+
+        @Override
+        public String toString() {


nit: Revert style changes.

NightlyNexus · 2017-03-06T21:39:47Z

moshi/src/main/java/com/squareup/moshi/ClassFactory.java

-import java.lang.reflect.Field;
-import java.lang.reflect.InvocationTargetException;
-import java.lang.reflect.Method;
+import java.lang.reflect.*;


Qualify imports.

cbruegg · 2017-03-06T21:59:49Z

Thanks for your quick feedback!

Different default values for String, interfaces, and Enums is rather surprising behavior.

With proper documentation, I don't see that as an issue. It's implied by the fact that only non-null values will be passed and of course String ∩ All Interfaces ∩ All Enums = {null}.

Definitely needs tests, but I'm hesitant to review/ask for changes because I'm not sure this is something Moshi will support in this way.

I've already added a simple test, but if you want more tests, sure, I can do that. If you're very concerned about integrating this into the library itself, there'd an alternative: The ClassFactory could be pluggable, for example by making the Moshi class contain a field for custom ClassFactory instances that are preferred over ClassFactory.get(...). The code from this PR could then be refactored to be a separate library.
I'd prefer to have this in the Moshi library itself though, since I'm a heavy Kotlin user and I expect the Kotlin userbase to grow more.

To clarify, the goal here is to support Kotlin's lazy, right?

That and other property delegates.

JakeWharton

Missing test cases with multiple constructors where one and both are annotated.

JakeWharton · 2017-03-06T21:56:50Z

moshi/src/main/java/com/squareup/moshi/ClassFactory.java

+    }
+
+    // Try to find a constructor with the JsonConstructor annotation
+    for (int i = 0; i < rawType.getDeclaredConstructors().length; i++) {


Cache this call outside the loop or use for-each

JakeWharton · 2017-03-06T21:57:12Z

moshi/src/main/java/com/squareup/moshi/ClassFactory.java

+                } else if (parameterType == boolean.class || parameterType == Boolean.class) {
+                  args[j] = false;
+                } else if (parameterType == String.class) {
+                  args[j] = "";


Should be null

JakeWharton · 2017-03-06T21:57:44Z

moshi/src/main/java/com/squareup/moshi/ClassFactory.java

+                    }
+                  });
+                } else if (parameterType.isEnum() && parameterType.getEnumConstants().length > 0) {
+                  args[j] = parameterType.getEnumConstants()[0];


Where is this coming from? Reference types default to null

JakeWharton · 2017-03-06T21:58:23Z

moshi/src/main/java/com/squareup/moshi/ClassFactory.java

+                Class<?> parameterType = parameterTypes[j];
+                // Default values according to
+                // https://docs.oracle.com/javase/tutorial/java/nutsandbolts/datatypes.html
+                if (parameterType == byte.class || parameterType == Byte.class) {


Boxed primitives should all default to null, not the primitive default

From the PR description:

The reason why parameters of reference types aren't simply set to null is that constructors may check for parameters to be non-null. Kotlin does this by default.

There's tons of invariants constructors check. Maybe only one of two properties can be non-null or 0 is not in the valid range. This violates least-surprise and makes the objects serialize differently than what was actually deserialized. We cannot do this.

Good point. Perhaps my suggestion to make the ClassFactory pluggable may be a better and more flexible option?

Alternatively: Change this PR to use null for reference types, but still make the ClassFactory pluggable. The best of both worlds?

One final alternative: Null by default for reference types and non-null by setting a parameter of the JsonConstructor annotation to True.

JakeWharton · 2017-03-06T21:59:08Z

moshi/src/main/java/com/squareup/moshi/ClassFactory.java

+
+  private static <T> ClassFactory<T> createAnnotationClassFactory(final Class<?> rawType, final Constructor<?> constructor) {
+    return new ClassFactory<T>() {
+            @SuppressWarnings("unchecked")


Nit: too much indent

JakeWharton · 2017-03-06T21:59:55Z

moshi/src/main/java/com/squareup/moshi/ClassFactory.java

  abstract T newInstance() throws
      InvocationTargetException, IllegalAccessException, InstantiationException;

  public static <T> ClassFactory<T> get(final Class<?> rawType) {
+    if (unsafeFactories.containsKey(rawType)) {


Just call get and check against null so you don't do hash and lookup steps twice.

JakeWharton · 2017-03-06T22:02:48Z

moshi/src/test/java/com/squareup/moshi/JsonConstructorTest.java

+
+  @Test
+  public void testJsonConstructorAnnotation() {
+    try {


Delete try/catches. An exception always fails the test case.

…ructor.

swankjesse

I’m not sure we want this.

How’d you feel about adding a no-args constructor that calls this() with default values? That’s so simple.

swankjesse · 2017-03-07T02:27:56Z

moshi/src/main/java/com/squareup/moshi/ClassFactory.java

+      }
+    }
+    if (annotationFactory != null) {
+      unsafeFactories.put(rawType, annotationFactory);


ick, this is particularly unsafe . . . static value without synchronization

swankjesse · 2017-03-07T02:28:15Z

moshi/src/main/java/com/squareup/moshi/ClassFactory.java

    } catch (Exception ignored) {
    }

    throw new IllegalArgumentException("cannot construct instances of " + rawType.getName());
  }
+
+  private static <T> ClassFactory<T> createAnnotationClassFactory(final Class<?> rawType, final Constructor<?> constructor) {


I’m not very comfortable with this.

cbruegg · 2017-03-07T08:43:18Z

How’d you feel about adding a no-args constructor that calls this() with default values? That’s so simple.

With many entities that have lots of properties, this would take more effort and in addition, I don't like the idea since it would lead to a lot of repetition. I understand you're concerned about the non-standard values passed to the annotated constructor and I've already suggested a solution:

Make the ClassFactory pluggable, perhaps by introducing a new property Map<Class<?>, ClassFactory<?>> userFactories in the Moshi instance. If for a class C userFactories.get(C) != null, Moshi will use that factory. Otherwise, it will call ClassFactory.get(C) as before.
This way I could implement this change on my own, without placing the necessary code inside the Moshi library itself.
An alternative, but based on my PR: Set reference types to null, but still make the ClassFactory pluggable as proposed in (1.) so I can implement this on the user-side, while the safer @JsonConstructor annotation may still be beneficial.
Another alternative based on my PR: Introduce a flag into the JsonConstructor annotation that specifies whether non-null values should be generated, which defaults to false. If it's true, parameter values are generated as in this version of the PR. If it's false, reference types are set to null.

I'd like avoid forking Moshi permanently to be able to use it with Kotlin, hence it would be great if we could compromise on one of the three alternatives. Personally, I think (1.) would be a safe and the best option to let users of the library customize the way instances are created. Currently it will fallback to sun.misc.Unsafe if no constructor is found, thus bypassing real constructors and that is a lot unsafer than having an option for customization, since the former leads the unproperly initialized objects, which is obviously unexpected.

swankjesse · 2017-03-07T11:44:45Z

I'm trying to limit how clever Moshi is. What about adding default parameters to your constructor? Would that make a no-args constructor less verbose?

cbruegg · 2017-03-07T11:59:29Z

What about adding default parameters to your constructor?

That's not a good option if I need actual default values for some parameters. Adding default values to other parameters just to generate a no-args constructor would make it really hard to distinguish between real default values and ones that are only there to generate a no-args constructor.

Would option (1.) really make Moshi more clever? The code changes would be minimal and all of the logic of this PR would be moved to the user-side. And if the user doesn't register any ClassFactory instances, behavior doesn't change at all. Users would have to actively register factories to make Moshi more clever, but the cleverness would come from their own code.

swankjesse · 2017-03-07T13:49:52Z

Why wouldn't your actual default parameters be sufficient? Like, those are the parameters it can use, it doesn't need to be this 0 stuff.

cbruegg · 2017-03-07T14:57:24Z

@swankjesse Consider the following example:

data class Purchase(val amount: Int = 1, val price: Int = -1, val customer: Customer = ..., val item: Item = ...)

The default value for amount may make sense, because in our scenario customers don't purchase an item more than once usually .
The default value for price makes no sense and is only there to generate a no-args constructor. I don't think it's immediately obvious to a reader though, one may think that price == -1 simply means that there is no price, the item was free or that it's a special value for something. If I was new to a project, this would leave me confused.
Default values for customer and item are hard. There are no sensible defaults and constructing a default customer for example perhaps isn't even possible or very cumbersome since it could be a framework class or anything else the developer of Purchase has no control over. With this PR, the ClassFactory would just go ahead and unsafely instantiate a Customer, eventually replacing it with a valid value from the JSON after instantiation of course.

I understand you wouldn't want the latter behavior in the library, but I hope I've made clear why letting users provide ClassFactory instances would be very useful.

What potential harm do you see in letting users do that?

JakeWharton · 2017-03-07T17:44:16Z

I too feel like the defaults in the constructor should be sufficient. If you can't rely on those then it sounds like you have a modeling problem between your network and application domain and perhaps shouldn't be sharing the same object between them.

…

On Tue, Mar 7, 2017, 9:57 AM Christian Brüggemann ***@***.***> wrote: @swankjesse <https://github.com/swankjesse> Consider the following example: data class Purchase(val amount: Int = 1, val price: Int = -1, val customer: Customer = ..., val item: Item = ...) - The default value for amount may make sense, because in our scenario customers don't purchase an item more than once usually . - The default value for price makes no sense and is only there to generate a no-args constructor. I don't think it's immediately obvious to a reader though, one may think that price == -1 simply means that there is no price, the item was free or that it's a special value for something. If I was new to a project, this would leave me confused. - Default values for customer and item are hard. There are no sensible defaults and constructing a *default* customer for example perhaps isn't even possible or very cumbersome since it could be a framework class or anything else the developer of Purchase has no control over. With this PR, the ClassFactory would just go ahead and unsafely instantiate a Customer, eventually replacing it with a valid value from the JSON after instantiation of course. I understand you wouldn't want the latter behavior in the library, but I hope I've made clear why letting users provide ClassFactory instances would be very useful. What potential harm do you see in letting users do that? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#266 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEEEfJncn6BmaYYvwhQmT8fjMx22rRGks5rjXBUgaJpZM4MUqAZ> .

cbruegg · 2017-03-07T18:05:33Z

So I should duplicate my entity classes and give the duplicate class a constructor with nonsensical defaults, probably marked as @Deprecated to warn anyone trying to call it, just so Moshi can initialize them properly? I know that Moshi will correctly deserialize a JSON into an object initialized using nonsensical default values, but software developers make mistakes.

In my opinion, it's a huge issue that many serialization frameworks don't allow users to specificy how an object should be created and instead rely on sun.misc.Unsafe and alike. Making the ClassFactory pluggable would fix this issue in an easy way and since using this feature would be absolutely optional, it wouldn't get in the way of or make using Moshi more complicated for anyone.

cbruegg · 2017-03-07T18:15:08Z

By the way, Gson supports this very feature, but I prefer Moshi as it's based on Okio, prevents me from shooting myself in the foot by simply serializing everything I throw at it, including classes without a registered type adapter and because of other features..

swankjesse · 2017-03-07T20:23:03Z

I still don’t understand why a no-arguments constructor is bad. That way the “nonsensical defaults” live in your code rather than in Moshi.

cbruegg · 2017-03-07T20:44:43Z

I still don’t understand why a no-arguments constructor is bad.

It's bad because it can be called from regular code and because it's boilerplate. Everytime a property is added, removed or changed, the no-args constructor that passes default values to the primary constructor needs to be updated to. If I instead specify defaults using the syntax data class X(val x: Int = 1), the IDE will let me know there are default values for the parameters I'd otherwise need to provide when I write code that invokes the primary constructor and this can be irritating, since semantically the default values don't make sense and only exist for Moshi.

That way the “nonsensical defaults” live in your code rather than in Moshi.

With a pluggable ClassFactory the nonsensical defaults wouldn't live in Moshi either. They'd still live in my code, but only once and not for each entity. I'm talking about something along the lines of:

class Moshi {
  Map<Class<?>, ClassFactory<?>> factories = ...;
  <T> void registerClassFactory(Class<T> clazz, ClassFactory<T> factory) {
    factories.put(clazz, factory);
  }
}

class ClassJsonAdapter<T> extends JsonAdapter<T> {
  private final Moshi moshi;
  ...
  public T fromJson(JsonReader reader) {
    T result;
    if (moshi.factories.containsKey(T.class)) result = moshi.factories.get(T.class).newInstance(); // new
    else result = classFactory.newInstance(); // As before
    ... // Parse data into result as before
  }
}

Obviously not exactly like this, but you get the idea. In my code, I could then call moshi.registerClassFactory(...) with the factory doing what this PR would otherwise have done (calling an annotated constructor with non-null values created out of thin air).

cbruegg · 2017-03-09T17:38:28Z

Since I'm going to use this anyway, I've implemented the (small) necessary changes to support user-supplied ClassFactorys. Let me know if you're comfortable with them, since I'd really like to see them in the mainline Moshi version. Otherwise this discussion can be closed.

cbruegg · 2017-03-20T08:07:28Z

Any news on this?

swankjesse · 2017-03-20T10:40:48Z

Still contemplating options on this. At the moment burying defaults in Moshi feels unreasonable.

Probably the right option for data classes is to use Kotlin’s reflection and actually call the constructor rather than assigning fields. I’m not sure if that’s symmetric, but it seems like a stronger design.

cbruegg · 2017-03-20T11:28:18Z

Thanks for responding so quickly!

Probably the right option for data classes is to use Kotlin’s reflection and actually call the constructor rather than assigning fields. I’m not sure if that’s symmetric, but it seems like a stronger design.

That definitely sounds like a better approach. Here's how it could be done:

Alternative 1

Let constructor parameters be annotated with @Json. As a plus, this would also work for Java classes. A drawback would be having to annotate fields/properties and constructor parameters to support serialization instead of only deserialization.

Kotlin example with support for deserialization only:
data class Foo(@param:Json(name = "bar") bar: String)

Kotlin example with support for deserialization and serialization:
data class Foo(@param:Json(name = "bar") @field:Json(name = "bar") bar: String)

Java example with support for deserialization only:

class Foo {
  String bar;
  Foo(@Json(name = "bar") String bar) {
    this.bar = bar;
 }
}

Java example with support for deserialization and serialization:

class Foo {
  @Json(name = "bar") String bar;
  Foo(@Json(name = "bar") String bar) {
    this.bar = bar;
 }
}

Alternative 2

Like you proposed, Kotlin reflection could be used to detect annotations like this: @property:Json(name = "bar"). Moshi would then know that the constructor parameter name equals the property name and could call the constructor for deserialization and use the field value for serialization. Discovering these annotations would require a dependency on the heavy kotlin-reflect library.

Alternative 3 (I think this would be the easiest to use)

Call the constructor for deserialization iff it is annotated with @JsonConstructor, indicating that the library user would like Moshi to call it. Require that constructor parameters are named the same as the fields they are assigned to.

Kotlin example with support for deserialization and serialization:
data class Foo @JsonConstructor constructor(@Json(name = "bar") bar: String)
which due to the currently allowed targets of the @Json annotation equals
data class Foo @JsonConstructor constructor(@field:Json(name = "bar") bar: String)

Java example with support for deserialization and serialization:

class Foo {
  @Json(name = "bar") String bar;
  @JsonConstructor Foo(String bar) {
    // Moshi detects that the constructor parameter bar is named the same as the annotated field bar
    this.bar = bar;
 }
}

swankjesse · 2017-03-20T12:30:17Z

Yeah, we’d want to do it as it's own module. It’d depend on kotlin.reflect and expose a JsonAdapter.Factory.

cbruegg · 2017-03-20T12:35:46Z

So alternative 2 it is? That would add 2 MB and ~ 16k methods to apps using that module. Are you sure that'd be preferable over the other alternatives?

swankjesse · 2017-03-20T13:35:17Z

Presumably people using Kotlin are already using Kotlin, so it’s not 2MiB in net new code.

cbruegg · 2017-03-20T13:37:58Z

It is. For most applications, only the dependency on kotlin-stdlib is needed, but not kotlin-reflect. I don't use kotlin-reflect in any of my applications, some of which rely heavily on Kotlin.

JakeWharton · 2017-03-20T13:42:19Z

The Jackson integration requires kotlin-reflect. Seems sensible to do the same here.

…

On Mon, Mar 20, 2017 at 9:38 AM Christian Brüggemann < ***@***.***> wrote: Presumably people using Kotlin are already using Kotlin, so it’s not 2MiB in net new code. It is. For most applications, only the dependency on kotlin-stdlib is needed, but not kotlin-reflect. I don't use kotlin-reflect in any of my applications, some of which rely heavily on Kotlin. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#266 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEEEXPjDM5Jklf9lyPL9Rcd56nIQmg_ks5rnoE2gaJpZM4MUqAZ> .

cbruegg · 2017-03-20T13:45:48Z

That's one of the reasons why I avoid Jackson. Any reason why alternatives 1 and 3 are apparently out of question?

JakeWharton · 2017-03-20T13:47:56Z

3 doesn't work because the class file format doesn't hold parameters names unless you target Java 8 and even then they're optional and opt-in (at least from the Java side). 1 is some nasty duplication.

…

On Mon, Mar 20, 2017 at 7:28 AM Christian Brüggemann < ***@***.***> wrote: Thanks for responding so quickly! Probably the right option for data classes is to use Kotlin’s reflection and actually call the constructor rather than assigning fields. I’m not sure if that’s symmetric, but it seems like a stronger design. That definitely sounds like a better approach. Here's how it could be done: Alternative 1 Let constructor parameters be annotated with @JSON. As a plus, this would also work for Java classes. A drawback would be having to annotate fields/properties *and* constructor parameters to support serialization instead of only deserialization. Kotlin example with support for deserialization only: data class ***@***.***:Json(name = "bar") bar: String) Kotlin example with support for deserialization and serialization: data class ***@***.***:Json(name = "bar") @field:Json(name = "bar") bar: String) Java example with support for deserialization only: class Foo { String bar; ***@***.***(name = "bar") String bar) { this.bar = bar; } } Java example with support for deserialization and serialization: class Foo { @JSON(name = "bar") String bar; ***@***.***(name = "bar") String bar) { this.bar = bar; } } Alternative 2 Like you proposed, Kotlin reflection could be used to detect annotations like this: @Property:Json(name = "bar"). Moshi would then know that the constructor parameter name equals the property name and could call the constructor for deserialization and use the field value for serialization. Discovering these annotations would require a dependency on the heavy kotlin-reflect library. Alternative 3 (I think this would be the easiest to use) Call the constructor for deserialization iff it is annotated with @JsonConstructor, indicating that the library user would like Moshi to call it. Require that constructor parameters are named the same as the fields they are assigned to. Kotlin example with support for deserialization and serialization: data class Foo @JsonConstructor ***@***.***(name = "bar") bar: String) which due to the currently allowed targets of the @JSON annotation equals data class Foo @JsonConstructor ***@***.***:Json(name = "bar") bar: String) Java example with support for deserialization and serialization: class Foo { @JSON(name = "bar") String bar; @JsonConstructor Foo(String bar) { // Moshi detects that the constructor parameter bar is named the same as the annotated field bar this.bar = bar; } } — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#266 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEEESLr1LOZby4jACq0h6LOvRlKbMqoks5rnmLSgaJpZM4MUqAZ> .

cbruegg · 2017-03-20T13:52:20Z

I see, thanks. I guess this PR can be closed then. I'll try to implement a module for Kotlin and send you a PR when it's done, unless you already have an exact implementation in mind.

swankjesse · 2017-03-20T21:50:37Z

I’m unsure whether you absolutely need kotlin.reflect. But I do think we win if we make a Kotlin-specific JsonAdapter. It can invoke the constructor, and potentially rely on the constructor to provide default values for fields that aren’t provided.

cbruegg added 5 commits March 6, 2017 20:56

Implement support for constructing objects using a specific constructor

1097096

Remove a debug stmt

fc1a349

Add test

75d550b

Fix indentation style

63e8379

Extract method

0ac2dab

NightlyNexus reviewed Mar 6, 2017

View reviewed changes

JakeWharton reviewed Mar 6, 2017

View reviewed changes

cbruegg added 2 commits March 6, 2017 23:19

Initial feedback integration

ca75cb3

More tests, check if only one constructor is annotated with JsonConst…

2bb51aa

…ructor.

swankjesse requested changes Mar 7, 2017

View reviewed changes

cbruegg closed this Mar 20, 2017

cbruegg mentioned this pull request Mar 20, 2017

Add kotlin-module with support for Kotlin data classes #269

Closed

Add support for forcing Moshi to construct instances using a specified constructor #266

Add support for forcing Moshi to construct instances using a specified constructor #266

Conversation

cbruegg commented Mar 6, 2017 • edited

NightlyNexus left a 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

cbruegg commented Mar 6, 2017

JakeWharton left a 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

Choose a reason for hiding this comment

swankjesse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbruegg commented Mar 7, 2017

swankjesse commented Mar 7, 2017

cbruegg commented Mar 7, 2017

swankjesse commented Mar 7, 2017

cbruegg commented Mar 7, 2017

JakeWharton commented Mar 7, 2017 via email

cbruegg commented Mar 7, 2017

cbruegg commented Mar 7, 2017

swankjesse commented Mar 7, 2017

cbruegg commented Mar 7, 2017

cbruegg commented Mar 9, 2017

cbruegg commented Mar 20, 2017

swankjesse commented Mar 20, 2017

cbruegg commented Mar 20, 2017

Alternative 1

Alternative 2

Alternative 3 (I think this would be the easiest to use)

swankjesse commented Mar 20, 2017

cbruegg commented Mar 20, 2017

swankjesse commented Mar 20, 2017

cbruegg commented Mar 20, 2017 • edited

JakeWharton commented Mar 20, 2017 via email

cbruegg commented Mar 20, 2017

JakeWharton commented Mar 20, 2017 via email

cbruegg commented Mar 20, 2017

swankjesse commented Mar 20, 2017

cbruegg commented Mar 6, 2017 •

edited

cbruegg commented Mar 20, 2017 •

edited