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

Use parseField to parse all parameter names #8964

Closed
clintongormley opened this issue Dec 15, 2014 · 11 comments
Closed

Use parseField to parse all parameter names #8964

clintongormley opened this issue Dec 15, 2014 · 11 comments
Assignees
Labels
:Core/Infra/REST API REST infrastructure and utilities >enhancement help wanted adoptme

Comments

@clintongormley
Copy link

Much of the code for parsing parameter names looks like this:

                } else if (  "slop".equals(currentFieldName) 
                             || "phrase_slop".equals(currentFieldName) 
                             || "phraseSlop".equals(currentFieldName)) {

The parseField method already handles the parsing of alternatives (including camelCase and deprecated syntax). It also provides a central point for warning when deprecated parameter names are used (see #11033).

All existing code should be migrated to use parseField.

@colings86
Copy link
Contributor

Uses references to XContentParser as a starting point I have created a list of the classes that need to be updated as part of this ticket. There may be some false positives in there (classes which don't need to be updated) but it's hopefully caught everything.

https://gist.github.com/colings86/923e3e0a0e87bc4fe10c

@nik9000
Copy link
Member

nik9000 commented Dec 22, 2014

This seems like it'd be covered by or a starting point for the issue for
using a parsers.
On Dec 22, 2014 5:53 AM, "Colin Goodheart-Smithe" notifications@github.com
wrote:

Uses references to XContentParser as a starting point I have created a
list of the classes that need to be updated as part of this ticket. There
may be some false positives in there (classes which don't need to be
updated) but it's hopefully caught everything.

https://gist.github.com/colings86/923e3e0a0e87bc4fe10c


Reply to this email directly or view it on GitHub
#8964 (comment)
.

@markharwood
Copy link
Contributor

As part of a mass tidy-up of the parse logic it would make sense to also add the expanded error reporting introduced in issue #3303 and related PR #7891

@MaineC
Copy link

MaineC commented Nov 16, 2015

@colings86 Updated your list of classes to touch here: https://gist.github.com/MaineC/2c630dea38077a2852cc

(clumsy bash command that generated the list on top of the gist, manually deleted references of aggregations and queries after running the command assuming #14249 and your aggregations refactorings fix those)

@clintongormley clintongormley removed the help wanted adoptme label Nov 21, 2015
MaineC pushed a commit to MaineC/elasticsearch that referenced this issue Nov 23, 2015
This switches query parsing from manual field parsing to using ParseField.

Also adds unit tests for each query that check original json can be parsed
into query builders.

Relates to elastic#8964
@spinscale spinscale added v2.3.0 and removed v2.2.0 labels Dec 23, 2015
@dakrone
Copy link
Member

dakrone commented Sep 27, 2016

I think this has been resolved with the query refactor? At least when I run @MaineC's script I no longer see any classes that need to be touched.

@javanna
Copy link
Member

javanna commented Sep 27, 2016

Not all queries were migrated to parse field while working on the query refactoring, only some were. Also, using parse field is necessary everywhere in our parsing code, not only in our queries. We should update the list and re-evaluate what needs to be done, but this can't be closed just yet ;)

@dakrone
Copy link
Member

dakrone commented Sep 27, 2016

We should update the list and re-evaluate what needs to be done, but this can't be closed just yet ;)

I didn't see any more when using Isabel's script, maybe we need to update the script if there are other its missing?

@MaineC
Copy link

MaineC commented Sep 28, 2016

Looked at the bash thingy again - first of all it only checked stuff in core/*, second of all looking closer it looks like there's some copy'n'paste mix-up in there. Going through the code there still seem to be a few classes where we could be using ParseField, see below.

https://gist.github.com/MaineC/ce2bf46d228230a1f676037a40614112

@dakrone
Copy link
Member

dakrone commented Sep 28, 2016

Thanks for coming up with the new list @MaineC !

@dakrone dakrone added the help wanted adoptme label Sep 28, 2016
@javanna
Copy link
Member

javanna commented Sep 28, 2016

That seems to mean that all queries have been migrated though, which I wasn't expecting. that's great!

@MaineC
Copy link

MaineC commented Oct 4, 2016

@javanna They should have all been covered as part of #14249 - unless we've overlooked some back then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/REST API REST infrastructure and utilities >enhancement help wanted adoptme
Projects
None yet
Development

No branches or pull requests

8 participants