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

[discuss] Plugin / SavedObjects architecture #72028

Closed
rudolf opened this issue Jul 16, 2020 · 7 comments
Closed

[discuss] Plugin / SavedObjects architecture #72028

rudolf opened this issue Jul 16, 2020 · 7 comments
Labels
discuss Feature:Saved Objects Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@rudolf
Copy link
Contributor

rudolf commented Jul 16, 2020

SavedObjects are typically used as follows:

React Component
  --> SavedObjectsClient (browser)
    --> SavedObjects HTTP API (autogenerated)
      --> SavedObjectsClient (server)
        --> Security, Spaces, SavedObjectsRepository
          --> Elasticsearch

This pattern leads to an architecture with several drawbacks:

1. Business logic is embedded in React components

When React components directly call the SavedObjectsClient it means all business logic lives in these components themselves. This can make it hard to understand the business logic and makes it harder to test.

Moving the business logic out of components can be done in several ways:

  • creating an external "model" or a service which abstracts away the SavedObjects interactions and exposes a domain-based JS API
  • Use a state management library like Redux so that components can create actions without having to know how these are applied

2. It's impossible to share business logic between the browser and server

Plugins often have a need to enforce the same business logic on the browser and server. Subtle differences in the browser and server SavedObjectsClient makes it impossible to write a common abstraction.

3. The SavedObjects HTTP API cannot maintain backwards compatibility in minors

Because all of a plugin's Saved Object types and all their fields forms part of the API, it's impossible for plugins to insert an abstraction layer to maintain backwards compatibility while evolving how that plugin's state gets persisted. (Through migrations, the API can successfully migrate outdated documents before writing them, but this is only one way. It's not possible for consumers who only understand the outdated fields to continue to read from the API). This means that even though the API routes and methods are stable, more complex integrations that read from and transform Saved Object attributes are likely to break in every minor.

4. The SavedObjects HTTP API is "dangerous" for users to integrate with

Because the HTTP API doesn't apply any business logic or validation, users integrating with this API can easily create invalid Saved Objects. This can lead to an integration which seems to work, but when a user later tries to use a feature in Kibana that feature unexpectedly fails. Coupled with the fact that there can be breaking changes in a minor when reading from the API (see (3)), this can cause integrations to silently start failing after an upgrade.

5. The SavedObjects HTTP API is unintuitive for users to integrate with

SavedObjects are designed primarily around the requirements of the persistence layer (and to some extent the security layer, since security can only be applied on an per object-type level). This can lead to a "persistence model" that doesn't have an intuitive mapping to the Plugin's domain.

Kibana's Alerts HTTP API is a nice example of an API that exposes the domain.

- [`POST /api/alerts/alert/{id}/_enable`: Enable an alert](#post-apialertidenable-enable-an-alert)
- [`POST /api/alerts/alert/{id}/_disable`: Disable an alert](#post-apialertiddisable-disable-an-alert)
- [`POST /api/alerts/alert/{id}/_mute_all`: Mute all alert instances](#post-apialertidmuteall-mute-all-alert-instances)

Although it's possible to emulate the behaviour of POST /api/alerts/alert/{id}/_unmute_all with the SavedObjects HTTP API this would be much more work to implement for a user and importantly won't capture the domain language that allow users to relate domain concepts to API actions.

(Note I'm not trying to argue that all API's should avoid CRUD on Restful resources in favour of an RPC-like API)

Although Plugins are able to still provide an additional API as an alternative to the auto-generated SavedObjects API, in practise this rarely happens.

@rudolf rudolf changed the title [discuss] Plugin < SavedObjects [discuss] Plugin / SavedObjects architecture Jul 16, 2020
@rudolf
Copy link
Contributor Author

rudolf commented Jul 16, 2020

@streamich Let me know if I missed anything from our conversation.

@rudolf rudolf added discuss Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Jul 16, 2020
@elasticmachine
Copy link
Contributor

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

@rudolf
Copy link
Contributor Author

rudolf commented Jul 16, 2020

There are many ways to solve these architectural problems. But I wonder long-term if this pattern of using the "database" directly from the browser and the auto-generated CRUD API is suitable for plugins with complex domains. It feels like it's a big enabler for getting started while a plugin and it's domain is small, but could become hard to maintain and inflexible as the plugin grows and it's domain starts to become more and more complex.

@pgayvallet
Copy link
Contributor

While working on SO Tagging, I came to very similar observations. Overall, the generic SO http apis are not really suited for domain-based endpoints. Most obvious reasons being:

  • The fact that create and update APIs do not support validation, while being officially the 'good' way to interact with SOs caused our developers to move validation logic where they could, that being the client-side, which is, of course, a very bad practice for many reasons, the most significant one probably being that direct calls to the endpoint (or using the server-side API instead) just bypass such validation.

There was, at some point, per-attribute validation for SO in the legacy platform. We removed it however when we migrated, because there was no (as zero) usages of it. Also this validation system was very basic. It only allowed per field synchronous and static validation, which is just not good enough for any complex validation needs.

  • All the SO APIs (find, get, create, update) are interacting with 'raw' saved objects, when the SO data structure should remains an implementation detail of the data and/or service layer (service layer that is currently inexistent). This forced developers to implement dual way SO<->DTO (or view, or model, choose the name) transformations on the client-side. Main obvious example of that will be the savedObjects client-side plugin. These transformations should remain on the server-side too. Ideally, a specific SO type's get endpoint would return the object in the expected, ready to operate, data format.

We discussed it a bit with @kobelb, and my opinion on the subject is quite simple: If these generic SO apis that are capable of handling all SO types in a raw format probably made sense initially, it just doesn't scale or work anymore. First proof of that would be the multiplication of the hidden types, that are using this SO mode to totally bypass the SO apis and implement a better service/client layer in the middle, between the SO repository and their endpoints.

If the hidden mode does 'solve' the issue, it still kinda leaves the developers on their own, as they have to manually reimplement all endpoints, and interact with SOs using custom repositories, which is quite a bad developer experience in my opinion.

Also, hidden types are not exportable / importable using the associated APIs or the SO management section, which becomes more and more problematic (see #82020 for instance)

Overall, I think that these generic SO API don't really make sense anymore, and that we should get away from them.

The route/APIs I think are 'dangerous' and should be removed (or at least kept internal for some, see the SO management comment lower) from the 'generic' SO HTTP API are, to be changed for per-type routes instead are:

  • create / bulk_create
  • update / bulk_update
  • delete
  • get / bulk_get
  • find (see notes below)

There are some routes / API that intrinsically requires to be 'generic' and work with raw SO structure, and that would remain unchanged:

  • export
  • import

Note: The SO management section, by nature, would still need to have access to some of these generic APIs (find, get, delete, update). One option would be to move these routes to our /internal prefix. Another one would be to move these endpoints to the SO management plugin instead. The second option probably make more sense.

Question: Are there other cases where these API would still requires to work for multiple type? Are we, anywhere in the code, performing batch operations on multiple types at the same time for example?

The most controversial route now: find. Unfortunately, we still need this API for some use cases. For example for SO tagging, From the tag management section, I must be able to search for all assignable types of objects to list them and then assign tags to them (this is probably not the only usage). For this one, I think we would need to preserve the generic find API, while having per-type find that would properly return the object in their model format.

What tools could core provide

Excellent question. As already said, it's kinda already done by plugins/solutions that are using hidden types, and created and used custom clients and HTTP endpoints instead. So we could just say that this approach is the recommended one, and just stop there. Of course, this is not what we want, but the point is: it is already doable today.

Now, about the real question: what tools core could/should provide to help consumers create these new, per type, endpoints that matches their model.

I think that, ideally, we would be able to provide tooling to help consumers generates these SO 'clients' and associated http endpoints.

Clients

A basic example of a 'custom client' I did was for the tag type: https://github.com/elastic/kibana/pull/79096/files#diff-c03b7ac31839838bdea6f08abc67bcff2f3b1e21d9d5f94d91af3e36f0c0b947

As you can see, the base 'custom client' is quite simple: it basically works with the SO's attributes instead of the raw SO. Of course, this is a naive reduction, and we would need to be able to handle cases where the object's model is not just the SO's attribute

for CRUD operations, we could get away, I think, with basic converters.

A naive prototype would be:

type SavedObjectTypeClient<Model, Attributes, FindOptions> = {
   get(id: string): Model;
   update(id: string, update: Omit<Model, 'id'>): Model;
   find(options: FindOptions): FindResponse<Model>;
   // [...]
}


type ObjectToModelConverter<SOAttributes, Model> = (attributes: SOAttributes) => Promise<Model>;
type ModelToObjectConverter<SOAttributes, Model> = (model: Model) => Promise<SOAttributes>;
type ModelValidator<Model> = (model: Model) => Promise<ValidationFormatThatStillNeedsToBeDefined>;
type FindConverter<FindModelOptions> = (findOptions: FindModelOptions) => Promise<SavedObjectFindOptions>;

const myTypeClient = core.savedObjects.createTypeClient('myType', scopedSoClient, {
   convertModelToSavedObject,
   convertSavedObjectToModel,
   convertFindOptions,
   validateModel,
});

Of course, the create/update options are slightly different than the actual model (you can't specify the ID when creating an object, but the resulting Model would have it), but this could be just solved with TS types such as Omit<Model, 'id'> or the other way around. Anyway, this is not meant to be a working, final interface, just the big picture.

We would need to also allow consumers to extend these clients with custom methods if they need to. Unsure what the approach would be. Maybe core.savedObjects.createTypeClient should return a class instead of an instance to allow consumers to extend it (FWIW this is basically what the savedObject client-side plugin was doing)? Is there any other obvious solution?

HTTP Endpoints

This one might be a little trickier, for the only reason that we would need to specify the handlers validation in addition to the previous ModelValidator.

const myTypeClient = ... // previously created type client
core.savedObjects.registerRouteHandler({
  prefix: `/my_endpoints_prefix`,
  client: myTypeClient,
  schema: myTypeSchema, // kbn-config schema
  findSchema: myFindSchema
})

This would register all the basic routes (get, create, find and so on) for our custom type, to be directly usable

Remark: As there is a OpenAPI spec generation discussion in some other issue, We have to note that this generated endpoints approach would make it very tedious to use the AST parsing approach to generate the OpenAPI spec from our endpoints, as these endpoints would be 'dynamics'.

And on the client-side

I have no idea for now. Let's discuss about the server-side proposal first?

@kobelb
Copy link
Contributor

kobelb commented Nov 3, 2020

Question: Are there other cases where these API would still requires to work for multiple type? Are we, anywhere in the code, performing batch operations on multiple types at the same time for example?

When saved-objects have references to each other, this can introduce some complexities because developers would have to use the specialized clients to get the related saved-objects. Perhaps this should be solved by the programmatic abstraction that allows that saved-object to be referenced in the first place?

@pgayvallet
Copy link
Contributor

When saved-objects have references to each other, this can introduce some complexities because developers would have to use the specialized clients to get the related saved-objects.

TBH I would include that in the goals of these changes. For example for dashboard, the client-side could use a dashboard 'DTO' that includes the referenced visualizations. The server-side dashboardClient would then be in charge of constructing a correct model.

Note that even long term, I don't see the current SO APIs being removed from the server-side, meaning that a custom client for a type having various kind of references would still be able to use bulk_get or find under the hood. But removing the http endpoints would 'force' SO API consumers to migrate to this new design.

@rudolf
Copy link
Contributor Author

rudolf commented Sep 6, 2024

I'm glad to report that all browser plugins are now using dedicated domain based APIs. In #174497 we're working on exposing dedicated public APIs for the last saved object types. As such I think we've addressed the architectural shortcomings identified in this issue 🥳

@rudolf rudolf closed this as completed Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Feature:Saved Objects Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

4 participants