Ensure Constructor Parameter annotations are linked with those of Field, Getter, or Setter

[christophercurrie]

This issue relates to FasterXML/jackson-module-scala#197.

In Scala, the normal syntax for declaring a class takes a simplified form:

class Foo(bar: String)

This generates a JVM class that is similar to the following in Java

class Foo {
  private final String bar;

  private Foo(String bar) {
    this.bar = bar;
  }

  public getBar() {
    return bar;
  }
}

Where things get tricky is in specifying annotations on fields. To rename a property, for example, the natural syntax would look like:

class Foo(@JsonProperty("renamed") bar: String)

But the JVM that results from this looks like the following Java equivalent:

class Foo {
  private final String bar;

  private Foo(@JsonProperty("renamed") String bar) {
    this.bar = bar;
  }

  public getBar() {
    return bar;
  }
}

The getter is not tagged with the new name, and so the renaming doesn't happen on serialization. Similar issues exist for other annotations or other uses of @JsonProperty (such as exposing private fields)

The issue can be worked around in Scala by changing the way the annotation is specified on the class:

class Foo(@(JsonProperty @field @getter @setter)("renamed") bar: String)

This example is deliberately extra verbose, because even though the annotation does not need to be present in all of those locations in all cases, figuring out which ones it's needed for is a pain for Scala users.

An alternate workaround would be to create type aliases in Scala that do the same thing:

object ScalaModule {
  type JsonProperty = com.fasterxml.jackson.databind.JsonProperty @field @getter @setter
}

This is slightly better for the end user, though still not ideal, and the set of annotations would need to be kept in sync with the databind library.

The ideal solution would be a way for the Scala module to signal to databind that the annotation for the constructor parameter is significant in other locations as well. Currently this is done using faking it in findNameForSerialization, but that's not a general solution.

[christophercurrie]

Note that the above examples assume the presence of the Parameter Names module or other mechanism for recognizing the property corresponding to the constructor parameter.

[cowtowncoder]

Actually: the fix checked in 2.5 should cover your case, I think. My version of the test was bit different, and problem there was that the non-public field wasn't being used, but that should not be necessary when there is a visible getter. Plus I can see if non-public getter handling, when a visible creator is available, could be "pulled in".

[christophercurrie]

I haven't yet tested the patch, but if non-public fields are still an issue there may still be problems:

class Foo(@JsonProperty private bar: String)

The intent in this case is to ensure that the private field gets serialized. The JVM code generated in this corresponds to:

class Foo {
  private final String bar;

  private Foo(@JsonProperty String bar) {
    this.bar = bar;
  }
}

I get that automatically triggering visibility of the private field is more problematic here; if no name is specified it could make sense, but if a name is specified, then it's not clear from context whether @JsonProperty is intended for visibility, renaming, or both.

I'm open to suggestions. I suppose the simplest solution is to not support this case, but I'd need a different lever to turn. The two options I can think of are

class Foo1(@(JsonProperty @field) private bar: String)
class Foo2(@JsonProperty @JsonIgnore(false) private bar: String)

Foo1 would have the annotation on the field, which would work out of the box, but the syntax isn't commonly known. Foo2 would not have the annotations on the field, and so the 'property-linking' behavior would need to understand that @JsonIgnore is being used to override the default visibility, which might not happen in the current code. I'll try putting some tests together to validate.

[cowtowncoder]

My main concern with private fields is that their use for access (instead of getters) really is something that violates expectations and leads to security issues by Java usage (and possibly Scala).
For mutation use (setter) it is more common.

One possibility would be adding yet one more MapperFeature, somewhat similar to INFER_PROPERTY_MUTATORS. In fact, INFER_PROPERTY_ACCESORS would follow same naming and could work. Scala module could then choose different default setting (true).
It feels bit hacky, cumbersome, but at least could work and not change behavior.

The canonical usage block I am thinking of is something like:

public class Credentials {
   private String password;

   @JsonCreator
   public Credentials(@JsonProperty("password") String pw, ....) {
   }
}

where user would have write-only field.

I am open to other ways of dealing with this, if it's possible to recognize case classes.
AnnotationIntrospector could have another lookup method to indicate cases where "hidden" fields ought to be considered as mutators, if (and only if?) there is no getter, but there is creator or setter?
This would be more pluggable since introspector would be asked to participate, without needing to change feature settings.

[cowtowncoder]

@christophercurrie With bit more thinking, I wonder if the better way here would actually be for Scala module's annotation introspector to raise visibility of fields for case classes. Thing is, these classes have somewhat special requirements so it is probably better to treat them as special, instead of trying to make general POJO handling work to also cover them.

Method to override would be findAutoDetectVisibility(), and returned VisibilityChecker has various with mutant factories to use to override visibility just for fields.
And if level was defined to expose private fields just for case classes, that would solve the remaining problem of field not being visible.

[cowtowncoder]

Going back to the earlier idea, maybe adding MapperFeature.INFER_PRIVATE_GETTER_FIELD is the path of least resistance. Its semantics would be quite specific, so that:

1. If a visible Creator property exists and
2. Only a non-visible (for serialization, that is, as a "getter") Field exists, no visible getters

then that Field should be left as a getter.

This could be relaxed, if necessary, to also add "visible getter" (for step (1)), but I think I'd want to start with more restrictive to avoid accidental exposure of private fields.

This setting should default to false, but Scala module could enable it on registration.

[cowtowncoder]

Ok one more idea still. I hope to implement #813, which adds a way to enforce read-only/write-only/read-write behavior, which in turn can override visibility based rules. This is for supporting direct annotation of @JsonProperty(access=Accessor.READ_ONLY), but since it goes through AnnotationIntrospector it is something Scala module could provide. I am not sure if it is easy to reliably detect if such access modification (to force "READ_WRITE", which in turn forces use of whatever getter and setter accessors may be found, no matter what visibility) is safe, but that could be an option.

[cowtowncoder]

FWIW, #813 now implemented; AnnotationIntrospector now has findPropertyAccess() used to detect setting for READ_ONLY, WRITE_ONLY, READ_WRITE or AUTO (default, rely on visibility).

[cowtowncoder]

I think remaining issues (which do exist, wrt problems with properly handling Creator properties to be available during property name merging) are covered by more explicit issues, so closing this one.