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

Report parser name and location in XContent deprecation warnings #53752

Merged
merged 5 commits into from
Mar 19, 2020

Conversation

romseygeek
Copy link
Contributor

It's simple to deprecate a field used in an ObjectParser just by adding deprecation
markers to the relevant ParseField objects. The warnings themselves don't currently
have any context - they simply say that a deprecated field has been used, but not
where in the input xcontent it appears. This commit adds the parent object parser
name and XContentLocation to these deprecation messages.

@romseygeek romseygeek requested a review from nik9000 March 18, 2020 17:15
@romseygeek romseygeek self-assigned this Mar 18, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Core)

@romseygeek
Copy link
Contributor Author

ParseField.match() is used in nearly 1,000 places in the code outside of an ObjectParser, so this PR leaves the existing method signature and adds another one, with a BWC layer so that if we don't have the parent parser name then we emit the old version of the error message. We can use the calls to the old signature to find places to convert to using ObjectParser.

I also changed the 'this field has been removed entirely' message to read more easily.

Comment on lines 55 to 60
if (parserName != null) {
deprecationLogger.deprecated("[{}][{}] Deprecated field [{}] used, expected [{}] instead",
parserName, location.get(), usedName, modernName);
} else {
deprecationLogger.deprecated("Deprecated field [{}] used, expected [{}] instead", usedName, modernName);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The only suggestion I'd make is about whether it's possible to consolidate the log messages. For example:

Suggested change
if (parserName != null) {
deprecationLogger.deprecated("[{}][{}] Deprecated field [{}] used, expected [{}] instead",
parserName, location.get(), usedName, modernName);
} else {
deprecationLogger.deprecated("Deprecated field [{}] used, expected [{}] instead", usedName, modernName);
}
String prefix = parserName == null ? "" :
"[" + parserName + "][" + location.get() + "] ";
deprecationLogger.deprecated("{}Deprecated field [{}] used, expected [{}] instead", prefix, usedName, modernName);

deprecationLogger.deprecated("[{}][{}] Deprecated field [{}] used, this field is unused and will be removed entirely",
parserName, location.get(), usedName);
String prefix = parserName == null ? "" : "[" + parserName + "][" + location.get() + "] ";
if (true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look 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.

It wasn't!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, out-of-date diff 😅

@romseygeek romseygeek merged commit 7636930 into elastic:master Mar 19, 2020
@romseygeek romseygeek deleted the objectparser/named-deprecations branch March 19, 2020 11:26
@romseygeek
Copy link
Contributor Author

Thanks for the review @pugnascotia !

romseygeek added a commit that referenced this pull request Mar 19, 2020
romseygeek added a commit that referenced this pull request Mar 19, 2020
…ngs (#53752)"

This reverts commit 7636930.

There is some randomization in the YAML test suite which means we can't check
for exact xcontentlocation in the deprecation warning headers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants