Skip to content
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

Require MediaType in Strings.toString API #6009

Merged
merged 4 commits into from
Jan 25, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- Migrate client transports to Apache HttpClient / Core 5.x ([#4459](https://github.com/opensearch-project/OpenSearch/pull/4459))
- Changed http code on create index API with bad input raising NotXContentException from 500 to 400 ([#4773](https://github.com/opensearch-project/OpenSearch/pull/4773))
- Change http code for DecommissioningFailedException from 500 to 400 ([#5283](https://github.com/opensearch-project/OpenSearch/pull/5283))
- Require MediaType in Strings.toString API ([#6009](https://github.com/opensearch-project/OpenSearch/pull/6009))

### Deprecated

Expand Down Expand Up @@ -98,4 +99,4 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
### Security

[Unreleased 3.0]: https://github.com/opensearch-project/OpenSearch/compare/2.x...HEAD
[Unreleased 2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.5...2.x
[Unreleased 2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.5...2.x
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.opensearch.common.xcontent.ToXContentObject;
import org.opensearch.common.xcontent.XContentBuilder;
import org.opensearch.common.xcontent.XContentParser;
import org.opensearch.common.xcontent.XContentType;

import java.io.IOException;
import java.util.Map;
Expand Down Expand Up @@ -168,6 +169,6 @@ public boolean equals(Object obj) {

@Override
public String toString() {
return Strings.toString(this);
return Strings.toString(XContentType.JSON, this);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.opensearch.common.xcontent.ToXContentObject;
import org.opensearch.common.xcontent.XContentBuilder;
import org.opensearch.common.xcontent.XContentParser;
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.snapshots.SnapshotId;

import java.io.IOException;
Expand Down Expand Up @@ -288,7 +289,7 @@ public boolean equals(Object obj) {

@Override
public String toString() {
return Strings.toString(this);
return Strings.toString(XContentType.JSON, this);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import org.opensearch.common.xcontent.ToXContentObject;
import org.opensearch.common.xcontent.XContentBuilder;
import org.opensearch.common.xcontent.XContentParser;
import org.opensearch.common.xcontent.XContentType;

import java.io.IOException;
import java.util.Collections;
Expand Down Expand Up @@ -187,7 +188,7 @@ public boolean equals(Object obj) {

@Override
public String toString() {
return Strings.toString(this);
return Strings.toString(XContentType.JSON, this);
}

public static class SnapshotPolicyStats implements ToXContentFragment {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import org.opensearch.common.xcontent.ToXContentObject;
import org.opensearch.common.xcontent.XContentBuilder;
import org.opensearch.common.xcontent.XContentParser;
import org.opensearch.common.xcontent.XContentType;

import java.io.IOException;
import java.util.Objects;
Expand Down Expand Up @@ -150,6 +151,6 @@ public boolean equals(Object obj) {

@Override
public String toString() {
return Strings.toString(this);
return Strings.toString(XContentType.JSON, this);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.opensearch.client.Response;
import org.opensearch.client.ResponseException;
import org.opensearch.common.Strings;
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.search.aggregations.AggregationBuilders;
import org.opensearch.search.builder.SearchSourceBuilder;

Expand Down Expand Up @@ -171,8 +172,8 @@ public void testAggregationUsage() throws IOException {
.aggregation(AggregationBuilders.terms("str_terms").field("str.keyword"))
.aggregation(AggregationBuilders.terms("num_terms").field("num"))
.aggregation(AggregationBuilders.avg("num_avg").field("num"));
searchRequest.setJsonEntity(Strings.toString(searchSource));
searchRequest.setJsonEntity(Strings.toString(searchSource));
searchRequest.setJsonEntity(Strings.toString(XContentType.JSON, searchSource));
searchRequest.setJsonEntity(Strings.toString(XContentType.JSON, searchSource));
client().performRequest(searchRequest);

searchRequest = new Request("GET", "/test/_search");
Expand All @@ -181,8 +182,8 @@ public void testAggregationUsage() throws IOException {
.aggregation(AggregationBuilders.avg("num1").field("num"))
.aggregation(AggregationBuilders.avg("num2").field("num"))
.aggregation(AggregationBuilders.terms("foo").field("foo.keyword"));
String r = Strings.toString(searchSource);
searchRequest.setJsonEntity(Strings.toString(searchSource));
String r = Strings.toString(XContentType.JSON, searchSource);
searchRequest.setJsonEntity(Strings.toString(XContentType.JSON, searchSource));
client().performRequest(searchRequest);

Response response = client().performRequest(new Request("GET", "_nodes/usage"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,6 @@ public interface MediaType {
default String typeWithSubtype() {
return type() + "/" + subtype();
}

XContent xContent();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just came to mind, do you think we could introduce XContentMediaType to break XContentType and MediaType:

interface XContentMediaType extends MediaType {
     XContent xContent()
}

And than we would have XContentType implements XContentMediaType ?
Sorry about that, didn't see that before.

Copy link
Collaborator Author

@nknize nknize Jan 25, 2023

Choose a reason for hiding this comment

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

Are you then wanting to keep the XContent base classes in the x-content library instead of refactoring like #5902? If so, we would still need to tease out those classes because so much of the gaggregation and field mappers framework depend on XContent base classes.

If not, then this doesn't buy us anything until we refactor the entire :server codebase to be less dependent on XContent and more MediaType generic

Update: Working backwards, these in-flight refactor PRs I've been opening are trying to move toward decoupling large functionality like the gaggregation and field mapper framework from the :server module

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I wanted to keep MediaType clean, right now it won't buy us anything but later the XContent won't be an obstacle going to "more MediaType generic" stage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's add that abstraction in a follow up PR then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can take it, btw with XContentMediaType we could backport it to 2.x as well, cuz MediaType won't be change and XContentType as well :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry. I didn't clarify well. We should add it in a follow up PR that makes :server less dependent on XContentType. If we do this now it will block refactoring the Strings class to core which is a core class needed to refactor a LOT of other base classes out of :server.

}
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,6 @@ public String mediaType() {
return mediaTypeWithoutParameters();
}

public abstract XContent xContent();

public abstract String mediaTypeWithoutParameters();

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,6 @@ public int hashCode() {

@Override
public String toString() {
return Strings.toString(this);
return Strings.toString(XContentType.JSON, this);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import org.opensearch.common.xcontent.ToXContentObject;
import org.opensearch.common.xcontent.XContentBuilder;
import org.opensearch.common.xcontent.XContentParser;
import org.opensearch.common.xcontent.XContentType;

import java.io.IOException;
import java.util.Arrays;
Expand Down Expand Up @@ -199,6 +200,6 @@ public static MultiSearchTemplateResponse fromXContext(XContentParser parser) {

@Override
public String toString() {
return Strings.toString(this);
return Strings.toString(XContentType.JSON, this);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
import org.opensearch.common.Strings;
import org.opensearch.common.lucene.search.MultiPhrasePrefixQuery;
import org.opensearch.common.xcontent.XContentBuilder;
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.index.IndexSettings;
import org.opensearch.index.analysis.AnalyzerScope;
import org.opensearch.index.analysis.IndexAnalyzers;
Expand Down Expand Up @@ -609,15 +610,15 @@ public void testAnalyzerSerialization() throws IOException {
b.field("type", "search_as_you_type");
b.field("analyzer", "simple");
}));
String serialized = Strings.toString(ms.documentMapper());
String serialized = Strings.toString(XContentType.JSON, ms.documentMapper());
assertEquals(
serialized,
"{\"_doc\":{\"properties\":{\"field\":"
+ "{\"type\":\"search_as_you_type\",\"doc_values\":false,\"max_shingle_size\":3,\"analyzer\":\"simple\"}}}}"
);

merge(ms, mapping(b -> {}));
assertEquals(serialized, Strings.toString(ms.documentMapper()));
assertEquals(serialized, Strings.toString(XContentType.JSON, ms.documentMapper()));
}

private void documentParsingTestCase(Collection<String> values) throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import org.opensearch.common.xcontent.XContentBuilder;
import org.opensearch.common.xcontent.XContentParser;
import org.opensearch.common.xcontent.XContentParserUtils;
import org.opensearch.common.xcontent.XContentType;

import java.io.IOException;
import java.util.Collections;
Expand Down Expand Up @@ -105,7 +106,7 @@ public Map<String, Exception> getFailures() {

@Override
public String toString() {
return Strings.toString(this);
return Strings.toString(XContentType.JSON, this);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import org.opensearch.common.xcontent.XContentBuilder;
import org.opensearch.common.xcontent.XContentParser;
import org.opensearch.common.xcontent.XContentParserUtils;
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.script.Script;

import java.io.IOException;
Expand Down Expand Up @@ -249,7 +250,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws

@Override
public String toString() {
return Strings.toString(this);
return Strings.toString(XContentType.JSON, this);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import org.opensearch.common.xcontent.ToXContentObject;
import org.opensearch.common.xcontent.XContentBuilder;
import org.opensearch.common.xcontent.XContentParser;
import org.opensearch.common.xcontent.XContentType;

import java.io.IOException;
import java.util.Objects;
Expand Down Expand Up @@ -128,7 +129,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws

@Override
public String toString() {
return Strings.toString(this);
return Strings.toString(XContentType.JSON, this);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import org.opensearch.common.xcontent.ToXContentObject;
import org.opensearch.common.xcontent.XContentBuilder;
import org.opensearch.common.xcontent.XContentParser;
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.index.rankeval.RatedDocument.DocumentKey;
import org.opensearch.search.builder.SearchSourceBuilder;

Expand Down Expand Up @@ -340,7 +341,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws

@Override
public String toString() {
return Strings.toString(this);
return Strings.toString(XContentType.JSON, this);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -323,10 +323,13 @@ public void testMetricDetails() {
+ ",\"unrated_docs\":"
+ unratedDocs
+ "}}",
Strings.toString(detail)
Strings.toString(XContentType.JSON, detail)
);
} else {
assertEquals("{\"dcg\":{\"dcg\":" + dcg + ",\"unrated_docs\":" + unratedDocs + "}}", Strings.toString(detail));
assertEquals(
"{\"dcg\":{\"dcg\":" + dcg + ",\"unrated_docs\":" + unratedDocs + "}}",
Strings.toString(XContentType.JSON, detail)
);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import org.opensearch.common.Strings;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.xcontent.XContentBuilder;
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.common.xcontent.json.JsonXContent;
import org.opensearch.common.xcontent.support.XContentMapValues;
import org.opensearch.index.IndexSettings;
Expand Down Expand Up @@ -1364,7 +1365,7 @@ public void testResize() throws Exception {
if (randomBoolean()) {
settings.put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), true);
}
shrinkRequest.setJsonEntity("{\"settings\":" + Strings.toString(settings.build()) + "}");
shrinkRequest.setJsonEntity("{\"settings\":" + Strings.toString(XContentType.JSON, settings.build()) + "}");
client().performRequest(shrinkRequest);
ensureGreenLongWait(target);
assertNumHits(target, numDocs + moreDocs, 1);
Expand All @@ -1376,7 +1377,7 @@ public void testResize() throws Exception {
settings.put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), true);
}
Request splitRequest = new Request("PUT", "/" + index + "/_split/" + target);
splitRequest.setJsonEntity("{\"settings\":" + Strings.toString(settings.build()) + "}");
splitRequest.setJsonEntity("{\"settings\":" + Strings.toString(XContentType.JSON, settings.build()) + "}");
client().performRequest(splitRequest);
ensureGreenLongWait(target);
assertNumHits(target, numDocs + moreDocs, 6);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -726,7 +726,7 @@ public void testCompletionSuggester() throws Exception {
sourceBuilder.suggest(suggestBuilder);
duelSearch(searchRequest, response -> {
assertMultiClusterSearchResponse(response);
assertEquals(Strings.toString(response, true, true), 3, response.getSuggest().size());
assertEquals(Strings.toString(XContentType.JSON, response, true, true), 3, response.getSuggest().size());
assertThat(response.getSuggest().getSuggestion("python").getEntries().size(), greaterThan(0));
assertThat(response.getSuggest().getSuggestion("java").getEntries().size(), greaterThan(0));
assertThat(response.getSuggest().getSuggestion("ruby").getEntries().size(), greaterThan(0));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@

package org.opensearch.upgrades;

import org.opensearch.LegacyESVersion;
import org.opensearch.Version;
import org.opensearch.action.support.PlainActionFuture;
import org.opensearch.client.Request;
Expand All @@ -44,13 +43,13 @@
import org.opensearch.common.Strings;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.util.concurrent.AbstractRunnable;
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.common.xcontent.support.XContentMapValues;
import org.opensearch.index.IndexSettings;
import org.opensearch.index.mapper.MapperService;
import org.opensearch.rest.RestStatus;
import org.opensearch.test.rest.yaml.ObjectPath;
import org.apache.hc.core5.http.io.entity.EntityUtils;
import org.hamcrest.Matcher;
import org.hamcrest.Matchers;

import java.io.IOException;
Expand Down Expand Up @@ -734,7 +733,7 @@ public void testSoftDeletesDisabledWarning() throws Exception {
settings.put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), softDeletesEnabled);
}
Request request = new Request("PUT", "/" + indexName);
request.setJsonEntity("{\"settings\": " + Strings.toString(settings.build()) + "}");
request.setJsonEntity("{\"settings\": " + Strings.toString(XContentType.JSON, settings.build()) + "}");
if (softDeletesEnabled == false) {
expectSoftDeletesWarning(request, indexName);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public void testAutomaticCancellationDuringQueryPhase() throws Exception {
Request searchRequest = new Request("GET", "/test/_search");
SearchSourceBuilder searchSource = new SearchSourceBuilder().query(scriptQuery(
new Script(ScriptType.INLINE, "mockscript", ScriptedBlockPlugin.SCRIPT_NAME, Collections.emptyMap())));
searchRequest.setJsonEntity(Strings.toString(searchSource));
searchRequest.setJsonEntity(Strings.toString(XContentType.JSON, searchSource));
verifyCancellationDuringQueryPhase(SearchAction.NAME, searchRequest);
}

Expand Down Expand Up @@ -147,7 +147,7 @@ public void testAutomaticCancellationDuringFetchPhase() throws Exception {
Request searchRequest = new Request("GET", "/test/_search");
SearchSourceBuilder searchSource = new SearchSourceBuilder().scriptField("test_field",
new Script(ScriptType.INLINE, "mockscript", ScriptedBlockPlugin.SCRIPT_NAME, Collections.emptyMap()));
searchRequest.setJsonEntity(Strings.toString(searchSource));
searchRequest.setJsonEntity(Strings.toString(XContentType.JSON, searchSource));
verifyCancellationDuringFetchPhase(SearchAction.NAME, searchRequest);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -741,13 +741,17 @@ public void testNoopUpdate() {

final BulkItemResponse noopUpdate = bulkResponse.getItems()[0];
assertThat(noopUpdate.getResponse().getResult(), equalTo(DocWriteResponse.Result.NOOP));
assertThat(Strings.toString(noopUpdate), noopUpdate.getResponse().getShardInfo().getSuccessful(), equalTo(2));
assertThat(Strings.toString(XContentType.JSON, noopUpdate), noopUpdate.getResponse().getShardInfo().getSuccessful(), equalTo(2));

final BulkItemResponse notFoundUpdate = bulkResponse.getItems()[1];
assertNotNull(notFoundUpdate.getFailure());

final BulkItemResponse notFoundDelete = bulkResponse.getItems()[2];
assertThat(notFoundDelete.getResponse().getResult(), equalTo(DocWriteResponse.Result.NOT_FOUND));
assertThat(Strings.toString(notFoundDelete), notFoundDelete.getResponse().getShardInfo().getSuccessful(), equalTo(2));
assertThat(
Strings.toString(XContentType.JSON, notFoundDelete),
notFoundDelete.getResponse().getShardInfo().getSuccessful(),
equalTo(2)
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import org.opensearch.common.collect.ImmutableOpenIntMap;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.util.set.Sets;
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.gateway.GatewayAllocator;
import org.opensearch.index.IndexNotFoundException;
import org.opensearch.index.engine.Engine;
Expand Down Expand Up @@ -136,7 +137,7 @@ public void testBulkWeirdScenario() throws Exception {
assertThat(bulkResponse.hasFailures(), equalTo(false));
assertThat(bulkResponse.getItems().length, equalTo(2));

logger.info(Strings.toString(bulkResponse, true, true));
logger.info(Strings.toString(XContentType.JSON, bulkResponse, true, true));

internalCluster().assertSeqNos();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ public void testMaybeFlush() throws Exception {
final FlushStats flushStats = shard.flushStats();
logger.info(
"--> translog stats [{}] gen [{}] commit_stats [{}] flush_stats [{}/{}]",
Strings.toString(translogStats),
Strings.toString(XContentType.JSON, translogStats),
translog.getGeneration().translogFileGeneration,
commitStats.getUserData(),
flushStats.getPeriodic(),
Expand Down
Loading