Change deleteByNamespace to include legacy URL aliases#115459
Merged
jportner merged 3 commits intoelastic:masterfrom Oct 19, 2021
Merged
Change deleteByNamespace to include legacy URL aliases#115459jportner merged 3 commits intoelastic:masterfrom
jportner merged 3 commits intoelastic:masterfrom
Conversation
jportner
added
v8.0.0
release_note:skip
jportner
commented
Oct 18, 2021
Contributor
Author
jportner
left a comment
jportner
left a comment
There was a problem hiding this comment.
Author's notes for reviewers
| properties: { | ||
| sourceId: { type: 'keyword' }, | ||
| targetType: { type: 'keyword' }, | ||
| targetNamespace: { type: 'keyword' }, |
Contributor
Author
There was a problem hiding this comment.
I had to add this field to the mappings because we are now querying on it in the SOR
Comment on lines
-783
to
+788
| const typesToUpdate = allTypes.filter((type) => !this._registry.isNamespaceAgnostic(type)); | ||
| const typesToUpdate = [ | ||
| ...allTypes.filter((type) => !this._registry.isNamespaceAgnostic(type)), | ||
| LEGACY_URL_ALIAS_TYPE, | ||
| ]; |
Contributor
Author
There was a problem hiding this comment.
Legacy URL aliases are space-agnostic by design (this made it easier for us to query/etc), but each one is only designed to be used in a single space.
TL;DR I had to add this in typesToUpdate.
Comment on lines
790
to
795
| // Construct kueryNode to filter legacy URL aliases (these space-agnostic objects do not use root-level "namespace/s" fields) | ||
| const { buildNode } = esKuery.nodeTypes.function; | ||
| const targetNamespaceField = `${LEGACY_URL_ALIAS_TYPE}.targetNamespace`; | ||
| const match1 = buildNode('is', targetNamespaceField, namespace); | ||
| const match2 = buildNode('not', buildNode('exists', targetNamespaceField)); | ||
| const kueryNode = buildNode('or', [match1, match2]); |
Contributor
Author
There was a problem hiding this comment.
This kueryNode matches a result with one of two conditions:
- The object's
legacy-url-alias.targetNamespacefield equals the current space, or The object doesn't have aThe object is not thelegacy-url-alias.targetNamespacefield at all (e.g., it is not a legacy URL alias)legacy-url-aliastype
This seemed to be the least messy way to ensure that all alias objects and all non-alias objects in this space are deleted in a single ES call.
If it is a legacy URL alias, it follows this code path in the painless script below:
if (!ctx._source.containsKey('namespaces')) {
ctx.op = "delete";
}
Edit: changed the second match condition in 9be4f61 based on the suggestion in #115459 (comment)
Comment on lines
-806
to
+818
| namespaces: namespace ? [namespace] : undefined, | ||
| namespaces: [namespace], |
Contributor
Author
There was a problem hiding this comment.
namespace is always defined (see L780-L782 above), so I removed this unnecessary ternary operator
| @@ -65,47 +66,37 @@ export function deleteTestSuiteFactory( | |||
| doc_count_error_upper_bound: 0, | |||
| sum_other_doc_count: 0, | |||
| buckets: [ | |||
Contributor
Author
There was a problem hiding this comment.
The spaces API integration test fixtures already include three legacy URL aliases:
- One in
space_1 - One in
space_2 - One in
other_space(that doesn't actually exist)
Each test deletes space_2, so the agg buckets should reflect that aliases (1) and (3) still exist afterwards 👍
Member
There was a problem hiding this comment.
It might be good to memorialize these remarks as code comments so the next soul to wander in here has this context
| { key: 'visualization', doc_count: 3 }, | ||
| { key: 'dashboard', doc_count: 2 }, | ||
| { key: 'index-pattern', doc_count: 1 }, | ||
| { key: 'legacy-url-alias', doc_count: 1 }, |
Contributor
Author
There was a problem hiding this comment.
RE: comment above, alias (1) still exists
| countByType: { | ||
| doc_count_error_upper_bound: 0, | ||
| sum_other_doc_count: 0, | ||
| buckets: [{ key: 'legacy-url-alias', doc_count: 1 }], // this alias is in a non-existent space (for other test suites) |
Contributor
Author
There was a problem hiding this comment.
RE: comment above, alias (3) still exists
Contributor
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/ml/jobs/categorization_field_examples·ts.apis Machine Learning jobs Categorization example endpoint - invalid, too many tokens.Standard OutStack TraceMetrics [docs]Saved Objects .kibana field count
To update your PR or re-run it, just comment with: |
Contributor
Author
|
Flaky ML test failure is certainly unrelated, I'll say this is ready for review 👍 |
legrego
reviewed
Oct 19, 2021
| @@ -65,47 +66,37 @@ export function deleteTestSuiteFactory( | |||
| doc_count_error_upper_bound: 0, | |||
| sum_other_doc_count: 0, | |||
| buckets: [ | |||
Member
There was a problem hiding this comment.
It might be good to memorialize these remarks as code comments so the next soul to wander in here has this context
Contributor
Author
|
@elasticmachine merge upstream |
Contributor
💚 Build Succeeded
Metrics [docs]Saved Objects .kibana field count
History
To update your PR or re-run it, just comment with: |
pgayvallet
approved these changes
Oct 19, 2021
Comment on lines
+791
to
+794
| const { buildNode } = esKuery.nodeTypes.function; | ||
| const match1 = buildNode('is', `${LEGACY_URL_ALIAS_TYPE}.targetNamespace`, namespace); | ||
| const match2 = buildNode('not', buildNode('is', 'type', LEGACY_URL_ALIAS_TYPE)); | ||
| const kueryNode = buildNode('or', [match1, match2]); |
Contributor
There was a problem hiding this comment.
🙈 (combined with the script param that removes documents not having a namespaces property)
legrego
approved these changes
Oct 19, 2021
jportner
added
the
auto-backport
Contributor
💔 Backport failed
To backport manually run: |
Contributor
Author
|
The Spaces API integration tests have changed in master but those changes were not backported to 7.x, which caused merge conflicts when attempting to backport this PR. Since this functionality isn't needed in 7.x (there are no aliases until 8.0) and the changes to the SOR are really minor, I think it's safe just to not backport this PR. |
jportner
added
backport:skip
Contributor
|
Seems good to me |
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Oct 19, 2021
…-link-to-kibana-app * 'master' of github.com:elastic/kibana: (30 commits) Fix potential error from undefined (elastic#115562) [App Search, Crawler] Fix validation step panel padding/whitespace (elastic#115542) [Cases][Connectors] ServiceNow ITOM: MVP (elastic#114125) Change default session idle timeout to 8 hours. (elastic#115565) Upgrade EUI to v39.1.1 (elastic#114732) [App Search] Wired up organic results on Curation Suggestions view (elastic#114717) [i18n] remove i18n html extractor (elastic#115004) [Logs/Metrics UI] Add deprecated field configuration to Deprecations API (elastic#115103) [Transform] Add alerting rules management to Transform UI (elastic#115363) Update UI links to Fleet and Agent docs (elastic#115295) [ML] Adding ability to change data view in advanced job wizard (elastic#115191) Change deleteByNamespace to include legacy URL aliases (elastic#115459) [Unified Integrations] Remove and cleanup add data views (elastic#115424) [Discover] Show ignored field values (elastic#115040) [ML] Stop reading the ml.max_open_jobs node attribute (elastic#115524) [Discover] Improve doc viewer code in Discover (elastic#114759) [Security Solutions] Adds security detection rule actions as importable and exportable (elastic#115243) [Security Solution] [Platform] Migrate legacy actions whenever user interacts with the rule (elastic#115101) [Fleet] Add telemetry for integration cards (elastic#115413) 🐛 Fix single percentile case when ES is returning no buckets (elastic#115214) ... # Conflicts: # x-pack/plugins/reporting/public/management/__snapshots__/report_listing.test.tsx.snap
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #115422.
Now when you delete the contents of a space (
deleteByNamespace), you also delete any legacy URL aliases for that space 👏