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

[Meta] Saved Object Tagging #74571

Closed
26 of 41 tasks
alexfrancoeur opened this issue Aug 6, 2020 · 56 comments
Closed
26 of 41 tasks

[Meta] Saved Object Tagging #74571

alexfrancoeur opened this issue Aug 6, 2020 · 56 comments
Assignees
Labels
Feature:Saved Object Tagging Saved Objects Tagging feature Feature:Saved Objects Meta release_note:enhancement Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@alexfrancoeur
Copy link

alexfrancoeur commented Aug 6, 2020

Summary

There are numerous deployments of the Elastic Stack across our community that have hundreds or thousands of saved objects with no good way to manage the content. This has been a longstanding request for quite some time, with past attempts that were put on hold. With the addition of Ingest Manager and Elastic Packages, it will become even more important to tag these system generated saved objects (#70461). With tagging, we aim to provide a simplistic way to organize, manage, import and export saved objects within Kibana. It’s possible that entities outside of saved objects would also require tags but for the MVP, we should aim to reduce scope as much as possible.

We will use this issue to track the requirements and status of the initial implementation of saved object tagging.

Feature requirements

  • Saved objects should be able to support one or multiple tags
  • Tags are mutable - strings and colors can change, but the id of that tag is still persisted with the associated objects
  • Tags should make it easy to group and sort on objects, whether that's in dashboards, Elastic packages with groupings of ML job configs, alerts, etc. or saved object management for quick export or copy to space
  • Tags should be consistent across solutions and applications within Kibana with a reusable and backwards compatible service that developers can trust
  • Tags should have a central area for management, with metadata, full CRUD capabilities and support for RBAC

Implementation Breakdown (WIP)

MVP

  • Base infrastucture - Platform
    • SavedObjects schema
    • Feature control integration
    • Server-side APIs (if any)
  • Tag Management UI - Platform
    • Create a tag modal - Core UI
      • Randomize color (copy / steal this from Spaces)
    • Edit tag modal
    • Table view
      • Includes: name, description, connections (tagged objects), shared spaces
    • Share & copy to space flyout
      • Not sure if this is going to be in 7.11, need to talk to Joe / Larry
      • Should be able to reuse existing UI with minimal changes
      • Defaults to including all tagged objects
  • Saved Objects UI - Platform
    • Add tags column
    • Filter by tags
  • Tag UI Service (components shared with plugins)
    • Design Plugin APIs - Platform
    • Assign a tag dropdown / autocomplete - Core UI
    • Create a tag modal (reuse from above) - Core UI
  • Integrate with applications - Core UI
    • Dashboard
    • Visualize
      • Sync with app team on priority
    • For each we need:
      • Add tags to list view table (column + filtering)
      • Add tags to saving workflow
    • This will require that we expose APIs from OSS to X-Pack for enhancing with tags

Future iterations

Questions

  • If we don't support export in MVP, will Ingest Manager be able to import tags as part of their packages? (Pierre)
  • Look into feature controls UI for tag & tag assignment permissions (Josh)
  • Check on bug where SO UI would export everything regardless of filters (Josh)

Flow diagrams

Saved object tagging@2x (1)

Screenshots / Prior art

All updated mockups can be found in this comment below: #74571 (comment)

94181547-a74db700-fe6d-11ea-91ce-f85eabdc471c

94185612-85573300-fe73-11ea-93db-7bb7562bb531

94185638-8c7e4100-fe73-11ea-8b4c-df0fbb38c579

There was some older work done on tagging from design and a recent space time project / POC (#71650, #16484 (comment)).

Personas and user stories

Persona User stories
Administrator As an administrator I can create, edit, delete and filter on tags and control access to who can do the same
End user (full access) As an end user, I can create, edit, delete and filter on tags
End user (write access for app, read only tags) As an end user, I can apply pre-existing tags to saved objects I create and filter by them
End user (read only access) As an end user, I can filter on pre-existing tags

Impacted parties or dependencies

Team / application name Areas of impact
Kibana Analyze This team will be affected most, their saved objects are in need of tagging. Saved searches, visualizations, dashboards, index patterns, saved queries, maps, canvas workpads, graph workspaces, and more.
Alerting Alerting supports tags, but they’re just strings. We’d like to support adding alerts to Elastic packages and at the point we can, treating these as saved objects with tags makes sense
Ingest Manager Ingest manager would like to use tags with their pre-packaged saved objects (#67258, #70461)
Machine Learning Machine learning jobs have tags and will be converting to saved objects (#64172). Ideally, these would support tagging
Observability Observability will be using alerts and pre-canned ML jobs, so we’ll need to support tagging here. As to whether or not other entities require tags, that’s probably out of scope for MVP
Security Solution Security will be using “alerts” in their threat detection engine and pre-canned ML jobs, so we’ll need to support tagging here. As to whether or not other entities require tags, that’s probably out of scope for MVP

Assumptions and open questions

  • The tagging MVP will be constrained to select saved objects
  • RBAC needs to be discussed in more detail
  • Ownership of implementation across other applications will need to be coordinated, planned and prioritized in cross-team roadmaps
  • There is an open question around using Elasticsearch _meta fields to unifying content across products. This type of standardization would enable "tagging" of Kibana saved objects with Elasticsearch assets like ingest pipelines, API keys, ILM/SLM policies, etc. If it makes sense for saved objects to adopt a similar approach, then we'll have to take tags into consideration as well.

Spaces

There are two high-level approaches to tag management with regards to spaces. We are leaning towards tags being multi-[name]space.

Tags are [name]space-agnostic; each tag that is created simultaneously exists in all spaces Tags are multi-[name]space; each tag that is created exists in one or more spaces
Pros: simplifies basic operations for saved objects (share, import/export) Pros: simplifies tag management (when an object is shared, the tag could be shared with it; “deleting” a tag in one space would have no effect on another space)
Cons: complicates tag management (e.g., admins in one space shouldn’t be able to delete a tag that is in use in another space), potential for information leakage when tags are created / doesn’t work with multi-tenant scenarios (e.g., one Kibana installation that has multiple tenants with their own space) Cons: complicates end-user experience (users can run into “tag collisions” where similar tags are created in different spaces and shared; tags lose meaning if users can’t differentiate)

cc: @arisonl @legrego @jportner @kobelb @ryankeairns @VijayDoshi @peterschretlen @AlonaNadler @joshdover @jinmu03 @mostlyjason @ruflin @rayafratkina

@alexfrancoeur alexfrancoeur added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Saved Objects labels Aug 6, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@ruflin
Copy link
Member

ruflin commented Aug 10, 2020

@alexfrancoeur About tags and spaces: I wonder if we could simplify things here by having tags by default in all spaces * (upcoming feature, but no the same as space agnostic). If we have tags in spaces, it might become odd that there are space agnostic saved objects which have different tags inside different spaces? Happy to take this conversation to an other issue to not "pollute" this one here.

@alexfrancoeur
Copy link
Author

@ruflin if you're up for an in person chat, I'd like to pick your brain about this over zoom. As a quick thought, I think we could possibly make a distinction between system generated saved objects and user generated saved objects and what their default experiences are. If we were to prioritize something like read only / system generated assets (#70461) then I can see the default experience to * spaces for read only tags shipped with packages. Given our security and saved object model today, it feels odd having user generated tags automatically be available in all spaces.

@joshdover
Copy link
Contributor

joshdover commented Sep 14, 2020

One thing that needs discussion is how we are going to handle sharing tags across spaces since there are a few unique attributes about tags that are different than other SO types. In the description above, we list these cons for space-specific tags:

Cons: complicates end-user experience (users can run into “tag collisions” where similar tags are created in different spaces and shared; tags lose meaning if users can’t differentiate)

The current share-to-space workflow has built-in handling for object collisions where two spaces contain objects with the same document ID. Depending on how we implement tags, we may have collisions on other fields than the _id field which will create a confusing UX if we do not explicitly handle these collisions as well.

For instance, if we use a schema like this (this is pseudo-code of sorts):

interface Tag {
  _id: string; // UUID
  attributes: {
    label: string; // user-visible tag label, should be unique per space
    color: string; // the tag's color in the UI
  };
  namespaces: string[];
}
interface TagAssignment {
  _id: string; // UUID
  attributes: {
    tag_id: string;    // UUID of the tag
    object_id: string; // UUID of the object that is being tagged
  };
  references: string[]; // includes the two UUIDs above
}

A few notes about this schema:

  • We don't use the tag label as the tag's _id so that tags can easily be renamed without having to update potentially 1000s of objects.
  • The primary reason for two separate SO types is so that we can properly apply feature controls for the desired UX. For example, users with 'All' permissions to TagAssignments and only 'Read' permissions to Tags should be able to assign tags to new dashboards, but not have the ability to create new tags.

The share-to-space issue arises when a user does something like:

  • In the default space, create a tag with the label "prod"
  • In the Marketing space, create a tag with the label "prod" (this will be a different tag object with a different _id)
  • User shares a dashboard tagged with the "prod" tag from the default space to the Marketing space.

In this case, the default space's "prod" tag should also be shared since it is a referenced object. However, there is already a "prod" tag in the Marketing space. Because the current system only handles _id collisions, this would not be detected in the current implementation and now the uniqueness constraint of tag labels within a space has been violated.

It seems to me we need to have explicit logic for this situation or else we are likely to introduce a confusing UX. For instance, if we didn't do anything here and simply shared the prod tag in the Marketing space (so now there are two "prod" tags in Marketing) the user would be presented with two tags of the same label in the tag assignment UI.

I believe we're going to need a "merge tags" UI flow for when this situation is encountered. I believe the only decisions the user would need to make are:

  1. Choose the color (only if the colors do not match between the conflicting tags); and
  2. Confirm that this is what they want to do (maybe unnecessary?)

It's also worth pointing out that tags are not the only SO type with a uniqueness constraint. Another one that comes to mind is Index Patterns. It is possible to have two index patterns in the same space with the same pattern, however it is quite confusing to the user and not functionally useful as far as I understand. Maybe the @elastic/kibana-app-arch can confirm here, but I suspect having a merge flow for Index Patterns would also be useful. If so, it may make sense to solve this problem more holistically by adding a new generic feature to SavedObjects for uniqueness constraints.

@jportner
Copy link
Contributor

jportner commented Sep 14, 2020

The current share-to-space workflow has built-in handling for object collisions where two spaces contain objects with the same document ID.

This isn't quite accurate.

  • Copy-to-space and import has built-in handling for object collisions for documents with the same id or originId (the latter of which is new concept introduced with the "Sharing saved objects" feature).
  • Share-to-space will not check for collisions based on a document's originId, and two multi-namespace (shareable) objects intrinsically cannot have the same id.

When I wrote users can run into “tag collisions” where similar tags are created in different spaces and shared, I didn't mean an ID collision. I meant a situation where there are two tags with different IDs that appear to be the same, because they have the same label. So, exactly what you described in more detail above 🙂

One thing I'm not sure of is if the Tag type would also need to include a reference to the TagAssignment type for import/export/share-to-space to work properly with references. If it does require bi-directional references like this, then this point is moot. @jportner do you have an answer on how this works?

Currently, when you export an object, only its outbound references are included. I think that's behavior that we want to keep in general, though we could make a special exception for just this TagAssignment object type.

I did introduce new rootSearchFields functionality to the saved object find operation that we should be able to use for this (e.g., find all TagAssignment objects that have outbound references to Dashboard 'Foo'). However, some exports may have tons of saved objects, and we'd need to run such an individual query for each one. Exports shouldn't be an extremely common occurrence, though, so it's probably OK if they are more expensive.

It seems to me we need to have explicit logic for this situation or else we are likely to introduce a confusing UX.
...
I believe we're going to need a "merge tags" UI flow for when this situation is encountered.

Agree.


Additional comments regarding your proposal:

If we implement a separate TagAssignment object, we will need to ensure we can always get rid of those when saved objects are deleted. As mentioned above, it also complicates document exports, and sharing will have the same issue.

@joshdover
Copy link
Contributor

joshdover commented Sep 14, 2020

This isn't quite accurate.

* _Copy-to-space_ and _import_ has built-in handling for object collisions for documents with the same `id` or `originId` (the latter of which is new concept introduced with the "Sharing saved objects" feature).

* _Share-to-space_ will not check for collisions based on a document's `originId`, and two multi-namespace (shareable) objects intrinsically cannot have the same `id`.

Thanks for clarifying here. I believe this doesn't change what we need to do here, as originId won't be useful for this case. Please let me know if I'm wrong. Good information to have though.

Just to confirm, what does share-to-space do when it encounters an ID collision? Does it always generate a new ID?

When I wrote users can run into “tag collisions” where similar tags are created in different spaces and shared, I didn't mean an ID collision. I meant a situation where there are two tags with different IDs that appear to be the same, because they have the same label. So, exactly what you described in more detail above 🙂

Yep, we're on the same page here. My intention was simply to explain this situation more thoroughly for those not in the technical weeds 😄

Currently, when you export an object, only its outbound references are included. I think that's behavior that we want to keep in general, though we could make a special exception for just this TagAssignment object type.

I did introduce new rootSearchFields functionality to the saved object find operation that we should be able to use for this (e.g., find all TagAssignment objects that have outbound references to Dashboard 'Foo'). However, some exports may have tons of saved objects, and we'd need to run such an individual query for each one. Exports shouldn't be an extremely common occurrence, though, so it's probably OK if they are more expensive.

I don't think this will ultimately be necessary and is probably a premature optimization. I was only trying to think through any other potential benefits of having a separate object but it sounds like this isn't one. I believe we can get around version conflicts with scripted updates or application-side logic for retries. I think the RBAC / feature control justification is good enough reason for separate objects. I'll update the parent comment with your info.

If we implement a separate TagAssignment object, we will need to ensure we can always get rid of those when saved objects are deleted. As mentioned above, it also complicates document exports, and sharing will have the same issue.

Definitely something we will need to consider. The easy way to solve this is to make tags a hidden type that can only be managed via the dedicated tags UI, however that has other downsides as well (not being included in the global SavedObjects UI & API, etc.).

@jportner
Copy link
Contributor

I believe this doesn't change what we need to do here, as originId won't be useful for this case. Please let me know if I'm wrong. Good information to have though.

I agree!

Just to confirm, what does share-to-space do when it encounters an ID collision? Does it always generate a new ID?

Share-to-space doesn't check for collisions based on originId. And multi-namespace saved objects cannot share the same ID, so there can't be any collisions based on id.

Copy-to-space and Import both have the top-level option to check for conflicts or to generate new IDs.

I don't think this will ultimately be necessary and is probably a premature optimization.

Unless I'm misunderstanding, I do think this will be necessary. When you 1. export/import, 2. copy, or 3. share a saved object, we want to include tags, according to the Flow Diagrams in the issue description.

So for instance when you export a Dashboard, we'll need to:

  • Find all outbound references (direct and transitive)
  • For each of those, find all inbound-referencing TagAssignment objects

@kobelb
Copy link
Contributor

kobelb commented Sep 14, 2020

If we implement a separate TagAssignment object, we will need to ensure we can always get rid of those when saved objects are deleted. As mentioned above, it also complicates document exports, and sharing will have the same issue.

Definitely something we will need to consider. The easy way to solve this is to make tags a hidden type that can only be managed via the dedicated tags UI, however that has other downsides as well (not being included in the global SavedObjects UI & API, etc.).

Couldn't the saved-objects themselves have a direct reference to the tag saved-objects? For example, Dashboards would directly reference the tag saved-objects?

@jportner
Copy link
Contributor

Couldn't the saved-objects themselves have a direct reference to the tag saved-object? For example, Dashboards would directly reference the tag saved-objects?

That's probably a better option than what I suggested, though my understanding is that currently we don't have any such "circular references" so we'd need to make some small tweaks to export/copy/share to account for that.

@kobelb
Copy link
Contributor

kobelb commented Sep 14, 2020

That's probably a better option than what I suggested, though my understanding is that currently we don't have any such "circular references" so we'd need to make some small tweaks to export/copy/share to account for that.

Would tags need to have an explicit reference to what they tag? We can always just query for which objects have a specific tag, when that's required...

@joshdover
Copy link
Contributor

Couldn't the saved-objects themselves have a direct reference to the tag saved-objects? For example, Dashboards would directly reference the tag saved-objects?

I believe this would either require that each object that wants tagging to add explicit support for it OR that we have a mapping field that applies to all SavedObjects, which has some licensing implications. Ideally, we build tagging in a way that can be turned on for any SavedObject with little to no work and also be disabled entirely if admins choose to do so.

Would tags need to have an explicit reference to what they tag? We can always just query for which objects have a specific tag, when that's required...

I believe this would require more work to support the "export everything with this tag" use case which I think is something we would want. @alexfrancoeur do you imagine this use case being a requirement in the MVP or near future?

@kobelb
Copy link
Contributor

kobelb commented Sep 14, 2020

I believe this would either require that each object that wants tagging to add explicit support for it OR that we have a mapping field that applies to all SavedObjects, which has some licensing implications. Ideally, we build tagging in a way that can be turned on for any SavedObject with little to no work and also be disabled entirely if admins choose to do so.

I was initially thinking that tags would be just another saved-object, and use the normal saved-object references for this relationship. However, that potentially has repercussions with the referential integrity checking which will be implemented as part of Sharing saved-objects in multiple spaces.

I believe this would require more work to support the "export everything with this tag" use case which I think is something we would want. @alexfrancoeur do you imagine this use case being a requirement in the MVP or near future?

After further thought, it would also potentially make implementing the following UI more complicated as well, since each tag explicitly denotes which saved-object it's attached to.

89560487-0f283f80-d7e5-11ea-99e8-e2b99bd33ce3

@joshdover
Copy link
Contributor

Unless I'm misunderstanding, I do think this will be necessary. When you 1. export/import, 2. copy, or 3. share a saved object, we want to include tags, according to the Flow Diagrams in the issue description.

So for instance when you export a Dashboard, we'll need to:

  • Find all outbound references (direct and transitive)
  • For each of those, find all inbound-referencing TagAssignment objects

I apologize, my response was confusing there. I meant that we do not need to optimize for the "version conflicts when there are simultaenous edits" problem I was talking about previously since there are other workarounds.

For import/export we will definitely need to add support for including inbound-references. Either way we structure the schema (TagAssignment or including the tag references in the objects themselves), we will need to do the reverse lookup to support both export use cases (export all objects with tag X; and export object Y with all associated tags).

@alexfrancoeur
Copy link
Author

I believe this would require more work to support the "export everything with this tag" use case which I think is something we would want. @alexfrancoeur do you imagine this use case being a requirement in the MVP or near future?

As useful as a feature as I think this is, I believe that the primary goal for MVP should be organization.

For import/export we will definitely need to add support for including inbound-references. Either way we structure the schema (TagAssignment or including the tag references in the objects themselves), we will need to do the reverse lookup to support both export use cases (export all objects with tag X; and export object Y with all associated tags).

Either approach will work, and at some point, I think this functionality will be necessary.

We'll be fine tuning MVP requirements next week and can update this issue accordingly.

@pgayvallet
Copy link
Contributor

pgayvallet commented Sep 24, 2020

'Quick' thoughts about the technical implementation options:

I apologize in advance for the structuring and the ordering that may be a little chaotic, but there is a lot to talk about here.

Lexicon:

  • SOC: SavedObjectClient
  • SOR: SavedObjectRepository
  • SOT: Saved Object Tagging Plugin

Regarding the TagAssignment approach versus direct references from SOs to Tags

I'm mixed on this one. using a n-n join table is definitely what I would be doing a relational database, but that's quite an uncommon pattern in a document-based DB (which, by itself, doesn't mean it is necessarily bad). Still, it seems to be the easiest way to plugin into our permissions system due to the tag vs assignment permissions.

Using TagAssignment as a 'join table' SO

My main concern if we go with two SO types, Tag and TagAssignment, is the interaction with spaces and permissions. Please correct the following statements if any, or all, are wrong:

  • to work properly, a SO and its associated TagAssignment(s) should be in the same namespace(s). that means that all share/copy to space workflows would need to be adapted
    • when I add a tag to a SO from a space, I create an assignment that will be assigned to the current space (or to all the spaces the SO is accessible from?)
    • when I share a SO to another space(s), I will need to ALSO share the tag assignment (and the tag?) to the same space(s)
    • when I copy a SO to another space, I should also copy the tag assignment (and the tag?) to the other space
      • or should I copy the tag assignment and point it to the existing tag (that probably don't work in some case because of permissions, as the Tag would not be visible in the namespace the SO has been copied to)

Note that the issue about having to share/copy the Tag to the space(s) the SO is shared/copied to is also present in the other approach.

  • if, instead, TagAssignment was to be namespace agnostic, we loose per-namespace feature control on the 'assign tag' action, which was, I believe, the main argument for this approach, so I think this is not an option. That also doesn't address the issue that the assignment, once copied or shared, might point to a Tag that is not visible/present in the destination space (unless tags are, also, namespace agnostic, which is probably not an option).

Using direct references from SO to Tag

This feels more natural to me, but has some downsides regarding feature control, and impacts of the main common queries we will be using when performing operations between SOs and their tags (see below for more details).

Regarding feature control, as we will not have the TagAssignment SO to use for tag assignment permission, we would have to manually manage that. Also, as these would be 'virtual' SO permissions, this would not be able to be handled under the hood by the security plugin, we will need to handle that in the tag SOC wrapper (that would use security for that).

Performing the most common queries / workflows

find SOs by criteria + tags

Even if that feature is not available in OSS, we will have to adapt the SOC.find API to have a additional tags option

SavedObjectClient.find({ 
  filter: 'some filter',
  ...someOtherArbitraryOptions,
  tags: ['tagId1', 'tagId2'] 
})

With tag assignment model

  1. Fetch the results as it is currently done
  2. Fetch all TagAssignment objects that are assigned to the fetched objects (could maybe be cached or something)
  3. filter the results depending on their actual assignments to the requested tags

Note: 2. and 3. could be done in the SOT client wrapper, keeping the SOR implementation clean.

With direct references model

  1. Adapt the SOR (getSearchDsl) to handle this new option

Note:

  • as SO feature control would not be available with this approach, we probably also need to fetch the tags to see which one are available in current user context?
  • as the implementation is in the SOR, searching by tag would work even if the SOT plugin is disabled. Not sure if this is a pro or a con

find / export all objects attached to given tag

With tag assignment model

  1. fetch all assignment for given tag using SOR.find
  2. fetch all SOs using SOR.bulkGet for the SO ids from the tag assignment
  3. (for export, if includeReferencesDeep is enabled) fetch the references of the objects as already done

With direct references model

  1. fetch all SO having a reference to the Tag SO

delete a tag by id

With tag assignment model

  • delete the tag SO object
  • delete all assignments SO objects referencing the deleted tag

Note: user would need write permission on BOTH the Tag and TagAssignment types to perform a tag deletion.

With direct references model

  • delete the tag SO object
  • remove the reference to the tag to all SO objects referencing it

Note: user would need write permission on the Tag type, and on ALL types that the tag has been applied to. Also, we will need to update all the objects referencing the tag. This seems to be a major con.

Using the export API

The export API is currently not extensible, as it's not directly exposed on the SO client. The SO wrapper enhancement pattern is not applied here (security and spaces wrapper are still used by the fact that the export scripts uses the SO client, but the 'which objects to export' is not extensible.

const rootObjects = await fetchObjectsToExport({
types,
objects,
search,
savedObjectsClient,
exportSizeLimit,
namespace,
});
let exportedObjects: Array<SavedObject<unknown>> = [];
let missingReferences: SavedObjectsExportResultDetails['missingReferences'] = [];
if (includeReferencesDeep) {
const fetchResult = await fetchNestedDependencies(rootObjects, savedObjectsClient, namespace);
exportedObjects = sortObjects(fetchResult.objects);
missingReferences = fetchResult.missingRefs;
} else {
exportedObjects = sortObjects(rootObjects);
}

With tag assignment model

  • If we are using the TagAssignment join, we will need to add APIs to wrap the export logic. One option would be to move the fetchObjectsToExport + fetchNestedDependencies part of the export logic to the SO client public API. That way, we could use a tag SOC wrapper to add the additional assignments + tag objects to the list of exported object before streaming it to the client.

As discussed with @joshdover on slack, I feel like adding a 'include inbound references' option to the export may not be is not a good option, because:

  • We only want inbound references that are TagAssignment (don't think this will even be used in any other use case)
  • Inbound references are not enough, as we also need to add the Tag (which is an outbound reference from TagAssignment). So we basically need 'inbound references with outbound references from them, but only one level deep please', which seem like a very specific need. This is why I would rather do something specific to Tagging types instead of trying to implements a generic solution for a single specific use case (but once again, I'm all ears for any counter argument)

With direct references model

  • If the SO-Tag relation a direct relation via the SO's references, I think it would work without modifications (as long as 'include references deep' is enabled, the tags will also be exported)

Using the import API

In both cases, I think that as long as the export API is properly adapted, The import API should not need any modification at all, as the assignments and/or the tags will be present in the exported dump.

Tag / Assignment API design

CRUD on Tag type

Probably no real problematic here. The SOT plugin will expose an API to perform crud operations on the Tag type on the server-side, with probably a client-side counterpart. This API will just use the SO client under the hood.

Changing tag assignment on a SO

This is where the challenging part regarding the API design is. I see two main options here, integrating that in the SOC API, or having it as a totally dissociated one.

Dissociated API

Adding or removing tags would be done using an API exposed by the savedObjectTagging plugin.

For example, when creating a new object, the consumer would call SOC.create, then savedObjectTagging.addTags.

Pros:

  • Slightly better isolation between the core SO API and the SOT one.
    Cons:
  • (way) worse developer experience
  • consumers will have to manually check when the SOT is not available (oss) or disabled (licensed) to not perform the call
  • the isolation is far from perfect, as there are still some use cases that will require to alter the SOR API. (For example SOR.find will still need to have a new tags option defined)

The cons are significants here, which is why I think the other option is preferable:

Integration in the SOC/SOR API

This would integrate the SO tagging feature into core's SOR.

For example, when creating a new object, the consumer would just call SOC.create({ tags: myTags, ...otherOptions}), and the repository would create the TagAssignment (or add the references depending on the chosen implementation) when/after creating the object.

We will have the same challenges the spaces plugin did regarding integration into the SOR:

Technically, I was thinking that we could get most of the work done using a new client wrapper provided by the SOT plugin (please invalidate and/or comment on any of the following, this is one of the most impactful part of the design)

Some examples: (note: this is based on the TagAssignment approach)

  • SOC.create:

    • we add a tags: string[] to the SavedObjectsCreateOptions
    • the SOR/SOC implementation of create actually don't care about this new option
    • the newly introduced SOT client wrapper would first delegate to the client to perform the default action (create the object), then, if successful, would create the TagAssignment to link the object to its tags.
  • SOC.delete

  • the SOR/SOC implementation of delete actually don't care about this new option

  • the SOT client wrapper would delegate to the client, then, on success, also delete the associated TagAssignments

Important notes on delete:

  • The user performing the SO deletion does not necessarily have write permissions on the TagAssignment SO type, but we probably still want to delete the assignments during the deletion (as it is functionally just a join/relation) . Would the SOT client wrapper be able to act with higher privileges here to do that, and would is be acceptable in term of security? @elastic/kibana-security WDYT?
  • If the SOT plugin is disabled during the deletion of an object with tags, the assignments are not doing to be removed. I'm actually unsure of the importance of this point (seems similar but less impactful than what caused spaces to have the implementation in core's SOR instead of their wrapper)? If this is blocker, I guess we will be kinda forced to have this logic in the original SOR instead (which probably invalidates the SOT client wrapper on its whole, forcing to have the impl in the SOR instead, as spaces did)

We would also add APIs to add/remove tags from a SO to the SOR (addTags(so, tags) and removeTags(so, tags)), Similar to addToNamespaces and deleteFromNamespaces that were added to the SOR for the multi-namespace feature. Note that a notable difference with the namespaces feature is that, I think, we could just have the SOR/SOC implementation be a no-op, and have the SOT client wrapper handle this logic. This still means that using the SOR directly will not properly handle tags (but I think this is the same for the other wrappers?)

@legrego @jportner @kobelb You probably are the ones with the most experience regarding SO client wrapper. I would need your opinion on that ^

Pros:

  • Better developer experience. This just 'feels right' imho
  • If we can keep most of the logic in the SOT client wrapper, we still got a good isolation

Cons:

  • Depends on the open questions.
  • Anything else?

oss - xpack separation / isolation

This is also a tricky part. the tagging feature is going to be under a basic (or more) license, meaning that the actual implementation needs to be (as much as possible) in xpack. However, both the SO management section and all oss apps that are going to use tags needs to still access the feature from OSS APIs, one way or the other.

Server-side

Already discussed in previous section, Integration in the SOC/SOR API. I don't think there is more to cover here.

Client-side

The most commonly used approach for this problematic is the 'shim'/'bridge' plugin.

We would have a soTaggingOss plugin in OSS, that would be a facade of the actual soTagging one (the xpack one). Consumers of the SO tagging feature from OSS would use this 'oss' plugin. It would also provide an API to know if the tagging feature is actually enabled or not (to show, or not, the tag selection in SO creation/edition pages for example).

The soTagging plugin, when present, would call a registration API of the soTaggingOss one to provide the actual implementation.

Regarding the UI components the plugin needs to expose to consumers (such as the tag selector one to be used for the various SO creation pages, or the tag representation), I'm unsure what the best approach is:

  • The components should either be in OSS (and re-exported from the xpack one), and would requires to wire manually to the corresponding APIs what would be exposed on both plugins, or
  • Preconfigured components could be exposed via both plugins contracts

@elastic/kibana-app-arch You are probably the ones with the most knowledge of that specific oss/xpack split subject, WDYT?

In short

I think I would go with the TagAssignment approach and try to put as much as possible into the new SOT SO client wrapper, but I would really like to have everybody's opinion here.

@MichaelMarcialis
Copy link
Contributor

MichaelMarcialis commented Sep 24, 2020

Hey, gang. We've been working on design concepts for this issue, and I'd like to share our current progress. You may find links to our Figma mockups and clickable prototypes below. The prototypes are set up as a choose-your-own-adventure, where you can select the area of interest and see how the addition of tagging will impact that area of Kibana. Additionally, I've posted some screenshots that spotlight some of the key points of the experience.

If you have any questions or feedback, don't hesitate to let me know (either here or via Figma comments). If anyone feels that we should get together for a quick meeting to review and discuss further, I'd be happy to throw something on the calendar. Thanks!

Assign Tags & Create Inline

Canvas

canvas

Dashboard

dashboard

Discover

Saved Search

discover-search

Saved Query

discover-query

Graph

graph

Machine Learning

machine-learning

Maps

maps

Visualize

visualize

Stack Management

Tags

Initial

Saved Objects

Initial

@kobelb
Copy link
Contributor

kobelb commented Sep 28, 2020

Thanks for your detailed analysis, @pgayvallet.

Still, it seems to be the easiest way to plugin into our permissions system due to the tag vs assignment permissions.

Do you mind elaborating on this? I get that we want a different subset of users to be able to manage tags vs actually tag a saved-object. Is there some other complication here?

to work properly, a SO and its associated TagAssignment(s) should be in the same namespace(s). that means that all share/copy to space workflows would need to be adapted

Assuming we use saved-object references and the restrictions put in place by them, yup!

This feels more natural to me, but has some downsides regarding feature control, and impacts of the main common queries we will be using when performing operations between SOs and their tags (see below for more details).

The biggest noted downside seems to be the delete operation. If we use a TagAssignment saved-object, it'll be a _delete_by_query for the TagAssignment. Where-as if we have the SO reference the tag directly, it'll be a _update_by_query for the SO.

All of the other common queries seem easier to me with having the SO directly reference the tags.

Regarding feature control, as we will not have the TagAssignment SO to use for tag assignment permission, we would have to manually manage that. Also, as these would be 'virtual' SO permissions, this would not be able to be handled under the hood by the security plugin, we will need to handle that in the tag SOC wrapper (that would use security for that).

I was originally thinking that if a user is able to edit a saved-object, it should be able to edit the tags assigned to the saved-object. For example, if you can edit a Dashboard you can add/remove tags. Does this allude to a requirement that I overlooked where we want a discrete privilege for whether or not a user can tag a saved-object?

Even if that feature is not available in OSS, we will have to adapt the SOC.find API to have a additional tags option

If we're using the method where the SO references the tags directly, we can use the hasReferences parameter to SavedObjectsClient::find

@pgayvallet
Copy link
Contributor

Do you mind elaborating on this? I get that we want a different subset of users to be able to manage tags vs actually tag a saved-object. Is there some other complication here?

I don't think there is. I lack knowledge of how we actually manage non-so-based permissions, but.I thought that directly using SO permissions was the easier way. But you probably know way better than me on that subject, so please tell me what you think.

All of the other common queries seem easier to me with having the SO directly reference the tags.

That's actually right. What I like the most about the assignment approach is the fact that we might be able to delegates most of the logic in the SOT SO client wrapper, which, I think, would be harder with the direct reference approach. But I might be wrong here, and would like more opinions on that specific part.

If we're using the method where the SO references the tags directly, we can use the hasReferences parameter to SavedObjectsClient::find

Ideally, I would like a better developer experience that having to find the id of the tag you want to search for. SO tagging is 'more' than just a plain relation between the object and its tag(s) and should have a better integration imho.

SOC.find({tag: 'my-tag-id'}) seems way better than SOC.find({hasReferences: [{type: 'tag', 'id': 'technicalId'}]})

The conversion from {tag: 'my-tag-id'} to {hasReferences: [{type: 'tag', 'id': 'technicalId'}]} could eventually be handled in the SOT client wrapper's find method though, leaving the SOR logic unchanged. It would still require to add the tag option to SavedObjectsFindOptions, which is in OSS.

@joshdover
Copy link
Contributor

Have more to add, but some initial thoughts:

Regarding tag permissions, these two statements may be resolved by the same question:

if, instead, TagAssignment was to be namespace agnostic, we loose per-namespace feature control on the 'assign tag' action, which was, I believe, the main argument for this approach, so I think this is not an option

I was originally thinking that if a user is able to edit a saved-object, it should be able to edit the tags assigned to the saved-object. For example, if you can edit a Dashboard you can add/remove tags. Does this allude to a requirement that I overlooked where we want a discrete privilege for whether or not a user can tag a saved-object?

What do we want the SO tagging permissions to look like?

  1. Do we want admins to be able to grant access to assign tags for any object in a space?; or
  2. Do we want users with write access for an SO type always to be able to assign tags to those objects?; or
  3. Do we want both, that is, users must have write permissions on the object being tagged and write permissions for assigning tags in general?
  • This is the most flexible option, but could also be quite confusing.

From a UX perspective, (2) is probably the most straightfoward approach. I could see an argument for only wanting some users to have access to tags, but this seems like a contrived use case to me. @alexfrancoeur any thoughts?

If (2) is deemed the appropriate UX, then I believe that makes the argument for TagAssignments much less potent. The remaining reason to do it that way is to be able to encapsulate most of the tagging functionality inside the SOT. This still feels like a good reason, but maybe not the best reason depending on what we're optimizing for.

delete a tag by id

With tag assignment model

* delete the tag SO object

* delete all assignments SO objects referencing the deleted tag

Note: user would need write permission on BOTH the Tag and TagAssignment types to perform a tag deletion.

We can probably craft the UX to make this always the case (+ server-side validation).

With direct references model

* delete the tag SO object

* remove the reference to the tag to all SO objects referencing it

Note: user would need write permission on the Tag type, and on ALL types that the tag has been applied to. Also, we will need to update all the objects referencing the tag. This seems to be a major con.

I believe we could circumvent this by using the internal user in this case. However, I do wonder if this could be seen as a 'permissions leak' that could confuse users. If we went this route, I think that at the very least, we'd need to show in the UI something like "this will remove tags from X other objects".

If the SOT plugin is disabled during the deletion of an object with tags, the assignments are not doing to be removed. I'm actually unsure of the importance of this point (seems similar but less impactful than what caused spaces to have the implementation in core's SOR instead of their wrapper)? If this is blocker, I guess we will be kinda forced to have this logic in the original SOR instead (which probably invalidates the SOT client wrapper on its whole, forcing to have the impl in the SOR instead, as spaces did)

This sounds like a classic relational cascading delete problem. If we did support cascading deletes on SO references in general, that could solve this problem. But it faces the same permission leaks as above.

Regarding the UI components the plugin needs to expose to consumers (such as the tag selector one to be used for the various SO creation pages, or the tag representation), I'm unsure what the best approach is:

* The components should either be in OSS (and re-exported from the xpack one), and would requires to wire manually to the corresponding APIs what would be exposed on both plugins, or

* Preconfigured components could be exposed via both plugins contracts

In the preconfigured case, would the OSS components just render nothing if the x-pack plugin is not enabled?

* If the SOT plugin is disabled during the deletion of an object with tags, the assignments are not doing to be removed. I'm actually unsure of the importance of this point (seems similar but less impactful than what caused spaces to have the implementation in core's SOR instead of their wrapper)? If this is blocker, I guess we will be kinda forced to have this logic in the original SOR instead (which probably invalidates the SOT client wrapper on its whole, forcing to have the impl in the SOR instead, as `spaces` did)

This doesn't feel like a blocker to me and there are plausible workarounds (detect orphaned tag assigments at startup and delete them). I'd also like to consider the option of not allowing this plugin to be disabled. It adds significant complexity on top of an already complicated feature.

Instead, I think we could get away with exposing a config option to disable the UI for adding tags, but I don't think we need to support x-pack distributions completely disabling this plugin. Spaces could not do this since Spaces fundamentally changes many parts of Kibana. But I think tags are more isolated so that we could hide that they exist without turning off the fundamental logic that makes them work.

@pgayvallet
Copy link
Contributor

@joshdover

This sounds like a classic relational cascading delete problem. If we did support cascading deletes on SO references in general, that could solve this problem

Our SO relations are parent->child (the parent got the relation to its children when in a relational DB, it's usually the opposite, at least in 1-n joins). Cascade delete would work when deleting a dashboard, to delete the references viz for example.

With the tag assignment model, the references to the SO and the tag obj on the assignments. I don't think cascade delete would help us when we delete the 'referenced' SO.

(With direct references, this issue doesn't exist)

In the preconfigured case, would the OSS components just render nothing if the x-pack plugin is not enabled?

I was thinking a an API that exposes the whole optional API.

The global idea would be:

// API definition, type is in the oss plugin
type SavedObjectTaggingApi {
   listTags(): Tag[];
   getTagSelectionWidget({initialValue, onChange, ...}): TagSelectionWidget // React.FC<TagSelectionWidgetProps>
    // [...]
}

// xpack plugin contract, re-using the definition type from the oss plugin.
type SavedObjectPluginStart = SavedObjectTaggingApi;

// oss contract
SavedObjectOssPluginStart {
   isTaggingEnabled(): boolean;
   // facade to the xpack API. is undefined if `isTaggingEnabled` returns false
   getTaggingApi(): SavedObjectTaggingApi? 
}

I'd also like to consider the option of not allowing this plugin to be disabled. It adds significant complexity on top of an already complicated feature. Instead, I think we could get away with exposing a config option to disable the UI for adding tags

That would make sense to me

that way consumers would just check if tagging is enabled before retrieving the API from the OSS bridge.

@pgayvallet
Copy link
Contributor

I'll just add that, in my opinion, if this feature was meant for OSS, and given the pros and cons, we would have gone for the new tag field without questions. That was I meant by the 'Our implementation should not be impacted by licensing decisions`. I know having this feature in x-pack has technical implications, and that ideally, we would have a way to extend any OSS API arbitrary, but that is unfortunately not the case, at least right now, for a lot of reasons (including the usage of typescript, AFAIK it would be easier with plain JS regarding api/options extension). Taking that into account, and excluding the option of just rewrite all from scratch (at least now) I feel like the best option in term of API definition, integration in the existing codebase, developer and user experience, is to just go as we would have been if the feature was in OSS.

But, once again, this is only my personal opinion.

@kobelb
Copy link
Contributor

kobelb commented Oct 5, 2020

I'll just add that, in my opinion, if this feature was meant for OSS, and given the pros and cons, we would have gone for the new tag field without questions.

Even if tags need to abide by all of the rules and access patterns that references currently do?

@pgayvallet
Copy link
Contributor

Even if tags need to abide by all of the rules and access patterns that references currently do?

Correct me if I'm wrong, but as tags are still distinct objects, the security rules and access patterns would still apply anyway, won't they? I mean, accessing the tags associated with an object is still a distinct SOC call, so access control is already in place, isn't it?

@kobelb
Copy link
Contributor

kobelb commented Oct 5, 2020

Correct me if I'm wrong, but as tags are still distinct objects, the security rules and access patterns would still apply anyway, won't they? I mean, accessing the tags associated with an object is still a distinct SOC call, so access control is already in place, isn't it?

That's correct. However, the fact that a saved-object references another saved-object has implications on the ability to share saved-objects in multiple spaces per #27004:

Before a saved-object can be shared to a space, all direct and transitive references to saved-objects must be shared. For example, dashboards have references to visualizations, which have a reference to an index-pattern. When a dashboard is shared to a space, all referenced visualizations and in-turn index-patterns must be shared before the dashboard is shared.

Additionally, when a saved-object is either created or updated and references new saved-objects, the references will be checked to ensure they exist in at least all spaces which the saved-object exists in. This will prevent saved-objects from being updated or created with broken references.

@pgayvallet
Copy link
Contributor

To be honest, I'm not 100% sure of the expected behavior when sharing to space. I guess we'll want to also share the associated tags, as I don't see multi-spaces objects referencing single-space tags (even if technically, that is possible and would mean some tags would only be visible in some spaces of the object), but maybe @alexfrancoeur can confirm that.

If that was the case though, I agree that it would be some additional change to the integrity check of the sharing features and is a con. However, as (if?) this integrity check is going to be done at the repository level, adding additional checks for tags seems not that impacting, as the logic is very similar (but I have to agree that it would work out of the box by using references)

@legrego
Copy link
Member

legrego commented Oct 6, 2020

To be honest, I'm not 100% sure of the expected behavior when sharing to space. I guess we'll want to also share the associated tags, as I don't see multi-spaces objects referencing single-space tags (even if technically, that is possible and would mean some tags would only be visible in some spaces of the object

I'd expect the tags to be shared as well. We would run the risk of introducing seemingly duplicate tags within the space (different IDs, same name), but we that's hopefully something we can make clear in the tag management interface. We could try to be smart and assign to existing tags if they share the same name in the target space, but that might be even more confusing.

Getting back to the question of adding tags as a top-level property instead of using references:

I've gone back and forth on this so many times. I'm not conceptually opposed to adding a tags property, but I understand the desire to create a generalized references solution that we can build upon.

That said, it might be easier to see what this generalized solution looks like once we've proved out the tagging feature a bit more and solicited feedback. I worry about rushing into the generalized solution now to find out that we created the wrong abstractions, but on a now larger scale.

If we're concerned about the additional class of bugs/complexity that come with re-using references, then we could start with adding the tags property at the top level, and eventually migrate convert them to proper "references" once we are comfortable doing so. We will have to introduce the concept of a core SO conversion as part of #54837, so the additional effort of moving tags from one location to the other would theoretically be reduced.

@pgayvallet
Copy link
Contributor

pgayvallet commented Oct 7, 2020

After a sync discussion during yesterday's platform team weekly with @elastic/kibana-platform, @kobelb @legrego and @alexfrancoeur, we came to the following conclusions:

  • The main pain points for the references approach was when functionalities were making a distinction between tags and other relations
  • The references approach, even if requiring overall more changes on the client-side part of the code, is more robust and future-proof, especially regarding SO referencial integrity that is going to be implemented to share SO across spaces.

Which is why we acted that:

  • We are going to implement the SO-Tag relation using the references array
  • To get rid of the technical pain points:
    • Tags will not be editable from the inspect page of the SO management section (would still be able to do so using the references field editable in this page)
    • The export / share to space / copy to space actions will not make a distinction between tags and relations. The current include related objects option will just consider tags a related objects.

@pgayvallet
Copy link
Contributor

pgayvallet commented Oct 8, 2020

Update of #79096: Tagging is now wired to dashboard and visualize listing views: When the x-pack savedObjectTagging is enabled, the listing pages for these two apps are displaying a tags column and a tag filter (which is properly wired to use tag references when searching from these pages).

Screenshot 2020-10-08 at 11 31 33

Now for the bad news: after that, I spent some time looking on how to wire tagging to the update / create modals to allow selecting tags when creating a dashboard or a visualization.

In short: I missed it in my initial lookup, but my fears about the SavedObjectLoader behavior regarding references is confirmed: all references are transient on the client side, meaning that they are all supposed to be injected to the SO's data when the SO loader loads the object using injectReferences, and then injected back into the raw saved object during save operations using extractReferences. Any references not collected by these methods is lost during a save/update operation.

The call chain is a pain to follow, but basically _serialize just injects the 'legacy' references (searchSourceFields, unresolvedIndexPatternReference) and then let injectReferences, which is provided by the child class, do the rest of the job. Any existing reference not handled/ by either of these methods will be erased during a create or update operation, which, of course, is the case for the tag references we are adding.

let { attributes, references } = savedObject._serialize();
if (extractReferences) {
({ attributes, references } = extractReferences({ attributes, references }));
}

savedObject._serialize = () => serializeSavedObject(savedObject, config);

export function serializeSavedObject(savedObject: SavedObject, config: SavedObjectConfig) {
// mapping definition for the fields that this object will expose
const mapping = expandShorthand(config.mapping);
const attributes = {} as Record<string, any>;
const references = [];

export function createSavedVisClass(services: SavedObjectKibanaServices) {
const SavedObjectClass = createSavedObjectClass(services);
const savedSearch = createSavedSearchesLoader(services);

export function createSavedDashboardClass(
services: SavedObjectKibanaServices
): new (id: string) => SavedObjectDashboard {
const SavedObjectClass = createSavedObjectClass(services);
class SavedDashboard extends SavedObjectClass {

(Note that using a new tags field instead of references would have similar consequences on the changes to the loader)

Of course, both dashboard and visualize are using the SO loaders to perform save operations, so if we want to get this done, I see two (+1) options, which are both quite impactful:

Adapt the base SavedObject class to add tag support

We would add tag support directly to the client-side SavedObject class/api.

(This all starts there: src/plugins/saved_objects/public/saved_object/saved_object.ts)

We would have to:

  • adapt the SavedObject (src/plugins/saved_objects/public/types.ts) interface to add the following methods (I'm afraid of directly accessible properties to avoid risks of conflicts when child classes inject their own attributes to the instance...)
    • getTags(): string[] // return the ids of the tags associated with this object
    • setTags(tagIds: string[]) // sets the tags associated with this object
  • adapt initializeSavedObject (or underlying call to applyESResp) to load the tag references into a private field accessible via the previous getter/setters
  • adapt saveSavedObject (or underlying call to _serialize) to reinject the tag references after calling extractReferences

Note that we have the same issue than security had when implementing the spaces in core's SOR: We can't have the risk for tags to be erased from the references when saving an object when the SOT plugin is (temporarily) disabled, meaning that we can't have any abstraction / delegation here. the savedObject OSS plugin WILL have code/logic only used for a licensed feature, as the tag extraction/injection mechanism MUST always be enabled (we can't delegate/bridge it to the SOT OSS plugin).

Adapt the sub-classes of SavedObject to manually add tag support to each of them

For example, we would adapt the SavedVis class (src/plugins/visualizations/public/saved_visualizations/_saved_vis.ts) to add tag support. We would adapt their extractReferences and injectReferences to have the tag extraction / injection logic in the implementation instead of the base / abstract SavedObject class.

I think I still prefer the first option, as the 'licensing' problem remains the same (we also can't disable the inject/extract logic here), and this just looks like code duplication to me, which leads to more errors and more maintenance (especially when we'll plug tagging to other apps).

Just let the teams handle that

The last option, that @kobelb suggested during a sync discussion, would be to just let the owning teams perform the required changes for each individual application.

For that, we could basically just implement an assign tag action to the SO management table rows, which would allow us to properly test the feature, and then, after merging the PR (or using a feature branch), let the other teams perform the changes on their own. Note that It doesn't really change the SO loader root issue that they will just have to face instead of us, and I think it will just 'force' teams to implement the second option to avoid touching the root SO loader classes.

ping @kobelb @joshdover @spalger @legrego

cc @elastic/kibana-app @alexfrancoeur @ryankeairns

@kobelb
Copy link
Contributor

kobelb commented Oct 8, 2020

Out of the options you've presented, I think I prefer the "Adapt the base SavedObject class to add tag support" option. I don't like that X-Pack concerns are leaking into OSS though. Building upon this option, do you think we could generalize this approach and add "saved-object decorations"? This way instead of having OSS explicitly aware of tags, they could be aware that there are various "saved-object decorations", and the only implementation of these at the moment is tags?

@pgayvallet
Copy link
Contributor

As discussed on slack, the decoration pattern seems like a viable way to improve SoC. It requires some preliminary work to allow the savedObjects plugin to pass down the this decorator registry to all SavedObject instances. I created #80063 that is the first step.

@ryankeairns
Copy link
Contributor

@pgayvallet here is a quick take on validation errors for tag naming.

Special characters

The before and after versions are displayed below. The EuiFormRow's helpText prop can be used to display the gray text, while the error prop can display the resulting red text. This does cause the form to shift down, which is unfortunate, however there is an "urgent" EUI issue that will ultimately provide a better solution.

Screen Shot 2020-10-08 at 5 06 49 PM

Max 50 characters

As for the max characters, let's set a maxlength on the input and block further typing. That should prevent the need to ever display the max characters error. In the event that it does not fully prevent this from occurring, we can likewise output an additional validation error message using EuiFormRow's error prop.

Preventing duplicates

If we decide to show a warning or error for duplicates (presumably within the given Space), then an EuiCallOut placed atop the form would be the way to go. Use color: "danger" if we decide to go with an error instead of a warning.

Screen Shot 2020-10-08 at 5 15 44 PM

For reference, here are the EUI form validation docs: https://elastic.github.io/eui/#/forms/form-validation

@MichaelMarcialis
Copy link
Contributor

A fair bit of the mockups themselves feel future facing and not necessarily mapped to the MVP requirements defined above.

@alexfrancoeur, et al: Let me know if it would be desirable to have a slimmed-down, MVP-only set of mockups (rather than the full vision) for these initial development efforts. I'd be happy to throw it together.

@alexfrancoeur
Copy link
Author

Thanks for the offer @MichaelMarcialis! After our discussion during the platform team sync I think we're in a good place. It's good to have the full vision for future work, I just think we needed to prioritize for MVP.

@alexfrancoeur
Copy link
Author

@pgayvallet @ryankeairns @myasonik @MichaelMarcialis @joshdover I think we're good to close out this MVP, what do you think? We already have #81980 opened to address future requirements (most of which have been addressed). The only other feature that is mentioned in this MVP is share to space from tag management, which I think we've decided we'll keep in SO management for now. Maybe we can open an issue to track separately.

@pgayvallet
Copy link
Contributor

Closing.

Created

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 Meta release_note:enhancement Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

10 participants