Removes requirement of the arguments in the constructor method (Filtered) in favor #615 #616
Conversation
and validates the method toArray if one of them was set
|
ping @ruflin |
| 'filter' => $this->_filter->toArray() | ||
| )); | ||
| if ($this->_query === null && $this->_filter === null) { | ||
| throw new NotImplementedException('The query or filter have not been defined, you define at least one'); |
There was a problem hiding this comment.
I would suggest to use the not found exception here.
Also please use an exception per variable so it is clear, which one is missing for the developer using the library.
There was a problem hiding this comment.
I don't understand this point: "Also please use an exception per variable so it is clear, which one is missing for the developer using the library."
There was a problem hiding this comment.
Judging from the docs, both a query and filter are always required, so if one is missing the user needs to know which. Your message suggests it's possible to supply only one.
There was a problem hiding this comment.
@mal Not! both unnecessary arguments and documentation shows that you can use both "query", "filter" or both.
There was a problem hiding this comment.
In that case, you can save a comparison with something like;
public function toArray()
{
$filtered = array();
if ($this->_query !== null) {
$filtered['query'] = $this->_query->toArray();
}
if ($this->_filter !== null) {
$filtered['filter'] = $this->_filter->toArray();
}
if (empty($filtered)) {
throw new NotFoundException('A query and/or filter is required');
}
return array('filtered' => $filtered);
}There was a problem hiding this comment.
@mal thanks for the suggestion!
|
Yep, still here. Was just busy the last days. See comments. Can you also update the changes.txt file and add a test to make sure the changes work as expected? |
|
Out of curiosity, I'd be interested to understand the use case for a FilteredQuery with no filter to apply to the query, or no query to be filtered? |
…he sets add message of exception suggested by @mal
|
Also; while trying to understand filtered queries a bit more; I tried this on ES1.1 and got valid results; so maybe a check/exception isn't needed at all ... {
"query": {
"filtered": {}
}
} |
|
ping @ruflin |
|
@hugohenrique Can you confirm what @mal just wrote above? Is only one of the two necessary? Please also update the changes.txt file. |
| $filtered['filter'] = $this->_filter->toArray(); | ||
| } | ||
|
|
||
| if (empty($filtered)) { |
There was a problem hiding this comment.
Please use the InvalidException here, as not implemented is used for features which were not implemented yet. But here the arguments are invalid.
|
ping @ruflin |
|
@hugohenrique Did you see my other two comments in the code? |
|
Technically neither query, nor filter is required, it can in effect be empty, which seems to behave much like match_all. |
|
@ruflin, I change some points, based in your suggestions, but others do not seed the need, you can reevaluate? |
|
@hugohenrique Will do on the weekend. |
|
I would prefer to check for the specific objects instead of null to make sure, it only gets executed when the proper object is set (which should be always the case). This means set a whiteliste instead of blacklist for the params. The good part about your solution is, that we would discover potential problems, if somehow someone sets the value to something where toArray doesn't work. The Invalid exception looks good. The part I don't like is that it mixes the two error messages: "A query and/or filter is required". I would like the developer that uses the API exactly what is missing and what he has to add. As mentioned before, this adds a little bit more complexity on the client side, but in the end it simplifies the developers life. |
|
Hi @ruflin, |
|
It was more of an input then a question. Instead of me changing it I think we should have a conversation on how to do it best. My first comment is related to line number 102. |
|
How can we end this? |
|
@hugohenrique There are 2 options. Either add a commit with the suggested changes or I will try to find some time this weekend to implement it :-) |
|
@hugohenrique I implemented my changes here and also added some tests: #636 |
|
Thanks @ruflin! |
|
Hm, I just realized I made an error during the implementation. At the end, also parent::toArray() should be called to make sure |
Removes requirement of the arguments in the constructor method and validates the method toArray if one of them was set.
In favor #615