[BUG] DDBClient does not generate an id for PutRequest when id is an empty string

[MohammedAghil]

What is the bug?

The DDBClient implementation does not generate a new id in the case where an empty string is provided as the id. This is different from the LocalIndicesClient implementation where an Id is generated for both null or empty string.

DDB Client Existing Implementation

final String id = request.id() != null ? request.id() : UUID.randomUUID().toString();

Local Client Existing Implementation

if (!Strings.isNullOrEmpty(putDataObjectRequest.id())) {
    indexRequest.id(putDataObjectRequest.id());
}

How can one reproduce the bug?

Create a PutDataObjectRequest and provide id as an empty string, the operation will fail due to an error mentioning that the id is expected to be non-empty.

What is the expected behavior?

Similar to how the LocalIndicesClient implementation, the DDBClient implementation should also generate an id for both null and empty string.

Do you have any additional context?

The issue was noticed as part of onboarding opensearch alerting with the remote metadata sdk client

[dbwiddis]

Thanks for reporting! We'll get that fix in for 3.1.0.

[MohammedAghil]

Thanks @dbwiddis putting out a fix shortly

[dbwiddis]

Thanks @dbwiddis putting out a fix shortly

Beat you to it. ;)

[dbwiddis]

Thanks @dbwiddis putting out a fix shortly

Beat you to it. ;)

But it would be helpful if you add the same fix to the 2.19 branch since I don't expect a clean auto-backport.

[MohammedAghil]

Sure, I will backport it.

[MohammedAghil]

Doing Additional Testing to check the way id is handled with node client (Not using remote metadata sdk)

1. Test with whitespace string

val indexRequest = IndexRequest(SCHEDULED_JOBS_INDEX)
                .setRefreshPolicy(request.refreshPolicy)
                .id("  ")
                .source(request.monitor.toXContentWithUser(jsonBuilder(), ToXContent.MapParams(mapOf("with_type" to "true"))))
                .setIfSeqNo(request.seqNo)
                .setIfPrimaryTerm(request.primaryTerm)
                .timeout(indexTimeout)

Succeeded with

IndexResponse[
index=.opendistro-alerting-config,
id=  ,version=1,result=created,seqNo=0,primaryTerm=1,
shards={"total":2,"successful":1,"failed":0}
]

2. Test with empty string

val indexRequest = IndexRequest(SCHEDULED_JOBS_INDEX)
                .setRefreshPolicy(request.refreshPolicy)
                .id("")
                .source(request.monitor.toXContentWithUser(jsonBuilder(), ToXContent.MapParams(mapOf("with_type" to "true"))))
                .setIfSeqNo(request.seqNo)
                .setIfPrimaryTerm(request.primaryTerm)
                .timeout(indexTimeout)

Failed with

   {"error":{"root_cause":[{"type":"alerting_exception","reason":"if _id is specified it must not be empty"}],"type":"alerting_exception","reason":"if _id is specified it must not be empty","caused_by":{"type":"ex
ception","reason":"java.lang.IllegalArgumentException: if _id is specified it must not be empty"}},"status":400}  

This is the behavior of open search node client. In the case of a null id, it generates a new id.

[dbwiddis]

OK so we should match: white space ID string is OK. Empty string should 400.