Expose BulkResponseItem.status in BulkFailureException

[ilya-ulyanov]

Hi,

I would like to propose a usability change in BulkFailureException class.

The class provides information on failed documents in form of Map {document._id->BulkResponseItem.reason}:

public class BulkFailureException extends DataRetrievalFailureException {
	private final Map<String, String> failedDocuments;

	public BulkFailureException(String msg, Map<String, String> failedDocuments) {
		super(msg);
		this.failedDocuments = failedDocuments;
	}

	public Map<String, String> getFailedDocuments() {
		return failedDocuments;
	}
}

The problem is that the reason is a string and it is problematic to reason about it when your client code needs to handle those failures in a specific way.

For instance in my project I need to have special handling of conflicts (when an attempt was done to save a document with the same or lower version, HTTP status 409) and currently I have to rely on string message from native elasticsearch library (which could change in the future btw)

It would be great to have the integer status of a particular failure.

Not to introduce a breaking change, I would imagine a new Map to be added as:

public class BulkFailureException extends DataRetrievalFailureException {
	private final Map<String, String> failedDocuments;
        private final Map<String, Integer> failedDocumentStatuses;

	public BulkFailureException(String msg, Map<String, String> failedDocuments, Map<String, Integer> failedDocumentStatuses) {
		super(msg);
		this.failedDocuments = failedDocuments;
                this.failedDocumentStatuses = failedDocumentStatuses;
	}

	public Map<String, String> getFailedDocuments() {
		return failedDocuments;
	}

       public Map<String, Integer> getFailedDocumentStatuses() {
		return failedDocumentStatuses;
	}
}

I could submit a PR if the above makes sense.

Thanks.

[sothawo]

First, having two maps with the same key and different values would be an ungly solution in my opinion. This would need t be a map with a value that combines the reason with something.

Second, where should this value come from? We get a co.elastic.clients.elasticsearch.core.BulkResponse object in the response. This has a property items of type co.elastic.clients.elasticsearch.core.bulk.BulkResponseItem. In that there is an error of type co.elastic.clients.elasticsearch._types.ErrorCause, which has this properties:

    private final Map<String, JsonData> metadata;
    @Nullable
    private final String type;
    @Nullable
    private final String reason;
    @Nullable
    private final String stackTrace;
    @Nullable
    private final ErrorCause causedBy;
    private final List<ErrorCause> rootCause;
    private final List<ErrorCause> suppressed;

This is below the level of HTTP status codes.

[ilya-ulyanov]

The value is taken from https://artifacts.elastic.co/javadoc/co/elastic/clients/elasticsearch-java/7.17.0/co/elastic/clients/elasticsearch/core/bulk/BulkResponseItem.html#status().

When it comes to how to put it - agree, it could be done as you said, but in that case the current getter would need to perform projection to keep the current API not broken.

Unless it is ok to change the API of getFailedDocuments to return something like Map<String, FailureStatus>, where FailureStatus is basically pair of HTTP status and error message.

[sothawo]

Ok, I checked in the error itself.

This is a breaking API change, therefore it could make it into 5.2, no backporting. And of course it needs to be documented in the release/migration docs.