[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.

[reta]

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

Agree, at least it is worth looking, the reasons why ResponseException may not have status could be the connection issues, fe no host route or connection refused (basically anything that does not return the response), I don't remember from the top of my head where (and why) we use ResponseException

[rursprung]

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

i've created #756 to expose ResponseException#status

but i'm unsure how best to ensure that OpenSearchClient#create returns an OpenSearchException - i don't think that it's specific to this API: it just calls Transport#performRequest underneath and the only place in the ApacheHttpClient5Transport implementation which actually throws a OpenSearchException is this one:

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

Line 503 in 9213138

throw new OpenSearchException((ErrorResponse) error);

everything else is a TransportException (usually wrapping a ResponseException) or, IOException or similar (didn't check all of them). the story seems to be similar for the old RestClientTransport.
so while every API seems to claim that it throws a OpenSearchException (something which it technically doesn't have to do as it's a RuntimeException) i don't think that this is what's usually being thrown. changing that might even be considered a breaking change if someone explicitly catches a RuntimeException (or IOException or similar) at the moment to somehow handle that case.

as you're far more familiar with this library (i only just started to use it) you can probably judge better what the proper approach is. i'd suggest to maybe split this part out into a separate issue, where you clearly formulate the target picture and the expected impact.

[reta]

i've created #756 to expose ResponseException#status

Thanks @rursprung , I think this is good change in any case, thank you.

as you're far more familiar with this library (i only just started to use it) you can probably judge better what the proper approach is. i'd suggest to maybe split this part out into a separate issue, where you clearly formulate the target picture and the expected impact.

Makes perfect sense, thank you.

[dblock]

With #756 merged, I renamed this issue to "Throw OpenSearchException on 409 Conflict" which I think represents the feature request we discussed above. Feel free to edit/close/reopen as you see fit!

[rursprung]

@dblock: do you only want to change it from a ResponseException to an OpenSearchException in case of a 409 (as suggested by the title)? or in all cases? i think what's missing is a definition of what should be an OpenSearchException and what should be something else (the API definition suggests that everything would be an OpenSearchException (except low-level network errors, which i'd expect to be an IOException as this is the other exception type being specified))

[dblock]

I think when OpenSearch is generating a error response body it should be an OpenSearchException, and everything that's a transport problem should be an IOException. So does it make sense to have a ResponseException at all?

Maybe everything should inherit fromIOException and be increasingly specialized? So OpenSearchException <- HttpException <- IOException?

[rursprung]

i think that sounds reasonable. kicking out ResponseException would probably be a breaking change? the other changes probably not so

[dblock]

i think that sounds reasonable. kicking out ResponseException would probably be a breaking change? the other changes probably not so

Right, so proceed with caution.

[rursprung]

then i'd suggest to either rename the issue again (i'll abstain from doing it myself to make sure that this now reflects your plans) or raise a new one (or two: one for everything doable without a breaking change and one for the breaking change to be done later) and clearly outline what should be done and at which point. is there a release planning to know when a major release is planned / is ok to happen?

[dblock]

We theoretically can make a major release anytime. I think I'd want to look at a PR and see if it's really worth it, and an UPGRADING.md that explains how it changes to users. I know it's potentially throw away work, but I think we all agree that changing exceptions being raised is kinda a big deal (TM).