Openapi-generator: How can we set List to null by default in models?

[harikrishnanmurthy]

While generating Java models from OpenAPI spec (v3.0.0) using openapi-generator-maven-plugin, all the Lists in java class are initialized by default to new ArrayList<>(). How do we prevent this?

Description

We have an attribute defined as an array in our OpenAPI spec. While trying to generate the Java model classes, the resulting List is initialized by default to new ArrayList<>(). This is causing problems in our application since an empty arraylist and null are treated differently.

openapi-generator version

openapi-generator-maven-plugin
version : 3.3.0

OpenAPI declaration file content or url

    "references": {"type" : "array", "items" : {
        "type": "object",
        "properties" : {
            "type": {"type" : "string"},
            "source": {"type" : "string"},
            "value": {"type" : "string"}
        },
        "required": ["type", "value"],
        "additionalProperties": false
    }}

When the maven plugin is run, the resulting java code is

private List references= new ArrayList<>();

The reference field is not mandatory(optional)

What we want is:

private List references = null;

Command line used for generation

Generated using openapi-generator-maven-plugin version : 3.3.0

Steps to reproduce

Related issues/PRs

Suggest a fix

[clintfoster]

This behavior is also present in openapi-generator-gradle-plugin:4.0.0-beta2. However, it was not present in 3.3.2. It also affects Maps.

Example:

myMap:
  type: object
  additionalProperties:
    type: integer

Generated Java in 3.3.2

private Map<String, Integer> myMap = null;

Generated Java in 4.0.0-beta2

private Map<String, Integer> myMap = new HashMap<>();

I don't see a Java generator option to use the original behavior of providing null initializers instead of empty lists and maps.

This is a problem for us because we're persisting our models to Dynamo, which treats empty lists and maps differently than null.

[clintfoster]

@wing328 Here's a more complete description of the changed behavior specifically from 3.3.2 to 4.0.0-beta2:

Example MyModel definition

    MyModel:
      properties:
        myList:  # Java type: List<String>
          type: array
          items:
            type: string
        myMap:  # Java type: Map<String, String>
          type: object
          additionalProperties:
            type: string

3.3.2 Generated MyModel.java (good)

Provides default list & map initializers in convenience methods (but not declarations).

public class MyModel {

  // **** GOOD: No need to initialize these since put/add convenience methods do it
  private List<String> myList = null;
  private Map<String, String> myMap = null;

  public MyModel addMyListItem(String myListItem) {
    if (this.myList == null) {
      this.myList = new ArrayList<>();
    }
    this.myList.add(myListItem);
    return this;
  }

  public MyModel putMyMapItem(String key, String myMapItem) {
    if (this.myMap == null) {
      this.myMap = new HashMap<>();
    }
    this.myMap.put(key, myMapItem);
    return this;
  }
}

4.0.0-beta2 Generated MyModel.java (bad)

Provides default list & map initializers in convenience methods _and_ declarations. This is arguably redundant, and causes migration headaches and side effects:

public class MyModel implements Serializable {

  // **** BAD: Initializing these indiscriminately has unwanted side effects
  private List<String> myList = new ArrayList<>();
  private Map<String, String> myMap = new HashMap<>();

  public MyModel addMyListItem(String myListItem) {
    if (this.myList == null) {
      this.myList = new ArrayList<>();
    }
    this.myList.add(myListItem);
    return this;
  }

  public MyModel putMyMapItem(String key, String myMapItem) {
    if (this.myMap == null) {
      this.myMap = new HashMap<>();
    }
    this.myMap.put(key, myMapItem);
    return this;
  }
}

Analysis

The generated addXxxItem() / putXxxItem() convenience methods are the same in both cases, and are useful since they they prevent NullPointerException if someone tries to add an item to the list or map without calling setXxx to initialize it first, e.g.:

• addMyListItem() // always safe
• putMyMapItem() // always safe

The question is whether we also need to protect the programmer in case they do _not_ call the convenience method _and_ they fail to initialize the list or map:

• getMyList().add() // not safe if myList uninitialized
• getMyMap().put() // not safe it myMap uninitialized

I think the default initializers are not a good change in 4.0.0-beta2 because they have the following side effects:

• If the POJO is used with a db like Dynamo that treats an empty list or map differently than a null one, migration code must be added to nullify empty lists and maps as needed.
• If the POJO is used to serialize JSON, migration code must be added to nullify empty lists and maps to avoid cluttering the JSON output (possibly can be done with serializer settings in some cases).
• Existing code that checks for null must be migrated to also check for an empty list or map. Our project is in this boat as it now has quite a bit of runtime breakage with the new plugin.
• If you call setMyList() or setMyMap() to use a different list or map implementation, e.g. LinkedList instead of ArrayList or TreeMap instead of HashMap, the unwanted (and unavoidable) default initializer object serves no purpose other than to add work for the garbage collector (minor).

Since the initialization is always done there is no reasonable cure, short of adding code to undo the initialization as needed, or creating a custom pogo.mustache to prevent the initialization in the first place.

Possible fix: Make template aware of nullable?

It seems better not to require carrying around a custom pojo.mustache to avoid this new behavior. One way to address it while minimizing breakage for anyone already dependent on the new behavior might be to make the provided template conditional on the OAS 3 nullable attribute, as follows:

Current pojo.mustache

  private {{{datatypeWithEnum}}} {{name}}{{#defaultValue}} = {{{.}}}{{/defaultValue}};

"Fixed" pojo.mustache

    private {{{datatypeWithEnum}}} {{name}}{{^isNullable}}{{#defaultValue}} = {{{.}}}{{/defaultValue}}{{/isNullable}};

I tried this, and it works. But I'm not sure it's in keeping with the intent of nullable in OAS 3. Also, it's not quite right because we still want to initialize non-list, non-maps with the OAS default attribute (if any), e.g.:

  {{#isListContainer}}
  private {{{datatypeWithEnum}}} {{name}}{{^isNullable}}{{#defaultValue}} = {{{.}}}{{/defaultValue}}
  {{/isListContainer}}
  {{#isMapContainer}}
  private {{{datatypeWithEnum}}} {{name}}{{^isNullable}}{{#defaultValue}} = {{{.}}}{{/defaultValue}}
  {{/isMapContainer}}
  {{^isListContainer}}
  {{^isMapContainer}}
  private {{{datatypeWithEnum}}} {{name}}{{#defaultValue}} = {{{.}}}{{/defaultValue}};
  {{/isMapContainer}}
  {{/isListContainer}}

Now we're really making a mess! Also, perhaps it's exacerbating a problem that already existed in which the defaultValue template variable is being overloaded for two slightly different purposes.

Probably either a specific generator option like disableDeclarationInitializers should be added, or the original behavior should be restored. I may be biased because of backwards-incompatibility for our project, but I believe the latter is better because always providing initializers crosses a line into second-guessing what the programmer wants in a way that is not easily reversible when it's _not_ what they want.

[wing328]

What about fixing the issue in the toDefalutValue method instead?

[clintfoster]

I had the same thought. I think that might be part of the ideal solution. But if I'm understanding the code correctly it requires fixing some semantic overloading in the defaultValue template variable.

pojo.mustache references the defaultValue template variable produced by toDefalutValue() in three places:

1. Type initializer.
2. addXxxItem() convenience method for lists.
3. putXxxItem() convenience method for maps.

Case 1 uses the value of the default attribute from the schema for everything but lists & maps, where it instead blindly provides an empty ArrayList or HashMap initializer (or perhaps custom type from instantiationTypes). If the purpose of this variable is to communicate default values from the schema to the template you might think specifying [1, 2, 3] would generate an initializer like Arrays.asList(1, 2, 3) in the POJO. But instead the generator silently ignores the default value and provides an empty list.

Setting aside the issue of whether we should support default values for lists, this shows the meaning of defaultValue is being overloaded in a way that probably makes it impossible to solve the problem only in the generator code since we _do_ want default values to be transferred from the schema to the POJO in initializers for other types. But that's not the same thing as providing a rote empty initializer. The generator code has no way of knowing that it is populating the defaultValue template variable for the first purpose (e.g. a default int value of 2, or maybe even a default array value of [1, 2, 3]) versus the second purpose (e.g. an empty ArrayList).

Heck, if we're providing a rote initializer for lists & maps, why not also provide new Integer(0) for ints or new String() for strings? Of course, that's going too far. But it further illustrates that the defaultValue template variable is conflating two ideas.

I still believe the old behavior was correct, i.e. Case 1 simply should not exist. We should not provide a rote empty initializer for lists & maps (any more than we should for ints & strings). Cases 2 & 3 are useful and presumably part of the purpose of generating those convenience methods. But, even for Cases 2 & 3, the type initializer probably shouldn't come from the defaultValue template variable. Instead it should come from some other template variable such instantiationType (to match the instantiationTypes configuration option).

If you still think v4 needs to support the newly-introduced behavior in Case 1 (perhaps for people who have already come to depend on it), another idea would be to introduce a provideEmptyInstantiation option to enable it. Or it could have the reverse sense (disableEmptyInstantiation). But, again, I don't think this new behavior makes sense as the default since the convenience methods provide the initializer on a "just in time" basis when a value is actually being added to the list or map, avoiding pollution of the POJO with empty initializers that have the aforementioned side effects (most of which affect our project, at least):

• Subtle runtime backwards-compatibility issues in code that was previously checking only for null (not empty lists & maps).
• Empty lists & maps added to dbs such as Dynamo, causing needless read/write unit consumption.
• Empty lists & maps cluttering up serialized JSON (from which those properties would otherwise be entirely omitted).
• Needless garbage generated on heap in cases where no item was ever added, or the programmer chose to provide a field-specific list or map initializer at runtime via setXxx() (such as LinkedList or TreeMap).

[jmini]

Thank you for the analysis.

An other important reason why null is better than empty list was provided by @bkabrda in https://github.com/OpenAPITools/openapi-generator/issues/3585#issuecomment-519450159

In some scenario (typically with PATCH) a null list means "do not change the value" and an empty list means "remove the items of that list".

Setting the value to empty list is confusing because in that case the user needs to set the value to null explicitly, which is an uncommon pattern in Java.

---

With PR #3615 I have proposed to align the Java template:

https://github.com/OpenAPITools/openapi-generator/blob/7f36f2642209c6d7070e1d76209ba1928d816922/modules/openapi-generator/src/main/resources/Java/pojo.mustache#L61

With JavaJaxRS:

https://github.com/OpenAPITools/openapi-generator/blob/7f36f2642209c6d7070e1d76209ba1928d816922/modules/openapi-generator/src/main/resources/JavaJaxRS/pojo.mustache#L24-L29

There the trigger used to determine if the list should be initialized or not is required. Maybe usage of isNullable is better?

[andreas-schilling]

Hi all! Jumping in, as we're also discussing around those things right now.
In our case it would be really cool to have an option to allow using the default initializers for List/Map types even when the respective property is not required.
We don't use (or rather allow) null as a value for container types and Jackson is configured to omit empty containers upon serialization. Anything down to the persistence level is not using the generated classes, so the issue of unwanted behaviour on DB level is out of scope for us as well. For the PATCH use case we explicitly use nullable so that JsonNullable is used as the type and all variants for the property (drop/reset, update, ignore) work.

We know that @wing328 raised concerns related to too many options (totally valid), still it seems this is the only viable option in this case, allowing to employ the behaviour that is the subject of this issue (which is understandably undesired for many, but also desired for quite a few, including us :-) )

We'd be glad to discuss on this and also be happy to contribute!