Non constructor parameters properties are ignored about kotshi

[ansman]

@LouisCAD I would prefer to not have support for that unless there is a very compelling argument. I don't think we should consider properties declared outside the primary constructor to be a part of that class. This is similar to how Kotlin treats data classes (it doesn't include those properties in hashCode, equals or toString) which to me means that Kotlin's intentions are for those properties to be considered computed or cached.

One "hack" you could potentially use is to have two constructors like this:

@JsonSerializable
data class Foo(val prop1: String) {
    var prop2: String? = null

    @KotshiConstructor
    constructor(prop1: String, prop2: String?) : this(prop1) {
        this.prop2 = prop2
    }
}

Generated adapter:

public final class KotshiFooJsonAdapter extends JsonAdapter<Foo> {
  private static final JsonReader.Options OPTIONS = JsonReader.Options.of(
      "prop1",
      "prop2");

  public KotshiFooJsonAdapter() {
  }

  @Override
  public void toJson(JsonWriter writer, Foo value) throws IOException {
    if (value == null) {
      writer.nullValue();
      return;
    }
    writer.beginObject();
    writer.name("prop1");
    writer.value(value.getProp1());
    writer.name("prop2");
    writer.value(value.getProp2());
    writer.endObject();
  }

  @Override
  public Foo fromJson(JsonReader reader) throws IOException {
    if (reader.peek() == JsonReader.Token.NULL) {
      return reader.nextNull();
    }
    reader.beginObject();
    String prop1 = null;
    String prop2 = null;
    while (reader.hasNext()) {
      switch (reader.selectName(OPTIONS)) {
        case 0: {
          if (reader.peek() == JsonReader.Token.NULL) {
            reader.nextNull();
          } else {
            prop1 = reader.nextString();
          }
          continue;
        }
        case 1: {
          if (reader.peek() == JsonReader.Token.NULL) {
            reader.nextNull();
          } else {
            prop2 = reader.nextString();
          }
          continue;
        }
        case -1: {
          reader.nextName();
          reader.skipValue();
          continue;
        }
      }
    }
    reader.endObject();
    StringBuilder stringBuilder = null;
    if (prop1 == null) {
      stringBuilder = KotshiUtils.appendNullableError(stringBuilder, "prop1");
    }
    if (stringBuilder != null) {
      throw new NullPointerException(stringBuilder.toString());
    }
    return new Foo(
        prop1,
        prop2);
  }
}

[LouisCAD]

So, to clarify: my use case is to ignore property with Room, while still getting it from the JSON.

Your second constructor trick worked perfectly, and while I had to add a bit more boilerplate for the constructor delegation considering the high number of parameters, it enabled me to separate the Room annotations (@Embedded) that are seen in the main constructor from the Kotshi/Moshi ones (@Json(name= "…")) that are seen in the secondary one.
It makes it clearer what is persisted in DB and what is got from the remote API.

Thanks for your help, again!

I'm closing this issue now that this limitation is documented.

[ansman]

You are correct in assuming there is no support for this and it is by design. No non constructor property can be used with Kotshi, it would promote a bad design IMO (having mutable data-classes) plus it leads to many edge cases.

[LouisCAD]

Sometimes, you don't have control on what the remote server is sending, and being able to have such "transient" property helps.
Don't you think the design should be edited to support non constructor properties like Moshi for java and moshi-kotlin for Kotlin do?
This is preventing me to complete my kotshi migration, with no workaround since the field must not be part of the data class generated code, and I assume this issue will be faced by other people migrating to kotshi, especially from Java (which is not my case, could start this project 100% Kotlin)

[ansman]

I don't think so, I compare Kotshi to AutoValue and not the standard reflection adapter.
If you don't want it to be in hashCode and equals you could easily create a subclass to List< SupportTicket> that returns 0 for the hash code and true for equals. Or you could provide your own implementation of hashCode and equals.

It's a bit too tricky to support non constructor parameters at this time since delegates, lateinits and computed properties would have to be handled. I just rather see that some other solution is found.

[LouisCAD]

Also, I don't think this library should limit what we can do with our data classes more than Kotlin and the underlying Moshi of this library don't prohibit mutable values, and we've had mutable fields and properties for years with Java, which many projects took advantage of, among which some didn't suffer from the potential mutability issues.

I know mutable properties are risky, but I think preventing to use them in kotshi would do more harm than good because there can be good use cases for using non constructor properties from JSON, and these use cases are currently prohibited from using kotshi, which means an incompressible and unproguardable 1MB toll if you want to stick to Moshi.

[LouisCAD]

Wouldn't handling computed properties just mean calling the getter or setter?

Anyway, thanks for the subclass tip, will try!

[ansman]

Well, most of the time you wouldn't want computed properties in your JSON since they are just that, computed.

It is true that neither Moshi nor Kotlin prevents the use of mutable classes but I think it's reasonable to have that limitation. There are workarounds for your problem, even though I think that skipping properties in hash code and equals is very risky I'm sure you have your reasons for it.

I don't think it's unreasonable to say that we only support the primary properties of a data class. It doesn't really matter if they are mutable or not as long as they are in the primary constructor.

[LouisCAD]

My use case is to not leak the inconsistent remote API into the app's API. The supportTickets non constructor example property is something I put in a separate table in Room, so I want it to feel unsafe API design wise because I only deal with it at API request time, and not after, especially not after it's persisted in the Room DB.

I think I'll have to integrate it in the constructor for now since implementing List and overriding hashcode and equals safely is not trivial, and it doesn't prevent the property from appearing in the data class copy().

[JakeWharton]

My use case is to not leak the inconsistent remote API into the app's API.

Unless you're using the API model directly for the DB model, this desire indicates you should probably have separate model classes for the separate modelings of data.

…

On Sun, Dec 17, 2017, 11:37 AM Louis CAD ***@***.***> wrote: My use case is to not leak the inconsistent remote API into the app's API. The supportTickets non constructor example property is something I put in a separate table in Room, so I want it to feel unsafe API design wise because I only deal with it at API request time, and not after, especially not after it's persisted in the Room DB. I think I'll have to integrate it in the constructor for now since implementing List and overriding hashcode and equals safely is not trivial, and it doesn't prevent the property from appearing in the data class copy(). — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#30 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEEEYfNCeqi3-uE7ywLmPuyicBvAxchks5tBUM8gaJpZM4REsur> .

[ansman]

It feels like this is a very specific use case. Why not just have a separate data class for what you receive from the server and what you persist? This would give your much more flexibility and you wouldn't have these problems any more.

Out of curiosity, why don't you want the property to be a part of hash code and equals?

[LouisCAD]

@ansman I remember now that I tried to put it back in the constructor, my issues is not with hashCode and equals, nor copy, (although I prefer not include it in) but it is a compilation error.

Room compiler can't ignore constructor parameters, so putting it outside was my solution. EDIT: Here's an issue I created for the limitation in Room: https://issuetracker.google.com/issues/70762008

For the API & DB model separation, I find it counterproductive (both from programming and performance point of views) to copy the same model for just an optional property when everything else is how I need to save it in the DB.

[LouisCAD]

Whatever happens, I think this should be listed under the Limitations part of the README as long as it is. Should I make a PR?

[LouisCAD]

I cloned the repo on my computer and started digging through the code to understand what it'd mean to remove this limitation. It looks a bit hard, especially for a code generation beginner like me, but feasible.

@ansman If I succeed in adding support for non constructor properties, would it get merged assuming the code is clean enough according to your standards?

[LouisCAD]

I understand your point of sticking to the data class philosophy and approve the README clarifications.

My workaround to complete the migration has been to copy paste the generated adapter, (converting it to Kotlin in the process), then adding the missing bits I needed.

Your workaround seems better. I have to check it works with Room compiler, thanks for your help.