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

Function score parser should throw exception if both functions:[] and single function given #5995

Closed
wants to merge 4 commits into from

Conversation

brwe
Copy link
Contributor

@brwe brwe commented Apr 30, 2014

No description provided.

@brwe brwe added review and removed v1.1.2 labels Apr 30, 2014
@brwe
Copy link
Contributor Author

brwe commented Apr 30, 2014

We might want to consider treating the following case as a special case

...
"functions": [
...
], 
"boost_factor": some number
...

because the name "boost" and "boost_factor" are pretty similar. The error message could be the same with the addition "... Did you mean "boost" instead?"

client().search(
searchRequest().source(
searchSource().query(query))).actionGet();
assertFalse(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

fail() should work as well

@jpountz
Copy link
Contributor

jpountz commented Apr 30, 2014

LGTM

The error message could be the same with the addition "... Did you mean "boost" instead?"

This would be nice indeed. I can easily see some confusion here.

@jpountz jpountz removed the review label Apr 30, 2014
@brwe
Copy link
Contributor Author

brwe commented Apr 30, 2014

Thanks for the review! Iadded two commits implementing the suggestions.

// For better readability of error message
String singleFunctionName = null;
final String errorMessagePrefix = "You can either define \"functions\":[...] or a single function, not both. ";
final String boostNameMessageSuffix = " Did you mean \"boost\" instead?";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have these two variables as constants instead?

@brwe
Copy link
Contributor Author

brwe commented May 2, 2014

OK, added new commit to remove the duplicate code and make the strings constant

@@ -46,6 +49,9 @@

public static final String NAME = "function_score";
ScoreFunctionParserMapper funtionParserMapper;
// For better readability of error message
static final String misplacedFunctionMessagePrefix = "You can either define \"functions\":[...] or a single function, not both. ";
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually use upper case and underscore case for contants, see eg. NAME above.

Copy link
Contributor

Choose a reason for hiding this comment

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

that would give MISPLACED_FUNCTION_MESSAGE_PREFIX

@jpountz
Copy link
Contributor

jpountz commented May 2, 2014

Apart from a minor formatting issue, this looks good to me!

brwe added a commit that referenced this pull request May 2, 2014
…e function given

In addition, add a special warning if the misplaced function is a "boost_factor"
function to avoid confusion of "boost" and "boost_function".

closes #5995
@brwe brwe closed this in 2e44040 May 2, 2014
@clintongormley clintongormley changed the title function_score parser should throw exception if both functions:[] and si... Function score parser should throw exception if both functions:[] and single function given Jun 7, 2015
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query DSL labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search/Search Search-related issues that do not fall into other categories v1.2.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants