[Feature/multi_tenancy] Update GetDataObjectResponse to include full GetResponse parser

[dbwiddis]

Description

Some APIs (get model group) require the full GetResponse object from the API.

This changes the GetDataObjectResponse to use the full GetResponse as the parser. Since there will always be a response, there is no longer a need for Optional, and the isExists() checks are available again.

It retains the source map as-is.

This parallels the implementation in the SearchDataObjectResponse (and probably would have done it that way if I'd done search first).

This allows for even closer-to-original migration of code expecting a GetResponse.

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[codecov]

Codecov Report

Attention: Patch coverage is 64.88550% with 46 lines in your changes missing coverage. Please review.

Please upload report for BASE (feature/multi_tenancy@25bdbb9). Learn more about missing BASE report.

❗ Current head 9044be1 differs from pull request most recent head 2571725

Please upload reports for the commit 2571725 to get more accurate results.

Files	Patch %	Lines
...search/ml/action/tasks/GetTaskTransportAction.java	12.50%	13 Missing and 1 partial ⚠️
...rch/ml/action/tasks/DeleteTaskTransportAction.java	65.62%	9 Missing and 2 partials ⚠️
...search/ml/helper/ConnectorAccessControlHelper.java	50.00%	7 Missing and 1 partial ⚠️
...tion/model_group/GetModelGroupTransportAction.java	56.25%	5 Missing and 2 partials ⚠️
...n/model_group/TransportUpdateModelGroupAction.java	70.00%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@                   Coverage Diff                    @@
##             feature/multi_tenancy    #2587   +/-   ##
========================================================
  Coverage                         ?   81.01%           
  Complexity                       ?     6134           
========================================================
  Files                            ?      582           
  Lines                            ?    25916           
  Branches                         ?     2691           
========================================================
  Hits                             ?    20997           
  Misses                           ?     3783           
  Partials                         ?     1136           

Flag	Coverage Δ
ml-commons	81.01% <64.88%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dhrubo-os]

Are all these values constant for every cases? Like version will be 1 always?

[dbwiddis]

Probably need @arjunkumargiri to chime in here; this falls into the same category as "shard" information in DDB which is irrelevant, but I haven't been interacting with the DDB implementation enough to know what (if any) is relevant.

Otherwise best I can do is add a TODO to revisit this later.

[arjunkumargiri]

This would be a concern if plugins depends upon versions for any operation. It is preferred if the SDKClient interface hides all opensearch specific details and provides a generic CRUD+search interface.

[dbwiddis]

It is preferred if the SDKClient interface hides all opensearch specific details

What do we do with OpenSearch customers who are expecting this information and may already have automation relying on the GetResponse API behavior that has been there for years?

I need a really strong argument for taking away information here.

[dbwiddis]

Are all these values constant for every cases? Like version will be 1 always?

Looking more deeply I can just omit these from the strings and it will go with the defaults, which match what was there except version is -1.

[arjunkumargiri]

is _trasport() internal method? JsonMapper should rather be passed in as part of constructor

[dbwiddis]

It's a public method on the ApiClient class:

https://github.com/opensearch-project/opensearch-java/blob/3af2d776903123c336686ffbfaafc5503f949b5b/java-client/src/main/java/org/opensearch/client/ApiClient.java#L66-L68

It's specifically listed exactly like this in the opensearch-java documentation:
https://github.com/opensearch-project/opensearch-java/blob/3af2d776903123c336686ffbfaafc5503f949b5b/guides/generic.md?plain=1#L81

[arjunkumargiri]

This approach tightly couples SDKClient to the opensearch specific schema. This is resulting in non-opensearch clients building mimicking opensearch response which will not be flexible.

Rather than modifying SDKClient can model group action fetch required field from parser?

[dbwiddis]

Rather than modifying SDKClient can model group action fetch required field from parser?

When the GET request is on OpenSearch, we want to keep the same data that already exists in the response. (We're on round 4 of this conversation.). So yes, we will aways be in the situation of having to mimic an OpenSearch response. We have to either pretend there are shards/shard stats on non-OpenSearch implementations, or we have to take away information in a response that existing OpenSearch users are expecting to see. I strongly believe the former is the correct response.

If you strongly feel otherwise, let's have that conversation.

For the specifics of a GetResponse there are exactly two required fields, _index and _id. These can be recreated from the request, as I have done here.

The found field is also "required" due to a bug. That can also be recreated by a test whether DDB returned an item, which I have done.

The remaining fields (version, seq_no, primary term) are exactly the defaults if they are not included, except I see version should be -1.

        long version = -1;
        long seqNo = UNASSIGNED_SEQ_NO;
        long primaryTerm = UNASSIGNED_PRIMARY_TERM;

Seeing this I'll just remove those fields from the String.