FasterXML / jackson-dataformats-binary

Uber-project for standard Jackson binary format backends: avro, cbor, ion, protobuf, smile
Apache License 2.0
310 stars 133 forks source link

[avro] Add support for external nullability annotations #147

Open marquiswang opened 6 years ago

marquiswang commented 6 years ago

If I have a simple class

public class Foo {
    private final String value;

    public Foo(String aValue) {
        value = aValue;
    }

    public String getValue() {
        return value;
    }
}

and generate a schema with:

import com.fasterxml.jackson.dataformat.avro.AvroMapper;
import com.fasterxml.jackson.dataformat.avro.AvroSchema;

import java.io.IOException;

public class Main {
    public static void main(String[] args) throws IOException {
        AvroSchema myAvroSchema = new AvroMapper().schemaFor(Foo.class);
        System.out.println(myAvroSchema.getAvroSchema().toString(true));
    }
}

I get:

{
  "type" : "record",
  "name" : "Foo",
  "fields" : [ {
    "name" : "value",
    "type" : [ "null", "string" ]
  } ]
}

Primitive fields are not marked nullable. No @NotNull or @Nonnull annotation seems to change the generated schema to have non-null fields. Am I doing something wrong?

baharclerode commented 6 years ago

Jackson is primarily a Java mapper, and Java's non-primitive types are inherently nullable, so that is the Jackson's default stance on whether a property is required or not. You can alter this behavior by marking the fields as required:

public class Foo {
    @JsonProperty(required = true) // Tell Jackson that this field is mandatory
    private final String value;

    public Foo(String aValue) {
        value = aValue;
    }

    public String getValue() {
        return value;
    }
}
marquiswang commented 6 years ago

Would you have any objections to changing Jackson so that a findbugs @Nonnull and/or Jetbrains @NotNull are requirement to required = true? I could look into a PR.

cowtowncoder commented 6 years ago

@marquiswang Since adding support for external annotation types would require another external dependency, we would have to carefully consider how support would be added. I think addition itself is a good idea, but it should possibly be a separate AnnotationIntrospector.

Another consideration, and why I think separate implementation (and likely, module) makes sense is that it should be possible to also NOT use these annotations -- I have found that there are usually some users, somewhere, that prefer certain things not to default to handling. This was experience with JDK 7 annotation @ConstructorProperties, support for which caused problems for some users.

imoverclocked commented 4 years ago

NullAway works specifically by looking for an @Nullable annotation. AFAICT, you could roll your own or if you were willing to include the JSR305 library you could use javax.annotations.Nullable

From uber/NullAway:

NullAway allows for any @Nullable annotation to be used, so, e.g., @Nullable from the Android Support Library or JetBrains annotations is also fine.

Having @Nullable added to parameters of setters and return values of getters would vastly improve our code-quality with respect to generated code.

fireboy1919 commented 11 months ago

I was able to do this successfully by just adding: @JsonProperty(required=true)