-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
POC: Simplifying un-abstracting projections and setup #70157
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 |
---|---|---|
|
@@ -10,7 +10,6 @@ import { | |
SERVICE_NAME, | ||
} from '../../../common/elasticsearch_fieldnames'; | ||
import { getServicesProjection } from '../../../common/projections/services'; | ||
import { mergeProjection } from '../../../common/projections/util/merge_projection'; | ||
import { PromiseReturnType } from '../../../typings/common'; | ||
import { Setup, SetupTimeRange } from '../helpers/setup_request'; | ||
import { transformServiceMapResponses } from './transform_service_map_responses'; | ||
|
@@ -76,21 +75,17 @@ async function getServicesData(options: IEnvOptions) { | |
setup: { ...setup, uiFiltersES: [] }, | ||
}); | ||
|
||
const { filter } = projection.body.query.bool; | ||
const serviceNameFilter = options.serviceName | ||
? [{ term: { [SERVICE_NAME]: options.serviceName } }] | ||
: []; | ||
|
||
const params = mergeProjection(projection, { | ||
const params = { | ||
body: { | ||
index: projection.index, | ||
size: 0, | ||
query: { | ||
bool: { | ||
...projection.body.query.bool, | ||
filter: options.serviceName | ||
? filter.concat({ | ||
term: { | ||
[SERVICE_NAME]: options.serviceName, | ||
}, | ||
}) | ||
: filter, | ||
filter: [...projection.body.query.bool.filter, ...serviceNameFilter], | ||
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. This is a good example of the confusion I get from the projection:
What takes precendence? Is In my rewrite I propose we do away with 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.
|
||
}, | ||
}, | ||
aggs: { | ||
|
@@ -109,7 +104,7 @@ async function getServicesData(options: IEnvOptions) { | |
}, | ||
}, | ||
}, | ||
}); | ||
}; | ||
|
||
const { client } = setup; | ||
|
||
|
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.
Moving away from
mergeProjection
will make the code a lot more clear - especially for reviewers when things are changed/moved aroundThere 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.
The reason why we have
mergeProjection
is two-fold:lodash.merge
merges arrays in really weird waysThe downside of spreading only means that you can miss places where the projection is being used. E.g., you'd have to manually inspect all references. Maybe that's a good thing though. Again not a defense - just want to highlight why it's there.
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.
This makes sense. Could we achieve the same with unit tests to ensure that projections are correctly applied?
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.
that depends on the nature of the change. for additive changes, no. for other changes, yes (arguably already covered by snapshot testing of queries)