Add types deprecation to script contexts #37554
Conversation
jdconrad
added
:Core/Infra/Scripting
|
Pinging @elastic/es-core-infra |
|
Looks good to me from the types deprecation side. Thanks @jdconrad for the very speedy work on this! |
| @@ -72,7 +85,7 @@ | |||
| @Override | |||
| public IngestDocument execute(IngestDocument document) { | |||
| IngestScript.Factory factory = scriptService.compile(script, IngestScript.CONTEXT); | |||
| factory.newInstance(script.getParams()).execute(document.getSourceAndMetadata()); | |||
| factory.newInstance(script.getParams()).execute(new DeprecationMap(document.getSourceAndMetadata(), DEPRECATIONS)); | |||
There was a problem hiding this comment.
I noticed that DeprecationMap uses deprecated instead of deprecatedAndMaybeLog. Is this something we might want to change?
There was a problem hiding this comment.
That's a good point. @rjernst What are your thoughts here?
There was a problem hiding this comment.
I agree deprecatedAndMaybeLog should be used.
|
@elasticmachine run gradle build tests 1 |
1 similar comment
|
@elasticmachine run gradle build tests 1 |
|
@rjernst Modified DeprecationMap to use deprecateAndMaybeLog. Ready for another review. |
|
@elasticmachine run gradle build tests 1 |
|
@elasticmachine run gradle build tests 2 |
1 similar comment
|
@elasticmachine run gradle build tests 2 |
|
@elasticmachine run gradle build tests 1 |
|
@elasticmachine run gradle build tests 2 |
| Map<String, String> deprecations = new HashMap<>(); | ||
| deprecations.put( | ||
| "_type", | ||
| "[types removal] Looking up doc types [_type] in scripts is deprecated." |
There was a problem hiding this comment.
Why the prefix [types removal]? I dont' think we have any prefixes like that in other deprecation messages.
There was a problem hiding this comment.
We had to choose a shared prefix in order for there to be a consistent way to detect types deprecation messages in REST tests (and ignore them). I think @jdconrad is just using this prefix here for consistency.
There was a problem hiding this comment.
This was copied from the other types removal messages. I'd like to keep it for consistency unless there are other objections.
| @@ -64,7 +67,7 @@ public boolean containsValue(final Object value) { | |||
| public Object get(final Object key) { | |||
| String deprecationMessage = deprecations.get(key); | |||
| if (deprecationMessage != null) { | |||
| deprecationLogger.deprecated(deprecationMessage); | |||
| deprecationLogger.deprecatedAndMaybeLog(logKeyPrefix + "_" + key, deprecationMessage); | |||
There was a problem hiding this comment.
nit: i would use - as the separator, so there is a clear distinction with the map key, since all our context names use underscores.
|
@rjernst @jtibshirani Thanks for the reviews. Will commit as soon as CI passes. |
|
@elasticmachine run gradle build tests 1 |
1 similar comment
|
@elasticmachine run gradle build tests 1 |
This adds deprecation to _type in the script contexts for ingest and update. This adds a DeprecationMap that wraps the ctx Map containing _type for these specific contexts.