Migrate deprecated match query syntax #11554
Conversation
There was a problem hiding this comment.
Hmm, I guess my concern here is that, as of now, you can still manually edit the query DSL for a given filter by clicking on the "edit" button. Seems to me that it would make sense to also use this migrateFilter in the mapFilter module so that if someone edits the DSL, we actually send what they specify. Thoughts @Bargs?
| @@ -0,0 +1,19 @@ | |||
| import _ from 'lodash'; | |||
|
|
|||
| export function migrateFilter(filter) { | |||
There was a problem hiding this comment.
Could you add a unit test for this?
|
I'm not sure what you mean by "if someone edits the DSL, we actually send what they specify". Can you give me an example? |
|
I mean when they click on the edit button in the filter popover:
|
|
|
||
| if (_.isPlainObject(params) && params.type === 'phrase') { | ||
| const newFilter = { | ||
| match_phrase: _.clone(filter.match, true) |
There was a problem hiding this comment.
How about turning this into a single declarative expression like
return {
match_phrase: {
[fieldName]: _.omit(params, 'type'),
},
};| const fieldName = Object.keys(filter.match)[0]; | ||
| const params = filter.match[fieldName]; | ||
|
|
||
| if (_.isPlainObject(params) && params.type === 'phrase') { |
There was a problem hiding this comment.
Could it be worth extracting this into a separate predicate?
function isMatchPhraseFilter(filter, fieldName) {
return _.get(filter, ['match', fieldName, 'type']) === 'phrase';
}Then the condition would be very descriptive as to its purpose:
if (isMatchPhraseFilter(filter, fieldName)) {
// ...
}
|
@lukasolson @weltenwort ready for another look |
| match_all: {} | ||
| }; | ||
| const migratedFilter = migrateFilter(originalFilter); | ||
| expect(migratedFilter).to.be(originalFilter); |
There was a problem hiding this comment.
Maybe you could add to this test
expect(migratedFilter).to.not.eql(newMatchPhraseFilter);Just to make sure that the filter hasn't been modified.
There was a problem hiding this comment.
Good point, would the following be better though?
expect(_.isEqual(migratedFilter, originalFilter)).to.be(true);
There was a problem hiding this comment.
Either way is really fine... I prefer to.eql because when you use the syntax you've suggested, then the error is reported as something like "expected false to be true" instead of "expected { foo: bar } to sort of equal { foo: baz }" or whatever.
There was a problem hiding this comment.
I talking more about the idea of checking the equality between migratedFilter and originalFilter instead of checking that migratedFilter is not eql to newMatchPhraseFilter. It seems that the former would catch more errors, but I wasn't sure if I was missing something about what you were suggesting.
As an aside, I agree that the error reporting sucks when using _.isEqual but the problem I have with expect.js's eql function is that it uses loose equality (==) so there are subtle bugs it would miss. Another reason I wish we were using Chai.
Since I'm considering not doing a full blown re-write of the existing filter model I wanted to get this taken care in a smaller way so we're not taken by surprise when the deprecated syntax disappears in 6.0.
Fixes #10653
Since I'm considering not doing a full blown re-write of the existing filter model I wanted to get this taken care in a smaller way so we're not taken by surprise when the deprecated syntax disappears in 6.0.