-
Notifications
You must be signed in to change notification settings - Fork 25k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optionally require a valid content type for all rest requests with content #22691
Changes from 1 commit
e4b2d08
c53e296
9448729
7a267ff
3e78549
63ab38a
99f551f
76c9f5e
d4f0e13
b5d9e6f
221f797
f43bf3b
323e634
c6be540
8072a05
0be9af3
84db4dd
2391d2d
19cdb27
0264168
b18cae7
7e94108
65bfe9e
1d1d47e
7357e4e
88a1a56
842806a
26c6df8
a441458
4d37b1b
e208082
7b89522
d2a3c6f
5270a44
e4f46d3
4bd706a
dd19f71
9f862a6
4699539
fd62a34
9bbdd71
ed3ee08
159b450
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,12 +19,14 @@ | |
|
||
package org.elasticsearch.action.admin.cluster.storedscripts; | ||
|
||
import org.elasticsearch.Version; | ||
import org.elasticsearch.action.ActionRequestValidationException; | ||
import org.elasticsearch.action.support.master.AcknowledgedRequest; | ||
import org.elasticsearch.common.bytes.BytesReference; | ||
import org.elasticsearch.common.io.stream.StreamInput; | ||
import org.elasticsearch.common.io.stream.StreamOutput; | ||
import org.elasticsearch.common.xcontent.XContentHelper; | ||
import org.elasticsearch.common.xcontent.XContentType; | ||
|
||
import java.io.IOException; | ||
|
||
|
@@ -35,6 +37,7 @@ public class PutStoredScriptRequest extends AcknowledgedRequest<PutStoredScriptR | |
private String id; | ||
private String scriptLang; | ||
private BytesReference script; | ||
private XContentType xContentType; | ||
|
||
public PutStoredScriptRequest() { | ||
super(); | ||
|
@@ -92,17 +95,31 @@ public BytesReference script() { | |
return script; | ||
} | ||
|
||
public XContentType xContentType() { | ||
return xContentType; | ||
} | ||
|
||
@Deprecated | ||
public PutStoredScriptRequest script(BytesReference source) { | ||
this.script = source; | ||
return this; | ||
} | ||
|
||
public PutStoredScriptRequest script(BytesReference source, XContentType xContentType) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does it still make sense to store in the cluster state whatever format was sent? Maybe we should rather translate to json in the request like we do with mappings? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. forget about this comment, scripts are stored in json format, the conversion happens in ScriptMetaData, which is ok I think. we just seem to be sending out via transport whatever format we have, but the master will do the conversion before storing it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we may want to simplify things and move this conversion so we don't need to send over the content_type as part of this request, but up to you. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'll leave this one as is so we do not do a double parse of the script or move the ScriptMetaData logic into the request class (see the different ways stored scripts can be defined comment in the parseStoredScript method). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fine with me! |
||
this.script = source; | ||
this.xContentType = xContentType; | ||
return this; | ||
} | ||
|
||
@Override | ||
public void readFrom(StreamInput in) throws IOException { | ||
super.readFrom(in); | ||
scriptLang = in.readString(); | ||
id = in.readOptionalString(); | ||
script = in.readBytesReference(); | ||
if (in.getVersion().onOrAfter(Version.V_5_3_0_UNRELEASED) && in.readBoolean()) { | ||
xContentType = XContentType.readFrom(in); | ||
} | ||
} | ||
|
||
@Override | ||
|
@@ -111,13 +128,25 @@ public void writeTo(StreamOutput out) throws IOException { | |
out.writeString(scriptLang); | ||
out.writeOptionalString(id); | ||
out.writeBytesReference(script); | ||
if (out.getVersion().onOrAfter(Version.V_5_3_0_UNRELEASED)) { | ||
if (xContentType == null) { | ||
out.writeBoolean(false); | ||
} else { | ||
out.writeBoolean(true); | ||
xContentType.writeTo(out); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you fix the indentation here? |
||
} | ||
} | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
String sSource = "_na_"; | ||
try { | ||
sSource = XContentHelper.convertToJson(script, false); | ||
if (xContentType == null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you clarify that this if is only needed for bw comp and should go away once the bw comp layer is removed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm actually going to remove this and require a non-null xContentType. |
||
sSource = XContentHelper.convertToJson(script, false); | ||
} else { | ||
sSource = XContentHelper.convertToJson(script, false, xContentType); | ||
} | ||
} catch (Exception e) { | ||
// ignore | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ | |
import org.elasticsearch.action.support.master.AcknowledgedRequestBuilder; | ||
import org.elasticsearch.client.ElasticsearchClient; | ||
import org.elasticsearch.common.bytes.BytesReference; | ||
import org.elasticsearch.common.xcontent.XContentType; | ||
|
||
public class PutStoredScriptRequestBuilder extends AcknowledgedRequestBuilder<PutStoredScriptRequest, | ||
PutStoredScriptResponse, PutStoredScriptRequestBuilder> { | ||
|
@@ -40,9 +41,14 @@ public PutStoredScriptRequestBuilder setId(String id) { | |
return this; | ||
} | ||
|
||
@Deprecated | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. javadocs? |
||
public PutStoredScriptRequestBuilder setSource(BytesReference source) { | ||
request.script(source); | ||
return this; | ||
} | ||
|
||
public PutStoredScriptRequestBuilder setSource(BytesReference source, XContentType xContentType) { | ||
request.script(source, xContentType); | ||
return this; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -331,11 +331,20 @@ public CreateIndexRequest alias(Alias alias) { | |
|
||
/** | ||
* Sets the settings and mappings as a single source. | ||
* @deprecated use {@link #source(String, XContentType)} | ||
*/ | ||
@Deprecated | ||
public CreateIndexRequest source(String source) { | ||
return source(source.getBytes(StandardCharsets.UTF_8)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here too, let's change it to create new BytesArray directly? |
||
} | ||
|
||
/** | ||
* Sets the settings and mappings as a single source. | ||
*/ | ||
public CreateIndexRequest source(String source, XContentType xContentType) { | ||
return source(source.getBytes(StandardCharsets.UTF_8), xContentType); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe we could directly create new BytesArray(source) here instead |
||
} | ||
|
||
/** | ||
* Sets the settings and mappings as a single source. | ||
*/ | ||
|
@@ -345,23 +354,51 @@ public CreateIndexRequest source(XContentBuilder source) { | |
|
||
/** | ||
* Sets the settings and mappings as a single source. | ||
* @deprecated use {@link #source(byte[], XContentType)} | ||
*/ | ||
@Deprecated | ||
public CreateIndexRequest source(byte[] source) { | ||
return source(source, 0, source.length); | ||
} | ||
|
||
/** | ||
* Sets the settings and mappings as a single source. | ||
*/ | ||
public CreateIndexRequest source(byte[] source, XContentType xContentType) { | ||
return source(source, 0, source.length, xContentType); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what were we doing before? if content type is null we considered the source settings? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that we previously supported settings in properties format as part of the create index api request body? Is that it? Oh my.... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's correct. |
||
|
||
/** | ||
* Sets the settings and mappings as a single source. | ||
* @deprecated use {@link #source(byte[], int, int, XContentType)} | ||
*/ | ||
@Deprecated | ||
public CreateIndexRequest source(byte[] source, int offset, int length) { | ||
return source(new BytesArray(source, offset, length)); | ||
} | ||
|
||
/** | ||
* Sets the settings and mappings as a single source. | ||
*/ | ||
public CreateIndexRequest source(byte[] source, int offset, int length, XContentType xContentType) { | ||
return source(new BytesArray(source, offset, length), xContentType); | ||
} | ||
|
||
/** | ||
* Sets the settings and mappings as a single source. | ||
* @deprecated use {@link #source(BytesReference, XContentType)} | ||
*/ | ||
@Deprecated | ||
public CreateIndexRequest source(BytesReference source) { | ||
XContentType xContentType = XContentFactory.xContentType(source); | ||
source(source, xContentType); | ||
return this; | ||
} | ||
|
||
/** | ||
* Sets the settings and mappings as a single source. | ||
*/ | ||
public CreateIndexRequest source(BytesReference source, XContentType xContentType) { | ||
if (xContentType != null) { | ||
source(XContentHelper.convertToMap(source, false).v2()); | ||
} else { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,19 +22,24 @@ | |
import com.carrotsearch.hppc.ObjectHashSet; | ||
|
||
import org.elasticsearch.ElasticsearchGenerationException; | ||
import org.elasticsearch.Version; | ||
import org.elasticsearch.action.ActionRequestValidationException; | ||
import org.elasticsearch.action.IndicesRequest; | ||
import org.elasticsearch.action.support.IndicesOptions; | ||
import org.elasticsearch.action.support.master.AcknowledgedRequest; | ||
import org.elasticsearch.common.Strings; | ||
import org.elasticsearch.common.bytes.BytesArray; | ||
import org.elasticsearch.common.io.stream.StreamInput; | ||
import org.elasticsearch.common.io.stream.StreamOutput; | ||
import org.elasticsearch.common.xcontent.XContentBuilder; | ||
import org.elasticsearch.common.xcontent.XContentFactory; | ||
import org.elasticsearch.common.xcontent.XContentHelper; | ||
import org.elasticsearch.common.xcontent.XContentType; | ||
import org.elasticsearch.index.Index; | ||
|
||
import java.io.IOException; | ||
import java.io.UncheckedIOException; | ||
import java.nio.charset.StandardCharsets; | ||
import java.util.Arrays; | ||
import java.util.Map; | ||
import java.util.Objects; | ||
|
@@ -245,7 +250,7 @@ public static XContentBuilder buildFromSimplifiedDef(String type, Object... sour | |
*/ | ||
public PutMappingRequest source(XContentBuilder mappingBuilder) { | ||
try { | ||
return source(mappingBuilder.string()); | ||
return source(mappingBuilder.string(), mappingBuilder.contentType()); | ||
} catch (IOException e) { | ||
throw new IllegalArgumentException("Failed to build json for mapping request", e); | ||
} | ||
|
@@ -259,17 +264,30 @@ public PutMappingRequest source(Map mappingSource) { | |
try { | ||
XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON); | ||
builder.map(mappingSource); | ||
return source(builder.string()); | ||
return source(builder.string(), XContentType.JSON); | ||
} catch (IOException e) { | ||
throw new ElasticsearchGenerationException("Failed to generate [" + mappingSource + "]", e); | ||
} | ||
} | ||
|
||
/** | ||
* The mapping source definition. | ||
* @deprecated use {@link #source(String, XContentType)} | ||
*/ | ||
@Deprecated | ||
public PutMappingRequest source(String mappingSource) { | ||
this.source = mappingSource; | ||
return source(mappingSource, XContentFactory.xContentType(mappingSource)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not something that you changed but maybe something to fix as a followup, we have 4 xContentTypes, but given that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, the way I suggested to go before is totally different from what I am saying in this second review. That previous direction was probably the most correct but had a high cost (bw comp mainly). It's the only one that doesn't require any conversion, but just parsing in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should also add tests for all these different content types around mappings, otherwise it's all theories. I can look into it as a followup |
||
} | ||
|
||
/** | ||
* The mapping source definition. | ||
*/ | ||
public PutMappingRequest source(String mappingSource, XContentType xContentType) { | ||
try { | ||
this.source = XContentHelper.convertToJson(new BytesArray(mappingSource.getBytes(StandardCharsets.UTF_8)), false, xContentType); | ||
} catch (IOException e) { | ||
throw new UncheckedIOException("failed to convert mapping source to json", e); | ||
} | ||
return this; | ||
} | ||
|
||
|
@@ -290,7 +308,11 @@ public void readFrom(StreamInput in) throws IOException { | |
indices = in.readStringArray(); | ||
indicesOptions = IndicesOptions.readIndicesOptions(in); | ||
type = in.readOptionalString(); | ||
source = in.readString(); | ||
if (in.getVersion().onOrAfter(Version.V_5_3_0_UNRELEASED)) { | ||
source = in.readString(); | ||
} else { | ||
source = XContentHelper.convertToJson(new BytesArray(in.readString().getBytes(StandardCharsets.UTF_8)), false); | ||
} | ||
updateAllTypes = in.readBoolean(); | ||
readTimeout(in); | ||
concreteIndex = in.readOptionalWriteable(Index::new); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we add javadocs with the motivation for the deprecation and what it is replaced with?
I assume that these methods can be removed in master once this PR is backported to 5.x?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this can go away in master after backporting.