[BUG] `required` spec not respected for maps and lists

[moesterheld]

What is the bug?

In the spec, a null value can be allowed for lists, e.g. here:
https://github.com/opensearch-project/opensearch-api-specification/blob/963f90293d480015deaf352d1987bec2f18916c2/spec/schemas/ism._common.yaml#L50-L57

Deserializing these will fail with a NPE due to the Objects.requireNonNull in _listAddAll

How can one reproduce the bug?

ISM Policies can be created without nested ISM Templates. As an example, see the create policy request/response in the docs

However, reading a policy created like this without the client fails due to the list being null

java.lang.NullPointerException
	at java.base/java.util.Objects.requireNonNull(Objects.java:233)
	at org.opensearch.client.util.ObjectBuilderBase._listAddAll(ObjectBuilderBase.java:101)
	at org.opensearch.client.opensearch.ism.Policy$Builder.ismTemplate(Policy.java:379)
	at org.opensearch.client.json.ObjectDeserializer$FieldObjectDeserializer.deserialize(ObjectDeserializer.java:82)
	at org.opensearch.client.json.ObjectDeserializer.deserialize(ObjectDeserializer.java:185)
	at org.opensearch.client.json.ObjectDeserializer.deserialize(ObjectDeserializer.java:146)
	at org.opensearch.client.json.JsonpDeserializer.deserialize(JsonpDeserializer.java:87)
	at org.opensearch.client.json.ObjectBuilderDeserializer.deserialize(ObjectBuilderDeserializer.java:91)

What is the expected behavior?

lists and maps should respect required like all other properties. If this is not possible, deserialization should take care of initializing them if they are missing from the JSON (however, the two representations would not be equal then)

What is your host/environment?

OS java client 3.2/3.3

Do you have any additional context?

I wonder why they are conditionally excluded from the required check. What is the reason for that?

[Xtansia]

Hi @moesterheld,

While the lists/maps are always non-null, they are still optional for deserialization, if it's null/unset on the builder then ApiTypeHelper.unmodifiable() in the type constructor replaces it with an empty "undefined" list/map type. With ApiTypeHelper.unmodifiableRequired() being the null/unset rejecting variant. You can differentiate the undefined and explicit empty list with ApiTypeHelper.isDefined() which is what's used in the serialization logic.

[moesterheld]

Hi @Xtansia
thanks for the quick answer.
When deserializing a ISM Policy without a ISMTemplate, I get

java.lang.NullPointerException
	at java.base/java.util.Objects.requireNonNull(Objects.java:233)
	at org.opensearch.client.util.ObjectBuilderBase._listAddAll(ObjectBuilderBase.java:101)
	at org.opensearch.client.opensearch.ism.Policy$Builder.ismTemplate(Policy.java:379)
	at org.opensearch.client.json.ObjectDeserializer$FieldObjectDeserializer.deserialize(ObjectDeserializer.java:82)
	at org.opensearch.client.json.ObjectDeserializer.deserialize(ObjectDeserializer.java:185)
	at org.opensearch.client.json.ObjectDeserializer.deserialize(ObjectDeserializer.java:146)
	at org.opensearch.client.json.JsonpDeserializer.deserialize(JsonpDeserializer.java:87)
	at org.opensearch.client.json.ObjectBuilderDeserializer.deserialize(ObjectBuilderDeserializer.java:91)

[Xtansia]

Hi @Xtansia
thanks for the quick answer.
When deserializing a ISM Policy without a ISMTemplate, I get

java.lang.NullPointerException
	at java.base/java.util.Objects.requireNonNull(Objects.java:233)
	at org.opensearch.client.util.ObjectBuilderBase._listAddAll(ObjectBuilderBase.java:101)
	at org.opensearch.client.opensearch.ism.Policy$Builder.ismTemplate(Policy.java:379)
	at org.opensearch.client.json.ObjectDeserializer$FieldObjectDeserializer.deserialize(ObjectDeserializer.java:82)
	at org.opensearch.client.json.ObjectDeserializer.deserialize(ObjectDeserializer.java:185)
	at org.opensearch.client.json.ObjectDeserializer.deserialize(ObjectDeserializer.java:146)
	at org.opensearch.client.json.JsonpDeserializer.deserialize(JsonpDeserializer.java:87)
	at org.opensearch.client.json.ObjectBuilderDeserializer.deserialize(ObjectBuilderDeserializer.java:91)

Can you provide a snippet of your code and the actual JSON output from OpenSearch?

[moesterheld]

This is a test that contains an actual response from OpenSearch https://raw.githubusercontent.com/opensearch-project/opensearch-java/5ac91edb5bddb1ecf83e8f7dcc49733dab8a1c24/java-client/src/test/java/org/opensearch/client/opensearch/ism/PolicyRequestTest.java

However, to get this to work in main, you will need to remove last_updated_time and error_notification from the json and the last_updated_time assertion (lines 44,53,55).

[moesterheld]

Looking at the test once more, I think the problem is not what I stated above. The problem is that the property is there, but that it is null

[moesterheld]

Which is ok according to the spec
https://github.com/opensearch-project/opensearch-api-specification/blob/963f90293d480015deaf352d1987bec2f18916c2/spec/schemas/ism._common.yaml#L50-L57
Let me rephrase the issue. Sorry for the confusion

[moesterheld]

There was another oneOf construct with a null in Policy which didn't affect lists in the ISM spec where I created a PR for preventing the NPE.

Maybe this kind of construct is only used there. Since you have more experience. Would this be something that could be changed on the spec side altogether?