Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/120725.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 120725
summary: Add common query param `include_source_on_error`
area: Infra/REST API
type: enhancement
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ public class XContentParserConfigurationImpl implements XContentParserConfigurat
RestApiVersion.current(),
null,
null,
false
false,
true
);

final NamedXContentRegistry registry;
Expand All @@ -40,21 +41,45 @@ public class XContentParserConfigurationImpl implements XContentParserConfigurat
final FilterPath[] includes;
final FilterPath[] excludes;
final boolean filtersMatchFieldNamesWithDots;
final boolean includeSourceOnError;

private XContentParserConfigurationImpl(
NamedXContentRegistry registry,
DeprecationHandler deprecationHandler,
RestApiVersion restApiVersion,
FilterPath[] includes,
FilterPath[] excludes,
boolean filtersMatchFieldNamesWithDots
boolean filtersMatchFieldNamesWithDots,
boolean includeSourceOnError
) {
this.registry = registry;
this.deprecationHandler = deprecationHandler;
this.restApiVersion = restApiVersion;
this.includes = includes;
this.excludes = excludes;
this.filtersMatchFieldNamesWithDots = filtersMatchFieldNamesWithDots;
this.includeSourceOnError = includeSourceOnError;
}

@Override
public boolean includeSourceOnError() {
return includeSourceOnError;
}

@Override
public XContentParserConfiguration withIncludeSourceOnError(boolean includeSourceOnError) {
if (includeSourceOnError == this.includeSourceOnError) {
return this;
}
return new XContentParserConfigurationImpl(
registry,
deprecationHandler,
restApiVersion,
includes,
excludes,
filtersMatchFieldNamesWithDots,
includeSourceOnError
);
}

@Override
Expand All @@ -65,7 +90,8 @@ public XContentParserConfigurationImpl withRegistry(NamedXContentRegistry regist
restApiVersion,
includes,
excludes,
filtersMatchFieldNamesWithDots
filtersMatchFieldNamesWithDots,
includeSourceOnError
);
}

Expand All @@ -80,7 +106,8 @@ public XContentParserConfiguration withDeprecationHandler(DeprecationHandler dep
restApiVersion,
includes,
excludes,
filtersMatchFieldNamesWithDots
filtersMatchFieldNamesWithDots,
includeSourceOnError
);
}

Expand All @@ -95,7 +122,8 @@ public XContentParserConfiguration withRestApiVersion(RestApiVersion restApiVers
restApiVersion,
includes,
excludes,
filtersMatchFieldNamesWithDots
filtersMatchFieldNamesWithDots,
includeSourceOnError
);
}

Expand Down Expand Up @@ -143,7 +171,8 @@ public XContentParserConfiguration withFiltering(
restApiVersion,
includePaths,
excludePaths,
filtersMatchFieldNamesWithDots
filtersMatchFieldNamesWithDots,
includeSourceOnError
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,23 +87,30 @@ public XContentGenerator createGenerator(OutputStream os, Set<String> includes,
return new JsonXContentGenerator(jsonFactory.createGenerator(os, JsonEncoding.UTF8), os, includes, excludes);
}

private XContentParser createParser(XContentParserConfiguration config, JsonParser parser) {
if (config.includeSourceOnError() == false) {
parser.disable(JsonParser.Feature.INCLUDE_SOURCE_IN_LOCATION); // enabled by default, disable if requested
}
return new JsonXContentParser(config, parser);
}
Comment thread
prdoyle marked this conversation as resolved.

@Override
public XContentParser createParser(XContentParserConfiguration config, String content) throws IOException {
return new JsonXContentParser(config, jsonFactory.createParser(content));
return createParser(config, jsonFactory.createParser(content));
}

@Override
public XContentParser createParser(XContentParserConfiguration config, InputStream is) throws IOException {
return new JsonXContentParser(config, jsonFactory.createParser(is));
return createParser(config, jsonFactory.createParser(is));
}

@Override
public XContentParser createParser(XContentParserConfiguration config, byte[] data, int offset, int length) throws IOException {
return new JsonXContentParser(config, jsonFactory.createParser(data, offset, length));
return createParser(config, jsonFactory.createParser(data, offset, length));
}

@Override
public XContentParser createParser(XContentParserConfiguration config, Reader reader) throws IOException {
return new JsonXContentParser(config, jsonFactory.createParser(reader));
return createParser(config, jsonFactory.createParser(reader));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ public interface XContentParserConfiguration {
*/
XContentParserConfiguration EMPTY = XContentProvider.provider().empty();

/**
* Disable to not include the source in case of parsing errors (defaults to true).
*/
XContentParserConfiguration withIncludeSourceOnError(boolean includeSourceOnError);

boolean includeSourceOnError();

/**
* Replace the registry backing {@link XContentParser#namedObject}.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.elasticsearch.xcontent.json.JsonXContent;

import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
Expand All @@ -33,6 +34,7 @@
import static org.hamcrest.Matchers.in;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.nullValue;
import static org.junit.internal.matchers.ThrowableMessageMatcher.hasMessage;

Expand Down Expand Up @@ -655,6 +657,25 @@ public void testCreateRootSubParser() throws IOException {

}

public void testJsonIncludeSourceOnParserError() throws IOException {
var xContent = XContentFactory.xContent(XContentType.JSON);
var source = "{\"field\": invalid}"; // causes parse exception
var sourceEnabled = XContentParserConfiguration.EMPTY;
var sourceDisabled = XContentParserConfiguration.EMPTY.withIncludeSourceOnError(false);

var parseException = assertThrows(XContentParseException.class, () -> createParser(xContent, sourceEnabled, source).map());
Comment thread
mosche marked this conversation as resolved.
Outdated
assertThat(parseException.getMessage(), containsString(source));

parseException = assertThrows(XContentParseException.class, () -> createParser(xContent, sourceDisabled, source).map());
assertThat(parseException.getMessage(), not(containsString(source)));
}

private XContentParser createParser(XContent xContent, XContentParserConfiguration config, String content) throws IOException {
return randomBoolean()
? xContent.createParser(config, content)
: xContent.createParser(config, content.getBytes(StandardCharsets.UTF_8));
}

/**
* Generates a random object {"first_field": "foo", "marked_field": {...random...}, "last_field": "bar}
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

package org.elasticsearch.rest.action.document;

import org.elasticsearch.client.Request;
import org.elasticsearch.client.Response;
import org.elasticsearch.client.ResponseException;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.io.Streams;
import org.elasticsearch.rest.RestUtils;
import org.elasticsearch.test.ESIntegTestCase;

import java.io.InputStreamReader;

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.hamcrest.Matchers.both;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.not;

public class RestBulkActionIT extends ESIntegTestCase {
@Override
protected boolean addMockHttpTransport() {
return false;
}

public void testBulkIndexWithSourceOnErrorDisabled() throws Exception {
var source = "{\"field\": \"index\",}";
var sourceEscaped = "{\\\"field\\\": \\\"index\\\",}";

var request = new Request("PUT", "/test_index/_bulk");
request.setJsonEntity(Strings.format("{\"index\":{\"_id\":\"1\"}}\n%s\n", source));

Response response = getRestClient().performRequest(request);
String responseContent = Streams.copyToString(new InputStreamReader(response.getEntity().getContent(), UTF_8));
assertThat(responseContent, containsString(sourceEscaped));

request.addParameter(RestUtils.INCLUDE_SOURCE_ON_ERROR_PARAMETER, "false");

response = getRestClient().performRequest(request);
responseContent = Streams.copyToString(new InputStreamReader(response.getEntity().getContent(), UTF_8));
assertThat(
responseContent,
both(not(containsString(sourceEscaped))).and(
containsString("REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled)")
)
);
}

public void testBulkUpdateWithSourceOnErrorDisabled() throws Exception {
var source = "{\"field\": \"index\",}";
var sourceEscaped = "{\\\"field\\\": \\\"index\\\",}";

var request = new Request("PUT", "/test_index/_bulk");
request.addParameter(RestUtils.INCLUDE_SOURCE_ON_ERROR_PARAMETER, "false");
request.setJsonEntity(Strings.format("{\"update\":{\"_id\":\"1\"}}\n{\"doc\":%s}}\n", source));

// note: this behavior is not consistent with bulk index actions
// In case of updates by doc, the source is eagerly parsed and will fail the entire request if it cannot be parsed
var exception = assertThrows(ResponseException.class, () -> getRestClient().performRequest(request));
String response = Streams.copyToString(new InputStreamReader(exception.getResponse().getEntity().getContent(), UTF_8));

assertThat(
response,
both(not(containsString(sourceEscaped))).and(
containsString("REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled)")
)
);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

package org.elasticsearch.rest.action.document;

import org.elasticsearch.client.Request;
import org.elasticsearch.client.ResponseException;
import org.elasticsearch.common.io.Streams;
import org.elasticsearch.rest.RestUtils;
import org.elasticsearch.test.ESIntegTestCase;

import java.io.InputStreamReader;

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.hamcrest.Matchers.both;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.not;

public class RestIndexActionIT extends ESIntegTestCase {
@Override
protected boolean addMockHttpTransport() {
return false;
}

public void testIndexWithSourceOnErrorDisabled() throws Exception {
var source = "{\"field\": \"value}";
var sourceEscaped = "{\\\"field\\\": \\\"value}";

var request = new Request("POST", "/test_index/_doc/1");
request.setJsonEntity(source);

var exception = assertThrows(ResponseException.class, () -> getRestClient().performRequest(request));
String response = Streams.copyToString(new InputStreamReader(exception.getResponse().getEntity().getContent(), UTF_8));
assertThat(response, containsString(sourceEscaped));

// disable source on error
request.addParameter(RestUtils.INCLUDE_SOURCE_ON_ERROR_PARAMETER, "false");
exception = assertThrows(ResponseException.class, () -> getRestClient().performRequest(request));
response = Streams.copyToString(new InputStreamReader(exception.getResponse().getEntity().getContent(), UTF_8));
assertThat(
response,
both(not(containsString(sourceEscaped))).and(
containsString("REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled)")
)
);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

package org.elasticsearch.rest.action.document;

import org.elasticsearch.client.Request;
import org.elasticsearch.client.ResponseException;
import org.elasticsearch.common.io.Streams;
import org.elasticsearch.rest.RestUtils;
import org.elasticsearch.test.ESIntegTestCase;

import java.io.InputStreamReader;

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.hamcrest.Matchers.both;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.not;

public class RestUpdateActionIT extends ESIntegTestCase {
@Override
protected boolean addMockHttpTransport() {
return false;
}

public void testUpdateByDocWithSourceOnErrorDisabled() throws Exception {
var updateRequest = "{\"doc\":{\"field\": \"value}}";
var sourceEscaped = "{\\\"field\\\": \\\"value}";

var request = new Request("POST", "/test_index/_update/1");
request.setJsonEntity(updateRequest);

var exception = assertThrows(ResponseException.class, () -> getRestClient().performRequest(request));
String response = Streams.copyToString(new InputStreamReader(exception.getResponse().getEntity().getContent(), UTF_8));
assertThat(response, containsString(sourceEscaped));

// disable source on error
request.addParameter(RestUtils.INCLUDE_SOURCE_ON_ERROR_PARAMETER, "false");
exception = assertThrows(ResponseException.class, () -> getRestClient().performRequest(request));
response = Streams.copyToString(new InputStreamReader(exception.getResponse().getEntity().getContent(), UTF_8));
assertThat(
response,
both(not(containsString(sourceEscaped))).and(
containsString("REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled)")
)
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ static TransportVersion def(int id) {
public static final TransportVersion ADD_INCLUDE_FAILURE_INDICES_OPTION = def(8_831_00_0);
public static final TransportVersion ESQL_RESPONSE_PARTIAL = def(8_832_00_0);
public static final TransportVersion RANK_DOC_OPTIONAL_METADATA_FOR_EXPLAIN = def(8_833_00_0);
public static final TransportVersion INGEST_REQUEST_INCLUDE_SOURCE_ON_ERROR = def(8_834_00_0);

/*
* STOP! READ THIS FIRST! No, really,
Expand Down
Loading