Refactors ScriptQueryBuilder and Parser#12115
Refactors ScriptQueryBuilder and Parser#12115alexksikes wants to merge 0 commit intoelastic:feature/query-refactoringfrom
Conversation
There was a problem hiding this comment.
given that the parser will never pass in null here, I think we could add a null check here as we did in other queries, so that if somebod using the java api sets null will get back error immediately. then we could remove the check from validate
|
left a small comment, looks good besides that |
8f976c2 to
722ebac
Compare
|
@javanna I updated the PR and rebased it. Thanks for the review. |
There was a problem hiding this comment.
can you please look at how we do this in other queries and do the same?
There was a problem hiding this comment.
I am not sure what to know you mean by do the same as in other queries. Can you point me to a particular query? Maybe I'm missing something, but it seems we had settled for not checking for null in the constructor, and only do it in validate. Checking for null in the constructor would make the tests fail because of our prototype pattern, unless we use empty constructors.
There was a problem hiding this comment.
look at BoostingQueryBuilder for instance. I explained in other PR when it makes sense to have checks in setter/constructors rather than validate, this is something that we are adapting as we go. I will say it again: check in setters/constructors are for things that can happen only through java api. depending on the parser, we might check for existence of elements in the parsers, meaning that null values will never come through from the parser to the builder. In that case null checks are only for the java api code and can be moved earlier on. If the check is valid for both rest layer (parser) and java api, then we should have the check in the validate method.
|
@javanna Thanks for the review. I updated the PR, please see my response to your comment. |
a1b9d44 to
7099e6c
Compare
Relates to elastic#10217 Closes elastic#12115 This PR is against the query-refactoring branch.
Relates to #10217
This PR is against the query-refactoring branch.