[FEATURE] Throw OpenSearchException on 409 Conflict

[rursprung]

Is your feature request related to a problem?

note: i'm using the ApacheHttpClient5Transport APIs.

i'm doing a request through OpenSearchClient#create because the API allows me to ensure that a document is only indexed once, as per the javadoc:

opensearch-java/java-client/src/main/java/org/opensearch/client/opensearch/OpenSearchClient.java

Lines 329 to 338 in c0f51d0

	/**
	* Creates a new document in the index.
	* <p>
	* Returns a 409 response when a document with a same ID already exists in the
	* index.
	*
	*
	*/
	public <TDocument> CreateResponse create(CreateRequest<TDocument> request) throws IOException, OpenSearchException {

however, if the document with this ID already exists then the API throws an exception instead of returning a response. and while the ResponseException would offer access to the actual Response:

opensearch-java/java-client/src/main/java/org/opensearch/client/transport/httpclient5/ResponseException.java

Lines 92 to 94 in c0f51d0

	public Response getResponse() {
	return response;
	}

which would also offer access to the HTTP status via StatusLine:

opensearch-java/java-client/src/main/java/org/opensearch/client/transport/httpclient5/Response.java

Lines 84 to 86 in c0f51d0

	public StatusLine getStatusLine() {
	return new StatusLine(response);
	}

the problem is that Response is package-private, so i cannot access any of its methods (even though they're marked as public):

opensearch-java/java-client/src/main/java/org/opensearch/client/transport/httpclient5/Response.java

Line 52 in c0f51d0

final class Response {

What solution would you like?

there should be a clear, canonical (and documented) way of accessing the HTTP status if such a request fails.

one possibility could be to make Response public (i don't understand why it isn't).

What alternatives have you considered?

parsing the exception as a string to see if it contains "[HTTP/1.1 409 Conflict]" is at best an ugly hack as the string is not a guaranteed API

Do you have any additional context?

i wasn't sure whether to categorise this as a feature request (because it needs a new API to be exposed), a bug report (because i consider it wrong that i can't access this information) or a question (because i might just have missed a way of getting to that information; in that case it'd be good if it could be documented as i didn't find it in this repo!).

[dblock]

+1, it's pretty common to want the entire underlying HTTP request/response to be accessible. The alternative of exposing methods is also fine if we want to support a generic way to retrieve various non-transport-specific things like HTTP status codes.

[rursprung]

i can provide a PR which just makes Response public if that's ok with you? or do you want to change the API to access the information in other ways?

[dblock]

@rursprung I don't know what's best. Do you have an opinion? Appreciate any PRs!

[reta]

@dblock @rursprung I believe we should do that as part of #377, the later should allow raw request / response manipulation, strong -1 to open up any client transport internals.

[dblock]

@reta Let's focus on the basic ask of exposing the HTTP status code. Can you explain what are your reasons not to allow the caller to explicitly be able to know whether the server responded with a 302 vs. a 301 for example, directly?

[reta]

@dblock in my opinion, the Java client general interface is typed and errors are reported using exceptions, the 302 or 301 is not relevant to the caller of typed API, the only relevant part is success or not, with generic client that would be possible to manipulate over raw request / response.

[dblock]

Makes sense @reta, so what is a proposed change to be able to extract a 409 Conflict? Should it be a specialized exception? What should it inherit from?

[reta]

Makes sense @reta, so what is a proposed change to be able to extract a 409 Conflict?

Thanks @dblock , this is already working this way, the OpenSearchException is being raised, it has status field with would have 409 status code.

[rursprung]

@reta: the problem is that OpenSearchClient#create throws a ResponseException and not an OpenSearchException in case of a 409 - thus i cannot access that field.
so maybe that is then a bug specific to this API (and possibly others?) and it should catch the ResponseException and wrap it in an OpenSearchException?

[dblock]

@rursprung That could make sense, but maybe it's easier if ResponseException exposes a status field? I think it's in the spirit of what OpenSearchException does. Want to give it a shot?

[reta]

@rursprung That could make sense, but maybe it's easier if ResponseException exposes a status field?

Thanks @rursprung , or OpenSearchClient#create should indeed throw OpenSearchException in this case?

[dblock]

We should probably do both. I don't see a reason why ResponseException wouldn't have status.