Skip to content
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

[SO Tagging] Migrate the tag type to namespaceType : "multiple-isolated" mode #102870

Closed
pgayvallet opened this issue Jun 22, 2021 · 13 comments · Fixed by #106341
Closed

[SO Tagging] Migrate the tag type to namespaceType : "multiple-isolated" mode #102870

pgayvallet opened this issue Jun 22, 2021 · 13 comments · Fixed by #106341
Labels
Feature:Saved Object Tagging Saved Objects Tagging feature Feature:Saved Objects Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@pgayvallet
Copy link
Contributor

pgayvallet commented Jun 22, 2021

Part of #100489

As we'll potentially want to make the tag type shareable in the (near?) future, we need to change its namespaceType to multiple-isolated before 8.0

See #100489 for more detail about the reasons and conversion process.

See #54837 for full background information and detailed description of the changes to saved objects

@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Saved Objects Feature:Saved Object Tagging Saved Objects Tagging feature labels Jun 22, 2021
@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Jul 15, 2021

I've been looking into this issue and I'm not sure what the scope is here. Tags don't use the root-level references property access other saved objects using http through the tags_client. Does that need to change? What concerns me is the warning from the [Meta] issue:

If you are linking to other objects and you aren't using the root-level references field, it will break in 8.0.

If the intent of this issue is only to migrate the namespaceType, then the scope appears to be pretty small. @pgayvallet, can you please advise?

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Jul 19, 2021

From offline communication:

Tags themselves do not use references, however, saved object tags are referenced from other saved objects (dashboard, vis, map and lens).

In order to migrate saved objects that reference other saved objects, the objects being referenced must also be migrated to multiple-isolated.

In addition to the type change, we also need to refactor all soClient.get calls to use soClient.resolve api and handle the three potential outcomes of the resolution.

The UI uses a custom route for retrieving SO tags by ID. This route is handled by the tagsClient to retrieve tags by ID

@TinaHeiligers
Copy link
Contributor

@legrego @jportner In #100489 there's only an explicit instruction to migrate client side code but we're assuming we also need to handle server-side calls to soClient.get since there is a an equivalent server-side implementation of soClient.resolve. Are there any recommendations on how to properly surface resolution outcomes other than exactMatch, besides potentially logging cases for aliasMatch and conflict and optimistically returning the best match for conflict?

@jportner
Copy link
Contributor

there's only an explicit instruction to migrate client side code but we're assuming we also need to handle server-side calls to soClient.get since there is a an equivalent server-side implementation of soClient.resolve.

Sorry, that is worded incredibly poorly. If the client is calling the generic soClient.get API, you only need to change the client-side code to use the generic soClient.resolve API instead.
Oh the other hand, if the client is calling a custom route, you need to change client and server-side code to use soClient.resolve.

Are there any recommendations on how to properly surface resolution outcomes other than exactMatch, besides potentially logging cases for aliasMatch and conflict and optimistically returning the best match for conflict?

The recommendations are in a Google Slide and also (a little buried in) the POC here: #95958

In a nutshell:

  • exactMatch: show the page as normal
  • aliasMatch: calculate the new URL, and use the new spacesApi.ui.redirectLegacyUrl API -- that will 1. initiate a client-side redirect to the new URL, and 2. show a helpful toast message to the user
    image
  • conflict: at the top of the page, use the spacesApi.ui.components.getLegacyUrlConflict API to get a special callout component, and display that component at the top of your page:
    image

Note: the POC uses the spaces_oss plugin as a dependency instead of the spaces (x-pack) plugin, but they both provide the spacesApi start contract.

@joshdover
Copy link
Contributor

@jportner let me know if I'm understanding this correctly.

I believe we may be able to get away without implementing any UI or using the resolve API at all for tags if the following are true (which AFAIK, they are, but we'll need to audit):

  • Tags are not exposed in the public SavedObject API (except in the references field of other objects)
  • Tag IDs are never stored in any Kibana URLs
  • Tag IDs are only ever stored in the references field for all SOs that consume tags

It seems to me if those two are true for this SO type, then we should be able to rewrite all the references to tags in the same pass (which I believe is what the core migration for this already does) and all aliases would go unused since users can't have any bookmarks that point to legacy tag IDs. If that's the case, then I actually don't think we have to make any changes to how tags are retrieved. We also shouldn't need to actually update any UIs in Kibana to handle the conflict case, because there shouldn't be any user flow that could encounter that scenario.

To be safe, we could go ahead and use the resolve API and throw an error if there is a conflict. This would be to help with any 3rd party plugins that have integrated with tags. However, I actually don't think 3rd party integrations is possible today b/c you have to update an internal list of SO types that support tags to get everything working. If that's true, then I don't think we even need to do that work either.

In other words, I really think this may be as simple as changing the namespaceType to multiple-isolated for this particular SO type.

@jportner
Copy link
Contributor

  • Tags are not exposed in the public SavedObject API (except in the references field of other objects)

Not true, they are exposed / they are not a hidden type

  • Tag IDs are never stored in any Kibana URLs

True, we don't have any pages that I know of that can deep-link to a tag

  • Tag IDs are only ever stored in the references field for all SOs that consume tags

True

To be safe, we could go ahead and use the resolve API and throw an error if there is a conflict. This would be to help with any 3rd party plugins that have integrated with tags. However, I actually don't think 3rd party integrations is possible today b/c you have to update an internal list of SO types that support tags to get everything working. If that's true, then I don't think we even need to do that work either.

Agree

In other words, I really think this may be as simple as changing the namespaceType to multiple-isolated for this particular SO type.

Yeah I think so. Technically aliases will get created for tags, but we don't need to worry about using them.

I'm wondering if we should add some sort of flag at SO registration to allow consumers to "opt out" of aliases for that type when it is converted to become share-capable. I'm not sure if there are other cases where objects are not deep-linkable and don't need aliases to begin with. Thoughts?

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Jul 20, 2021

I believe we may be able to get away without implementing any UI or using the resolve API at all for tags if the following are true (which AFAIK, they are, but we'll need to audit)

So far, I've confirmed that the points are true for maps and lens.
update
The points are also true for dashboard, dashboards, dashboards using embeddables and visualizations.

In other words, I really think this may be as simple as changing the namespaceType to multiple-isolated for this particular SO type.

Yeah I think so. Technically aliases will get created for tags, but we don't need to worry about using them.

Based on the comments above, the immediate scope for this issue seems to only be changing the namespace type then. We can always revisit refactoring the api to use resolve if any consumers face issues.
@lukeelmers Does that seem reasonable to you too?

@joshdover
Copy link
Contributor

Not true, they are exposed / they are not a hidden type

Good catch, I had assumed since we had built dedicated APIs for this that it was hidden. I wonder if that was an oversight and it should be hidden: true?

I'm wondering if we should add some sort of flag at SO registration to allow consumers to "opt out" of aliases for that type when it is converted to become share-capable. I'm not sure if there are other cases where objects are not deep-linkable and don't need aliases to begin with. Thoughts?

That would make some sense to me. I'm already worried about the number of Saved Objects we're going to be creating with the aliases, so if we can reduce that number, that may be helpful. But yeah I agree, only if there are other use cases for this. This may be a somewhat special case?

@jportner
Copy link
Contributor

if we can reduce that number, that may be helpful. But yeah I agree, only if there are other use cases for this. This may be a somewhat special case?

Cool, I'll keep this in mind as I engage with other teams.

@TinaHeiligers
Copy link
Contributor

With dashboards, a potential issue might be with embeddables, where SO references are also injected. AFAICT, dashboards don't have pages that can deep link to a tag but I'm waiting for full confirmation of that from the app team.

@lukeelmers
Copy link
Member

AFAICT, dashboards don't have pages that can deep link to a tag but I'm waiting for full confirmation of that from the app team.

FWIW I am not aware of any embeddables where we would be including a reference to a tag - at this point I think the only integrations are in "listing" pages (visualizations list, maps list, SO list, etc). But still a good idea to confirm with the app team to be safe.

Based on the comments above, the immediate scope for this issue seems to only be changing the namespace type then. We can always revisit refactoring the api to use resolve if any consumers face issues.

++ Agree, unless the app team uncovers any surprises for us 😉

@jportner
Copy link
Contributor

Not true, they are exposed / they are not a hidden type

Good catch, I had assumed since we had built dedicated APIs for this that it was hidden. I wonder if that was an oversight and it should be hidden: true?

Sorry @joshdover I missed this when I replied to you. I am not sure what the intent there was. For the longest time, you couldn't view hidden objects in the Saved Objects Management page (or export them). This is no longer the case though as of #90178. In the issue linked to that PR, Rudolf specifically mentioned tags as a motivation:

Some saved object types would like to prevent users interacting with them through the auto-generated saved objects API but still allow users to import and export these objects for backup purposes.

Examples:

  • tags

So yeah, likely we can/should mark tags as hidden.

@joshdover
Copy link
Contributor

Sorry @joshdover I missed this when I replied to you. I am not sure what the intent there was.

No worries, that question wasn't really directed at anyone 😄 Yeah we can revisit that decision now that #90178 is solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Saved Object Tagging Saved Objects Tagging feature Feature:Saved Objects Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants