Feature/get missing alias json exception 301

[szczepanczykd]

Description

Handling String error response from /_alias/not_existing_alias endpoint.

Add errorDeserializer to SimpleEndpoint constructor -> and use ErrorResponse._DESERIALIZER in every instance (except the GetAliasRequest._ENDPOINT) -> maybe two constructors would be better (one with default errorDeserializer)?

Created OpenSearchExceptionFactory -> thrown OpenSearchException is untouched.

API error response for GET https://localhost:9200/_alias/not_existing_alias (ErrorStringResponse format)

API error response for GET https://localhost:9200/not_existing_index/_search (ErrorResponse format)

Issues Resolved

#301

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.

[dblock]

Looks OK to me. Made some smallish suggestions.

Should we open an issue in OpenSearch to not return a different response format in 3.0? This seems like an odd choice server-side. Are there other places where the server does this?

[szczepanczykd]

Looks OK to me. Made some smallish suggestions.

Should we open an issue in OpenSearch to not return a different response format in 3.0? This seems like an odd choice server-side. Are there other places where the server does this?

I think yes, it seems it is only one case:
https://github.com/opensearch-project/OpenSearch/blob/8e0edee3d403e5f2339d176b800e9e5df1372692/server/src/main/java/org/opensearch/rest/action/admin/indices/RestGetAliasesAction.java

Lines: [160, 162]

Thanks for the review :)

[dblock]

Looks OK to me. Made some smallish suggestions.
Should we open an issue in OpenSearch to not return a different response format in 3.0? This seems like an odd choice server-side. Are there other places where the server does this?

I think yes, it seems it is only one case: https://github.com/opensearch-project/OpenSearch/blob/8e0edee3d403e5f2339d176b800e9e5df1372692/server/src/main/java/org/opensearch/rest/action/admin/indices/RestGetAliasesAction.java

Lines: [160, 162]

Thanks for the review :)

Will you please do it?

[dblock]

I have some nitpicky asks, the only must have is the license header update, we don't need the "modifications ..." part.

I think the rest of the code uses two spaces instead of 4/tabs? Mind checking and matching that. We should have some style checker/autoformatter do this and fail CI otherwise, too.

[szczepanczykd]

I have some nitpicky asks, the only must have is the license header update, we don't need the "modifications ..." part.

I think the rest of the code uses two spaces instead of 4/tabs? Mind checking and matching that. We should have some style checker/autoformatter do this and fail CI otherwise, too.

The tests are using 4 spaces.
Rest of the code uses tabs.

[szczepanczykd]

Looks OK to me. Made some smallish suggestions.
Should we open an issue in OpenSearch to not return a different response format in 3.0? This seems like an odd choice server-side. Are there other places where the server does this?

I think yes, it seems it is only one case: https://github.com/opensearch-project/OpenSearch/blob/8e0edee3d403e5f2339d176b800e9e5df1372692/server/src/main/java/org/opensearch/rest/action/admin/indices/RestGetAliasesAction.java
Lines: [160, 162]
Thanks for the review :)

Will you please do it?

Done: opensearch-project/OpenSearch#6904

[dblock]

Re-running flaky tests. Someone should look at those, wink wink :)

[reta]

Re-running flaky tests. Someone should look at those, wink wink :)

There is a fix suggested already #421

[dblock]

@reta I just merged that. Shall we just merge this one?

[reta]

@reta I just merged that. Shall we just merge this one?

@dblock the main is broken now (after opensearch-project/OpenSearch#6875), fixing it now

[VachaShah]

Hi @szczepanczykd, apologies for the delay! Can you merge the latest changes from main and fix the conflicts? We can get this merged once the CI is green.

[szczepanczykd]

Hi :) Sure, I will do it today, around 18 UTC.

…

On Fri, 5 May 2023, 07:57 Vacha Shah, ***@***.***> wrote: Hi @szczepanczykd <https://github.com/szczepanczykd>, apologies for the delay! Can you merge the latest changes from main and fix the conflicts? We can get this merged once the CI is green. — Reply to this email directly, view it on GitHub <#399 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALVQM4MRLPGFIZZGFMGDCSLXESJDBANCNFSM6AAAAAAVS7U6UA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[szczepanczykd]

Hi @szczepanczykd, apologies for the delay! Can you merge the latest changes from main and fix the conflicts? We can get this merged once the CI is green.

@VachaShah done :)

[reta]

@dblock @szczepanczykd @VachaShah I think this change is good but it does not follow the way we manage such API elements (when multiple forms are possible). Generally, it is done as a union (see for example https://github.com/opensearch-project/opensearch-java/blob/main/java-client/src/main/java/org/opensearch/client/opensearch/_types/Script.java#L164 but there are much more).

The way I would see it working is:

• in both cases, the error is the same class ErrorResponse
• but depending on the type (Object or String - solved by UnionDeserializer), it is either a full one or just stripped one with some part of the ErrorCause populated (like reason)

WDYT?

[szczepanczykd]

Sounds good. Let's try it.

…

On Fri, 5 May 2023, 20:16 Andriy Redko, ***@***.***> wrote: @dblock <https://github.com/dblock> @szczepanczykd <https://github.com/szczepanczykd> @VachaShah <https://github.com/VachaShah> I think this change is good but it does not follow the way we manage such API elements (when multiple forms are possible). Generally, it is done as a union (see for example https://github.com/opensearch-project/opensearch-java/blob/main/java-client/src/main/java/org/opensearch/client/opensearch/_types/Script.java#L164 but there are much more). The way I would see it working is: - in both cases, the error is the same class ErrorResponse - but depending on the type (Object or String - solved by UnionDeserializer), it is either a full one or just ErrorCause with message populated WDYT? — Reply to this email directly, view it on GitHub <#399 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALVQM4N2F6HOUDM6GYZ2U5LXEU7ZVANCNFSM6AAAAAAVS7U6UA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[szczepanczykd]

@VachaShah @reta
I have created a new PR with a simpler approach to this issue :)
#476

[szczepanczykd]

Done in: #476