Internal: make sure ParseField is always used in combination with parse flags #11859
Conversation
javanna
force-pushed
the
fix/parse_field_usages
branch
from
f4cb084
to
c45307b
Compare
| @@ -59,7 +53,7 @@ public ScriptParameterParser(Set<String> parameterNames) { | |||
| fileParameters = new HashSet<>(); | |||
| indexedParameters = new HashSet<>(); | |||
| for (String parameterName : parameterNames) { | |||
| if (ScriptService.SCRIPT_LANG.match(parameterName)) { | |||
| if (ScriptService.SCRIPT_LANG.match(parameterName, ParseField.EMPTY_FLAGS)) { | |||
There was a problem hiding this comment.
Here we should be able to get correct flags? Maybe not always, but at least in the context of aggs, I think the search context is available
There was a problem hiding this comment.
yes we could but it is not needed given what we are doing we did, in this specific case we are only interested in the return value of the call, cause if it's true we throw exception ourselves. Makes sense?
|
LGTM Just thinking out loud, but for future usage of this API, I'm wondering how we could make it easier to use in order to discourage lazy users to pass empty flags. Could be something like SearchContext.match(ParseField) that would automatically pass in the right flags. |
|
I agree @jpountz we could make it even better , I will look into your suggestion |
|
@jpountz I pushed another commit that should make it harder to forget parse flags, naming might need some adjustments, but I think this looks much better, thanks for your suggestion! |
|
One concern I have is that it introduces yet another API: in addition to SearchContext and ParseField, we now have ParseFieldMatcher. Should we just have this class hidden and just a SearchContext.match(ParseField) method? |
|
I see your point @jpountz , that said we have different places where we need the parseFlags and we would need the match method, it is not just about the |
|
ping @s1monw this is quite a big PR I would love to get to some agreement and merge it in soon otherwise it gets outdated pretty quickly. Opinions? |
|
@javanna I think we should push as is. The Matcher looks ok to me for now? |
|
cool will push as is then, thanks! |
…se flags Removed ParseField#match variant that accepts the field name only, without parse flags. Such a method is harmful as it defaults to empty parse flags, meaning that no deprecation exceptions will be thrown in strict mode, which defeats the purpose of using ParseField. Unfortunately such a method was used in a lot of places were the parse flags weren't easily accessible (outside of query parsing), and in a lot of other places just by mistake. Parse flags have been introduced now as part of SearchContext and mappers where needed. There are a few places (e.g. java api requests) where it is not possible to retrieve them as they depend on the index settings, in that case we explicitly pass in EMPTY_FLAGS for now, but this has to be seen as an exception. Closes elastic#11859
javanna
force-pushed
the
fix/parse_field_usages
branch
from
344d064
to
c7887f4
Compare
Removed
ParseField#matchvariant that accepts the field name only, without parse flags. Such a method is harmful as it defaults to empty parse flags, meaning that no deprecation exceptions will be thrown in strict mode, which defeats the purpose of using ParseField. Unfortunately such a method was used in a lot of places were the parse flags weren't accessible (outside of query parsing), and in a lot of other places just by mistake.Parse flags have been introduced now as part of SearchContext and mappers where needed. There are a few places (e.g. java api requests) where it is not possible to retrieve them as they depend on settings, in that case we explicitly pass in EMPTY_FLAGS, but it has to be seen as an exception.