[BUG] BulkRequest with scripted noop upsert UpdateOperation fails with "missing required property"

BrendonFaleiro commented 1 week ago

What is the bug?

Similar to Elasticsearch bug: https://github.com/elastic/elasticsearch-java/issues/403. When you do a scripted upsert with a script that does a "noop", there is no source in the response. But the InlineGet has source as a required field. https://github.com/opensearch-project/opensearch-java/blob/main/java-client/src/main/java/org/opensearch/client/opensearch/_types/InlineGet.java#L141-L146

A bulk request with an update operation set to do scripted upsert where the script chooses to set ctx.op = 'none' currently fails with

Missing required property 'InlineGet.source': org.opensearch.client.util.MissingRequiredPropertyException
org.opensearch.client.util.MissingRequiredPropertyException: Missing required property 'InlineGet.source'
    at org.opensearch.client.util.ApiTypeHelper.requireNonNull(ApiTypeHelper.java:89)
    at org.opensearch.client.opensearch._types.InlineGet.<init>(InlineGet.java:87)
    at org.opensearch.client.opensearch._types.InlineGet.<init>(InlineGet.java:55)
    at org.opensearch.client.opensearch._types.InlineGet$Builder.build(InlineGet.java:326)
    at org.opensearch.client.opensearch._types.InlineGet$Builder.build(InlineGet.java:205)
    at org.opensearch.client.json.ObjectBuilderDeserializer.deserialize(ObjectBuilderDeserializer.java:92)
    at org.opensearch.client.json.ObjectDeserializer$FieldObjectDeserializer.deserialize(ObjectDeserializer.java:81)
    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.ObjectBuilderDeserializer.deserialize(ObjectBuilderDeserializer.java:97)
    at org.opensearch.client.json.DelegatingDeserializer$SameType.deserialize(DelegatingDeserializer.java:60)
    at org.opensearch.client.json.JsonpDeserializerBase$ArrayDeserializer.deserialize(JsonpDeserializerBase.java:343)
    at org.opensearch.client.json.JsonpDeserializerBase$ArrayDeserializer.deserialize(JsonpDeserializerBase.java:308)
    at org.opensearch.client.json.JsonpDeserializer.deserialize(JsonpDeserializer.java:87)
    at org.opensearch.client.json.ObjectDeserializer$FieldObjectDeserializer.deserialize(ObjectDeserializer.java:81)
    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)
    at org.opensearch.client.json.DelegatingDeserializer$SameType.deserialize(DelegatingDeserializer.java:55)

How can one reproduce the bug?

BulkResponse response = client.bulk(
        BulkRequest.of(bulkRequest -> bulkRequest
                .index("test-index")
                .operations(bulkOperation -> bulkOperation
                        .update(updateOperation -> updateOperation
                            .index("indexName")
                            .id("myId")
                            .script(new Script.Builder()
                                .inline(new InlineScript.Builder()
                                    .lang("painless")
                                    .source("ctx.op = 'none';")
                                    .build())
                            .scriptedUpsert(true)
                            .upsert(ImmutableMap.of())
                        )
               )
        )
);

What is the expected behavior?

This should succeed.

The response from Opensearch that reaches the Transport is

{
    "took": 2,
    "errors": false,
    "items": [{
        "update": {
            "_index": "test-index",
            "_id": "myId",
            "_version": -1,
            "result": "noop",
            "_shards": {
                "total": 3,
                "successful": 3,
                "failed": 0
            },
            "get": {
                "found": false
            },
            "status": 200
        }
    }]
}

Do you have any additional context?

Similar to Elasticsearch bug: https://github.com/elastic/elasticsearch-java/issues/403.

BrendonFaleiro commented 1 week ago

I think this will also require a change in the api spec: https://github.com/opensearch-project/opensearch-api-specification/blob/33aa91a95dcb2155cc05b9917a790ca46aa7c35e/spec/schemas/_common.yaml#L1258

dblock commented 1 week ago

I think this will also require a change in the api spec: https://github.com/opensearch-project/opensearch-api-specification/blob/33aa91a95dcb2155cc05b9917a790ca46aa7c35e/spec/schemas/_common.yaml#L1258

Yes, please! With a test.

BrendonFaleiro commented 1 week ago

@dblock

With a test.

Did you mean in opensearch-java or in the api-spec? I have added a commit with the test.

I have sent the following requests:

Please let me know if anything else needs to be done.

BrendonFaleiro commented 1 week ago

Closed the previous pull requests. Here are the updated requests:

dblock commented 1 week ago

Closed the previous pull requests.

You can amend and force push btw, less work.

Xtansia commented 3 days ago

This fix was released in v2.11.1

opensearch-project / opensearch-java