Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 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
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ public XContentType type() {
return XContentType.CBOR;
}

@Override
public boolean hasBulkSeparator() {
return false;
}

@Override
public byte bulkSeparator() {
throw new XContentParseException("cbor does not support bulk parsing...");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ public XContentType type() {
return XContentType.JSON;
}

@Override
public boolean hasBulkSeparator() {
return true;
}

@Override
public byte bulkSeparator() {
return '\n';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,11 @@ public byte bulkSeparator() {
return (byte) 0xFF;
}

@Override
public boolean hasBulkSeparator() {
return true;
}

@Override
public boolean detectContent(byte[] bytes, int offset, int length) {
return length > 2
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ public XContentType type() {
return XContentType.YAML;
}

@Override
public boolean hasBulkSeparator() {
return false;
}

@Override
public byte bulkSeparator() {
throw new UnsupportedOperationException("yaml does not support bulk parsing...");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,20 @@ public interface XContent {
*/
XContentType type();

/**
* @return {@code true} if this {@link XContent} can be sent in bulk, delimited by the byte returned by {@link #bulkSeparator()}, or
* {@code false} if this {@link XContent} does not support a delimited bulk format (in which case {@link #bulkSeparator()} throws an
* exception.
* <p>
* In practice, this is {@code true} for content with canonical type {@link XContentType#JSON} or {@link XContentType#SMILE} and
* {@link false} for content with canonical type {@link XContentType#CBOR} or {@link XContentType#YAML}.
*/
boolean hasBulkSeparator();

/**
* @return a {@link byte} that separates items in a bulk request that uses this {@link XContent}.
* @throws RuntimeException if this {@link XContent} does not support a delimited bulk format. See {@link #hasBulkSeparator()}.
*/
byte bulkSeparator();

@Deprecated
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

package org.elasticsearch.xcontent;

import org.elasticsearch.core.Nullable;
import org.elasticsearch.xcontent.cbor.CborXContent;
import org.elasticsearch.xcontent.json.JsonXContent;
import org.elasticsearch.xcontent.smile.SmileXContent;
Expand Down Expand Up @@ -287,4 +288,15 @@ public XContentType canonical() {
public static XContentType ofOrdinal(int ordinal) {
return values[ordinal];
}

/**
* Indicates if the {@link XContentType} (from the {@code Content-Type} header of a REST request) is compatible with delimited bulk
* content. A bulk request contains multiple objects each terminated with {@link XContent#bulkSeparator()}.
* <p>
* In practice, this returns {@code true} if the argument has canonical type {@link XContentType#JSON} or {@link XContentType#SMILE}
* and {@link false} if the argument is {@code null} or has canonical type {@link XContentType#CBOR} or {@link XContentType#YAML}.
*/
public static boolean supportsDelimitedBulkRequests(@Nullable XContentType xContentType) {
return xContentType != null && xContentType.xContent.hasBulkSeparator();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import org.elasticsearch.rest.action.RestToXContentListener;
import org.elasticsearch.rest.action.search.RestMultiSearchAction;
import org.elasticsearch.rest.action.search.RestSearchAction;
import org.elasticsearch.xcontent.XContentType;

import java.io.IOException;
import java.util.List;
Expand Down Expand Up @@ -95,8 +96,8 @@ public MultiSearchTemplateRequest parseRequest(RestRequest restRequest, boolean
}

@Override
public boolean supportsBulkContent() {
return true;
public boolean mediaTypesValid(RestRequest request) {
return super.mediaTypesValid(request) && XContentType.supportsDelimitedBulkRequests(request.getXContentType());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,6 @@ public boolean canTripCircuitBreaker() {
return delegate.canTripCircuitBreaker();
}

@Override
public boolean supportsBulkContent() {
return delegate.supportsBulkContent();
}

@Override
public boolean mediaTypesValid(RestRequest request) {
return delegate.mediaTypesValid(request);
Expand Down
14 changes: 0 additions & 14 deletions server/src/main/java/org/elasticsearch/rest/RestController.java
Original file line number Diff line number Diff line change
Expand Up @@ -421,20 +421,6 @@ private void dispatchRequest(
sendContentTypeErrorMessage(request.getAllHeaderValues("Content-Type"), channel);
return;
}
final XContentType xContentType = request.getXContentType();
// TODO consider refactoring to handler.supportsContentStream(xContentType). It is only used with JSON and SMILE
if (handler.supportsBulkContent()
&& XContentType.JSON != xContentType.canonical()
&& XContentType.SMILE != xContentType.canonical()) {
channel.sendResponse(
RestResponse.createSimpleErrorResponse(
channel,
RestStatus.NOT_ACCEPTABLE,
"Content-Type [" + xContentType + "] does not support stream parsing. Use JSON or SMILE instead"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new version will return a generic unsupported type error rather than "stream parsing" error, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it will say Content-Type header [...] is not supported instead, with a (technically incorrect) 406 Not Acceptable either way.

)
);
return;
}
}
RestChannel responseChannel = channel;
if (apiProtections.isEnabled()) {
Expand Down
10 changes: 0 additions & 10 deletions server/src/main/java/org/elasticsearch/rest/RestHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import org.elasticsearch.core.Nullable;
import org.elasticsearch.core.RestApiVersion;
import org.elasticsearch.rest.RestRequest.Method;
import org.elasticsearch.xcontent.XContent;

import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -44,15 +43,6 @@ default boolean supportsContentStream() {
return false;
}

/**
* Indicates if the RestHandler supports bulk content. A bulk request contains multiple objects
* delineated by {@link XContent#bulkSeparator()}. If a handler returns true this will affect
* the types of content that can be sent to this endpoint.
*/
default boolean supportsBulkContent() {
return false;
}

/**
* Returns the concrete RestHandler for this RestHandler. That is, if this is a delegating RestHandler it returns the delegate.
* Otherwise it returns itself.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.elasticsearch.rest.action.RestToXContentListener;
import org.elasticsearch.search.fetch.subphase.FetchSourceContext;
import org.elasticsearch.transport.Transports;
import org.elasticsearch.xcontent.XContentType;

import java.io.IOException;
import java.util.ArrayDeque;
Expand Down Expand Up @@ -303,12 +304,12 @@ private ArrayList<Releasable> accountParsing(int bytesConsumed) {
}

@Override
public boolean supportsBulkContent() {
return true;
public Set<String> supportedCapabilities() {
return capabilities;
}

@Override
public Set<String> supportedCapabilities() {
return capabilities;
public boolean mediaTypesValid(RestRequest request) {
return super.mediaTypesValid(request) && XContentType.supportsDelimitedBulkRequests(request.getXContentType());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,8 @@ public static void parseMultiLineRequest(
}

@Override
public boolean supportsBulkContent() {
return true;
public boolean mediaTypesValid(RestRequest request) {
return super.mediaTypesValid(request) && XContentType.supportsDelimitedBulkRequests(request.getXContentType());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@

import static org.mockito.Mockito.inOrder;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

/**
* Tests {@link DeprecationRestHandler}.
Expand Down Expand Up @@ -156,22 +155,6 @@ public void testInvalidHeaderValueEmpty() {
expectThrows(IllegalArgumentException.class, () -> DeprecationRestHandler.requireValidHeader(blank));
}

public void testSupportsBulkContentTrue() {
when(handler.supportsBulkContent()).thenReturn(true);
assertTrue(
new DeprecationRestHandler(handler, METHOD, PATH, Level.WARN, deprecationMessage, deprecationLogger, false)
.supportsBulkContent()
);
}

public void testSupportsBulkContentFalse() {
when(handler.supportsBulkContent()).thenReturn(false);
assertFalse(
new DeprecationRestHandler(handler, METHOD, PATH, Level.WARN, deprecationMessage, deprecationLogger, false)
.supportsBulkContent()
);
}

public void testDeprecationLevel() {
DeprecationRestHandler handler = new DeprecationRestHandler(
this.handler,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -640,8 +640,8 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c
}

@Override
public boolean supportsBulkContent() {
return true;
public boolean mediaTypesValid(RestRequest request) {
return RestHandler.super.mediaTypesValid(request) && XContentType.supportsDelimitedBulkRequests(request.getXContentType());
}
});

Expand Down Expand Up @@ -679,8 +679,8 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c
}

@Override
public boolean supportsBulkContent() {
return true;
public boolean mediaTypesValid(RestRequest request) {
return RestHandler.super.mediaTypesValid(request) && XContentType.supportsDelimitedBulkRequests(request.getXContentType());
}
});

Expand All @@ -704,8 +704,8 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c
}

@Override
public boolean supportsBulkContent() {
return true;
public boolean mediaTypesValid(RestRequest request) {
return RestHandler.super.mediaTypesValid(request) && XContentType.supportsDelimitedBulkRequests(request.getXContentType());
}
});

Expand All @@ -730,8 +730,8 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c
}

@Override
public boolean supportsBulkContent() {
return true;
public boolean mediaTypesValid(RestRequest request) {
return RestHandler.super.mediaTypesValid(request) && XContentType.supportsDelimitedBulkRequests(request.getXContentType());
}
});
assertFalse(channel.getSendResponseCalled());
Expand All @@ -755,8 +755,8 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c
}

@Override
public boolean supportsBulkContent() {
return true;
public boolean mediaTypesValid(RestRequest request) {
return RestHandler.super.mediaTypesValid(request) && XContentType.supportsDelimitedBulkRequests(request.getXContentType());
}
});
assertFalse(channel.getSendResponseCalled());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.elasticsearch.rest.RestRequest;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.rest.action.RestToXContentListener;
import org.elasticsearch.xcontent.XContentType;
import org.elasticsearch.xpack.core.ml.action.PostDataAction;
import org.elasticsearch.xpack.core.ml.job.config.Job;

Expand Down Expand Up @@ -54,7 +55,7 @@ protected RestChannelConsumer prepareRequest(RestRequest restRequest, NodeClient
}

@Override
public boolean supportsBulkContent() {
return true;
public boolean mediaTypesValid(RestRequest request) {
return super.mediaTypesValid(request) && XContentType.supportsDelimitedBulkRequests(request.getXContentType());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.elasticsearch.rest.RestResponse;
import org.elasticsearch.rest.action.RestBuilderListener;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.XContentType;
import org.elasticsearch.xpack.core.monitoring.MonitoredSystem;
import org.elasticsearch.xpack.core.monitoring.action.MonitoringBulkRequestBuilder;
import org.elasticsearch.xpack.core.monitoring.action.MonitoringBulkResponse;
Expand Down Expand Up @@ -100,8 +101,8 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client
}

@Override
public boolean supportsBulkContent() {
return true;
public boolean mediaTypesValid(RestRequest request) {
return super.mediaTypesValid(request) && XContentType.supportsDelimitedBulkRequests(request.getXContentType());
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.elasticsearch.xpack.core.monitoring.exporter.MonitoringTemplateUtils;

import java.util.HashMap;
import java.util.List;
import java.util.Map;

import static org.elasticsearch.xpack.core.monitoring.exporter.MonitoringTemplateUtils.TEMPLATE_VERSION;
Expand All @@ -47,7 +48,18 @@ public void testGetName() {

public void testSupportsBulkContent() {
// if you change this, it's a very breaking change for Monitoring
assertThat(action.supportsBulkContent(), is(true));
final var route = action.routes().get(0);
for (var xContentType : XContentType.values()) {
final var request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(route.getMethod())
.withPath(route.getPath())
.withHeaders(Map.of("Content-Type", List.of(xContentType.mediaType())))
.build();
assertEquals(
xContentType.toString(),
XContentType.supportsDelimitedBulkRequests(request.getXContentType()),
action.mediaTypesValid(request)
);
}
}

public void testMissingSystemId() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
import org.elasticsearch.rest.RestResponse;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.rest.FakeRestRequest;
import org.elasticsearch.xcontent.XContentType;
import org.elasticsearch.xcontent.json.JsonXContent;
import org.elasticsearch.xpack.core.monitoring.action.MonitoringMigrateAlertsResponse;
import org.elasticsearch.xpack.core.monitoring.action.MonitoringMigrateAlertsResponse.ExporterMigrationResult;
Expand All @@ -20,6 +22,7 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;

import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.startsWith;
Expand All @@ -34,8 +37,15 @@ public void testGetName() {
assertThat(action.getName(), is("monitoring_migrate_alerts"));
}

public void testSupportsContentStream() {
assertThat(action.supportsBulkContent(), is(false));
public void testSupportsAllContentTypes() {
final var route = action.routes().get(0);
for (var xContentType : XContentType.values()) {
final var request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(route.getMethod())
.withPath(route.getPath())
.withHeaders(Map.of("Content-Type", List.of(xContentType.mediaType())))
.build();
assertTrue(xContentType.toString(), action.mediaTypesValid(request));
}
}

public void testRestActionCompletion() throws Exception {
Expand Down