Skip to content

Start to uncouple deprecation handling from log4j #27955

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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
Closed

Start to uncouple deprecation handling from log4j #27955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
38fc94d
WIP
nik9000 Dec 21, 2017
01527c2
WAP
nik9000 Dec 21, 2017
585af19
Too much
nik9000 Dec 21, 2017
c6824e8
More constructing
nik9000 Dec 21, 2017
f792c60
Start to uncouple deprecation handling from logging
nik9000 Dec 21, 2017
a730054
Fix test
nik9000 Dec 21, 2017
bb5dc2a
Fix other test
nik9000 Dec 21, 2017
910d276
Undo silly change
nik9000 Dec 21, 2017
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: 4 additions & 1 deletion client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import org.elasticsearch.action.support.WriteRequest;
import org.elasticsearch.action.update.UpdateRequest;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.SuppressForbidden;
import org.elasticsearch.common.bytes.BytesReference;
Expand Down Expand Up @@ -270,7 +271,9 @@ static Request bulk(BulkRequest bulkRequest) throws IOException {
BytesReference indexSource = indexRequest.source();
XContentType indexXContentType = indexRequest.getContentType();

try (XContentParser parser = XContentHelper.createParser(NamedXContentRegistry.EMPTY, indexSource, indexXContentType)) {
// UNSUPPORTED_OPERATION_DEPRECATION_HANDLER is fine here because copyCurrentStructure does not interact with deprecation
try (XContentParser parser = XContentHelper.createParser(NamedXContentRegistry.EMPTY,
ParseField.UNSUPPORTED_OPERATION_DEPRECATION_HANDLER, indexSource, indexXContentType)) {
try (XContentBuilder builder = XContentBuilder.builder(bulkContentType.xContent())) {
builder.copyCurrentStructure(parser);
source = builder.bytes().toBytesRef();
Expand Down
8 changes: 7 additions & 1 deletion client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,13 @@ protected final <Resp> Resp parseEntity(final HttpEntity entity,
if (xContentType == null) {
throw new IllegalStateException("Unsupported Content-Type: " + entity.getContentType().getValue());
}
try (XContentParser parser = xContentType.xContent().createParser(registry, entity.getContent())) {
/*
* We use IGNORING_DEPRECATION_HANDLER because users of the high
* level rest client can't do anything about deprecated fields in
* the responses.
Copy link
Contributor

@imotov imotov Dec 26, 2017

Choose a reason for hiding this comment

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

This comment gave me pause. I think it's somewhat misleading. Users can upgrade the version of the server, so they can do something about it. However, they might choose not to do that and this is perfectly fine. Maybe we can change this comment into something like "We use IGNORING_DEPRECATION_HANDLER because we are expected to work with deprecated fields while parsing responses from older version of elasticsearch and there is no need to notify users about every single occurrence of such field."

Copy link
Member Author

@nik9000 nik9000 Dec 26, 2017

Choose a reason for hiding this comment

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

Makes sense to me.

Copy link
Member Author

@nik9000 nik9000 Dec 26, 2017

Choose a reason for hiding this comment

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

I like that this makes us think about deprecated fields here.

*/
try (XContentParser parser = xContentType.xContent()
.createParser(registry, ParseField.IGNORING_DEPRECATION_HANDLER, entity.getContent())) {
return entityParser.apply(parser);
}
}
Expand Down
5 changes: 4 additions & 1 deletion ...in/java/org/elasticsearch/action/admin/cluster/node/tasks/get/TransportGetTaskAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.concurrent.AbstractRunnable;
Expand Down Expand Up @@ -248,7 +249,9 @@ void onGetFinishedTaskFromIndex(GetResponse response, ActionListener<GetTaskResp
listener.onFailure(new ElasticsearchException("Stored task status for [{}] didn't contain any source!", response.getId()));
return;
}
try (XContentParser parser = XContentHelper.createParser(xContentRegistry, response.getSourceAsBytesRef())) {
// UNSUPPORTED_OPERATION_DEPRECATION_HANDLER is fine here because there are no deprecated field declared in TaskResult.PARSER
try (XContentParser parser = XContentHelper.createParser(xContentRegistry,
ParseField.UNSUPPORTED_OPERATION_DEPRECATION_HANDLER, response.getSourceAsBytesRef())) {
TaskResult result = TaskResult.PARSER.apply(parser, null);
listener.onResponse(new GetTaskResponse(result));
}
Expand Down
8 changes: 4 additions & 4 deletions core/src/main/java/org/elasticsearch/action/admin/indices/alias/Alias.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,16 +205,16 @@ public static Alias fromXContent(XContentParser parser) throws IOException {
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
} else if (token == XContentParser.Token.START_OBJECT) {
if (FILTER.match(currentFieldName)) {
if (FILTER.match(currentFieldName, parser.deprecationHandler())) {
Map<String, Object> filter = parser.mapOrdered();
alias.filter(filter);
}
} else if (token == XContentParser.Token.VALUE_STRING) {
if (ROUTING.match(currentFieldName)) {
if (ROUTING.match(currentFieldName, parser.deprecationHandler())) {
alias.routing(parser.text());
} else if (INDEX_ROUTING.match(currentFieldName)) {
} else if (INDEX_ROUTING.match(currentFieldName, parser.deprecationHandler())) {
alias.indexRouting(parser.text());
} else if (SEARCH_ROUTING.match(currentFieldName)) {
} else if (SEARCH_ROUTING.match(currentFieldName, parser.deprecationHandler())) {
alias.searchRouting(parser.text());
}
}
Expand Down
16 changes: 12 additions & 4 deletions core/src/main/java/org/elasticsearch/action/admin/indices/create/CreateIndexRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@
import org.elasticsearch.action.support.IndicesOptions;
import org.elasticsearch.action.support.master.AcknowledgedRequest;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.common.LoggingDeprecationHandler;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.ParseField.DeprecationHandler;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.collect.MapBuilder;
Expand Down Expand Up @@ -318,7 +320,10 @@ public CreateIndexRequest aliases(String source) {
*/
public CreateIndexRequest aliases(BytesReference source) {
// EMPTY is safe here because we never call namedObject
try (XContentParser parser = XContentHelper.createParser(NamedXContentRegistry.EMPTY, source)) {
// TODO LoggingDeprecationHandler probably should be visible to a request
Copy link
Contributor

@imotov imotov Dec 26, 2017

Choose a reason for hiding this comment

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

That wouldn't this kind of kill the whole idea of separating log4j from high level rest clients?

Copy link
Member Author

@nik9000 nik9000 Dec 26, 2017

Choose a reason for hiding this comment

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

I left it like this because I didn't want to change too many things in this PR. I'd love to change how this works but that is technically a breaking change for the transport client and the high level rest client and I didn't want to bury it with all the internal changes in this PR. Basically I think we should change this but I didn't want to do it now.

Copy link
Contributor

@imotov imotov Dec 27, 2017

Choose a reason for hiding this comment

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

I would be fine with dealing with this in a separate PR.

// because the requests might be in a separate jar from core
try (XContentParser parser = XContentHelper.createParser(NamedXContentRegistry.EMPTY,
LoggingDeprecationHandler.INSTANCE, source)) {
//move to the first alias
parser.nextToken();
while ((parser.nextToken()) != XContentParser.Token.END_OBJECT) {
Expand Down Expand Up @@ -380,16 +385,19 @@ public CreateIndexRequest source(BytesReference source, XContentType xContentTyp
*/
@SuppressWarnings("unchecked")
public CreateIndexRequest source(Map<String, ?> source) {
// TODO LoggingDeprecationHandler probably should be visible to a request
// because the requests might be in a separate jar from core
DeprecationHandler deprecationHandler = ParseField.UNSUPPORTED_OPERATION_DEPRECATION_HANDLER;
for (Map.Entry<String, ?> entry : source.entrySet()) {
String name = entry.getKey();
if (SETTINGS.match(name)) {
if (SETTINGS.match(name, deprecationHandler)) {
settings((Map<String, Object>) entry.getValue());
} else if (MAPPINGS.match(name)) {
} else if (MAPPINGS.match(name, deprecationHandler)) {
Map<String, Object> mappings = (Map<String, Object>) entry.getValue();
for (Map.Entry<String, Object> entry1 : mappings.entrySet()) {
mapping(entry1.getKey(), (Map<String, Object>) entry1.getValue());
}
} else if (ALIASES.match(name)) {
} else if (ALIASES.match(name, deprecationHandler)) {
aliases((Map<String, Object>) entry.getValue());
} else {
// maybe custom?
Expand Down
6 changes: 5 additions & 1 deletion ...ain/java/org/elasticsearch/action/admin/indices/template/put/PutIndexTemplateRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.elasticsearch.action.support.IndicesOptions;
import org.elasticsearch.action.support.master.MasterNodeRequest;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.common.LoggingDeprecationHandler;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.collect.MapBuilder;
Expand Down Expand Up @@ -436,7 +437,10 @@ public PutIndexTemplateRequest aliases(String source) {
*/
public PutIndexTemplateRequest aliases(BytesReference source) {
// EMPTY is safe here because we never call namedObject
try (XContentParser parser = XContentHelper.createParser(NamedXContentRegistry.EMPTY, source)) {
// TODO LoggingDeprecationHandler shouldn't be visible in requests
// because LoggingDeprecationHandler is part of core and the requests would like to move to a separate jar
try (XContentParser parser = XContentHelper.createParser(
NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, source)) {
//move to the first alias
parser.nextToken();
while ((parser.nextToken()) != XContentParser.Token.END_OBJECT) {
Expand Down
38 changes: 22 additions & 16 deletions core/src/main/java/org/elasticsearch/action/bulk/BulkRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.elasticsearch.action.support.WriteRequest;
import org.elasticsearch.action.support.replication.ReplicationRequest;
import org.elasticsearch.action.update.UpdateRequest;
import org.elasticsearch.common.LoggingDeprecationHandler;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.Strings;
Expand Down Expand Up @@ -304,7 +305,10 @@ public BulkRequest add(BytesReference data, @Nullable String defaultIndex, @Null

// now parse the action
// EMPTY is safe here because we never call namedObject
try (XContentParser parser = xContent.createParser(NamedXContentRegistry.EMPTY, data.slice(from, nextMarker - from))) {
// TODO LoggingDeprecationHandler is probably not appropriate here because this is a request
// because LoggingDeprecationHandler is a server side thing but this is a request
Copy link
Contributor

@imotov imotov Dec 26, 2017

Choose a reason for hiding this comment

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

It's actually used in both server and client content. So, maybe we need to have another version of this method that accepts logger as a parameter and pass LoggingDeprecationHandler on the server side and IGNORING_DEPRECATION_HANDLER on the client.

try (XContentParser parser = xContent.createParser(NamedXContentRegistry.EMPTY,
LoggingDeprecationHandler.INSTANCE, data.slice(from, nextMarker - from))) {
// move pointers
from = nextMarker + 1;

Expand Down Expand Up @@ -348,45 +352,45 @@ public BulkRequest add(BytesReference data, @Nullable String defaultIndex, @Null
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
} else if (token.isValue()) {
if (INDEX.match(currentFieldName)){
if (INDEX.match(currentFieldName, parser.deprecationHandler())){
if (!allowExplicitIndex) {
throw new IllegalArgumentException("explicit index in bulk is not allowed");
}
index = parser.text();
} else if (TYPE.match(currentFieldName)) {
} else if (TYPE.match(currentFieldName, parser.deprecationHandler())) {
type = parser.text();
} else if (ID.match(currentFieldName)) {
} else if (ID.match(currentFieldName, parser.deprecationHandler())) {
id = parser.text();
} else if (ROUTING.match(currentFieldName)) {
} else if (ROUTING.match(currentFieldName, parser.deprecationHandler())) {
routing = parser.text();
} else if (PARENT.match(currentFieldName)) {
} else if (PARENT.match(currentFieldName, parser.deprecationHandler())) {
parent = parser.text();
} else if (OP_TYPE.match(currentFieldName)) {
} else if (OP_TYPE.match(currentFieldName, parser.deprecationHandler())) {
opType = parser.text();
} else if (VERSION.match(currentFieldName)) {
} else if (VERSION.match(currentFieldName, parser.deprecationHandler())) {
version = parser.longValue();
} else if (VERSION_TYPE.match(currentFieldName)) {
} else if (VERSION_TYPE.match(currentFieldName, parser.deprecationHandler())) {
versionType = VersionType.fromString(parser.text());
} else if (RETRY_ON_CONFLICT.match(currentFieldName)) {
} else if (RETRY_ON_CONFLICT.match(currentFieldName, parser.deprecationHandler())) {
retryOnConflict = parser.intValue();
} else if (PIPELINE.match(currentFieldName)) {
} else if (PIPELINE.match(currentFieldName, parser.deprecationHandler())) {
pipeline = parser.text();
} else if (FIELDS.match(currentFieldName)) {
} else if (FIELDS.match(currentFieldName, parser.deprecationHandler())) {
throw new IllegalArgumentException("Action/metadata line [" + line + "] contains a simple value for parameter [fields] while a list is expected");
} else if (SOURCE.match(currentFieldName)) {
} else if (SOURCE.match(currentFieldName, parser.deprecationHandler())) {
fetchSourceContext = FetchSourceContext.fromXContent(parser);
} else {
throw new IllegalArgumentException("Action/metadata line [" + line + "] contains an unknown parameter [" + currentFieldName + "]");
}
} else if (token == XContentParser.Token.START_ARRAY) {
if (FIELDS.match(currentFieldName)) {
if (FIELDS.match(currentFieldName, parser.deprecationHandler())) {
DEPRECATION_LOGGER.deprecated("Deprecated field [fields] used, expected [_source] instead");
List<Object> values = parser.list();
fields = values.toArray(new String[values.size()]);
} else {
throw new IllegalArgumentException("Malformed action/metadata line [" + line + "], expected a simple value for field [" + currentFieldName + "] but found [" + token + "]");
}
} else if (token == XContentParser.Token.START_OBJECT && SOURCE.match(currentFieldName)) {
} else if (token == XContentParser.Token.START_OBJECT && SOURCE.match(currentFieldName, parser.deprecationHandler())) {
fetchSourceContext = FetchSourceContext.fromXContent(parser);
} else if (token != XContentParser.Token.VALUE_NULL) {
throw new IllegalArgumentException("Malformed action/metadata line [" + line + "], expected a simple value for field [" + currentFieldName + "] but found [" + token + "]");
Expand Down Expand Up @@ -428,8 +432,10 @@ public BulkRequest add(BytesReference data, @Nullable String defaultIndex, @Null
.routing(routing)
.parent(parent);
// EMPTY is safe here because we never call namedObject
// TODO LoggingDeprecationHandler is probably not appropriate here because this is a request
// because LoggingDeprecationHandler is a server side thing but this is a request
try (XContentParser sliceParser = xContent.createParser(NamedXContentRegistry.EMPTY,
sliceTrimmingCarriageReturn(data, from, nextMarker, xContentType))) {
LoggingDeprecationHandler.INSTANCE, sliceTrimmingCarriageReturn(data, from, nextMarker, xContentType))) {
updateRequest.fromXContent(sliceParser);
}
if (fetchSourceContext != null) {
Expand Down
Loading