-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Ability to filter alerts by string parameters #92036
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -207,23 +207,33 @@ export const hasFilterKeyError = ( | |
|
||
export const fieldDefined = (indexMappings: IndexMapping, key: string): boolean => { | ||
const mappingKey = 'properties.' + key.split('.').join('.properties.'); | ||
const potentialKey = get(indexMappings, mappingKey); | ||
if (get(indexMappings, mappingKey) != null) { | ||
return true; | ||
} | ||
|
||
// If the `mappingKey` does not match a valid path, before returning null, | ||
// If the `mappingKey` does not match a valid path, before returning false, | ||
// we want to check and see if the intended path was for a multi-field | ||
// such as `x.attributes.field.text` where `field` is mapped to both text | ||
// and keyword | ||
if (potentialKey == null) { | ||
const propertiesAttribute = 'properties'; | ||
const indexOfLastProperties = mappingKey.lastIndexOf(propertiesAttribute); | ||
const fieldMapping = mappingKey.substr(0, indexOfLastProperties); | ||
const fieldType = mappingKey.substr( | ||
mappingKey.lastIndexOf(propertiesAttribute) + `${propertiesAttribute}.`.length | ||
); | ||
const mapping = `${fieldMapping}fields.${fieldType}`; | ||
|
||
return get(indexMappings, mapping) != null; | ||
} else { | ||
const propertiesAttribute = 'properties'; | ||
const indexOfLastProperties = mappingKey.lastIndexOf(propertiesAttribute); | ||
const fieldMapping = mappingKey.substr(0, indexOfLastProperties); | ||
const fieldType = mappingKey.substr( | ||
mappingKey.lastIndexOf(propertiesAttribute) + `${propertiesAttribute}.`.length | ||
); | ||
const mapping = `${fieldMapping}fields.${fieldType}`; | ||
if (get(indexMappings, mapping) != null) { | ||
return true; | ||
} | ||
|
||
// If the path is for a flattned type field, we'll assume the mappings are defined. | ||
const keys = key.split('.'); | ||
for (let i = 0; i < keys.length; i++) { | ||
const path = `properties.${keys.slice(0, i + 1).join('.properties.')}`; | ||
if (get(indexMappings, path)?.type === 'flattened') { | ||
return true; | ||
} | ||
} | ||
Comment on lines
+230
to
+236
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @elastic/kibana-core this PR is ready for review. I had to add this piece of code and a few early returns to support |
||
|
||
return false; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,16 @@ export default function createFindTests({ getService }: FtrProviderContext) { | |
|
||
afterEach(() => objectRemover.removeAll()); | ||
|
||
async function createAlert(overwrites = {}) { | ||
const { body: createdAlert } = await supertest | ||
.post(`${getUrlPrefix(Spaces.space1.id)}/api/alerts/alert`) | ||
.set('kbn-xsrf', 'foo') | ||
.send(getTestAlertData(overwrites)) | ||
.expect(200); | ||
objectRemover.add(Spaces.space1.id, createdAlert.id, 'alert', 'alerts'); | ||
return createdAlert; | ||
} | ||
|
||
it('should handle find alert request appropriately', async () => { | ||
const { body: createdAlert } = await supertest | ||
.post(`${getUrlPrefix(Spaces.space1.id)}/api/alerts/alert`) | ||
|
@@ -85,5 +95,23 @@ export default function createFindTests({ getService }: FtrProviderContext) { | |
data: [], | ||
}); | ||
}); | ||
|
||
it('should filter on string parameters', async () => { | ||
await Promise.all([ | ||
createAlert({ params: { strValue: 'my a' } }), | ||
createAlert({ params: { strValue: 'my b' } }), | ||
createAlert({ params: { strValue: 'my c' } }), | ||
]); | ||
|
||
const response = await supertest.get( | ||
`${getUrlPrefix( | ||
Spaces.space1.id | ||
)}/api/alerts/_find?filter=alert.attributes.params.strValue:"my b"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's feeling like we're leaking Alerting internal implementation here... I'm ok with that, as it feels like the overhead of trying to abstract the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gmmorris Agreed, I always felt like that with the KQL filter. Though, the enforcement of Under Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, makes sense. Do we actually explain that the Alerts are stored as SOs? 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the docs:
I think the only extra mention we need is that they're |
||
); | ||
|
||
expect(response.status).to.eql(200); | ||
expect(response.body.total).to.equal(1); | ||
expect(response.body.data[0].params.strValue).to.eql('my b'); | ||
}); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gchaps let me know what you think, I wanted to state that filtering on the
params
field has limitations.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've merged this PR, but I will create a follow-up if you would like changes to this line.