elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.66k stars 8.23k forks source link

[Discuss] API naming convention #52284

Open mshustov opened 4 years ago

mshustov commented 4 years ago

The current Kibana style guide enforces API format compatible with elasticsearch API: https://github.com/elastic/kibana/blob/master/STYLEGUIDE.md#snake_case

Kibana uses snake_case for the entire API, just like Elasticsearch. All urls, paths, query string parameters, values, and bodies should be snake_case formatted.

The reality that:

// or plugin B const data = await fetch('/api/pluginA'); data.some_property.name

- It enforces data transformation between the client and server sides. As a result, client and server sides have to adopt the shape of data for both environments. It's not that bad, but all consumers have to adapt their code not to handle elasticsearch API as a source of truth anymore. 
```js
licensing.features.my_feature // received from elasticsearch
licensing.features.myFeature // across the whole Kibana
licensing.features.my_feature // when plugin interacts with elasticsearch directly

Open questions

mshustov commented 4 years ago

@elastic/kibana-platform @epixa

pgayvallet commented 4 years ago

Being consistent in public API is the whole stack makes sense imho. However,

URLs

I don't see any difficulties in keeping our urls snake case, as this is something that does not depends on any data. An exception would be an url like /endpoint/{id}. even if id is, say, an uuid, we are already breaking the convention I guess?

Query params and body payload

This is where the difficulty is imho, as our API conventions kinda conflict with our JS naming conventions. I'm not even speaking about user input here, but just plain, static data structure.

Let say we have the following request that respect the API naming conventions:

POST  /api/some_endpoint
{
  some_property: 'someValue',
}

which would results in the following type:

type myDataStructure = {
     some_property: string;
}

is this was not constrained by the API convention, the type would probably have been

type myDataStructure = {
     someProperty: string;
}

However we are in JS. There is not such distinction as serializable vs not-serializable types. We don't have the tooling proper typed languages can offer such as field anotation, type converters and more. By default, our JS data structure is what got serialized.

So what are our options ? From what I see:

joshdover commented 4 years ago

A couple of thoughts:

There are other API niceties we'd like to add features for, at least for public APIs. For instance: OpenAPI spec, breaking change management / detection, etc. We could add an auto-magic renaming layer for public API routes that leverage these features. This automatic renaming could include a feature that allows you define which keys of the schema should and should not be renamed.

If we wanted automatic TypeScript support, we could leverage a TypeScript compiler plugin / transformer that would be able to generate the snake_case and camelCase versions of the interface.

All that said, building the tooling to make all this work, just to avoid the boilerplate of writing DTOs, is significant.


The complexity here stems from the mismatch between Elasticsearch's conventions and the JavaScript ecosystem's conventions. I'm like to push back on the assumption that our APIs need to be consistent with Elasticsearch. How valuable is this in the first place?

The primary instance where I would understand Kibana wanting to match Elasticsearch is APIs where part of the request body is actually an Elasticsearch query or filter. Mixing camelCase and snake_case in these requests would be awkward. However, taking a cursory glance at our currently available public REST APIs, none of them appear to expose raw Elasticsearch API interfaces in this way.

This begs the question: do we even plan to have public APIs that give raw access to Elasticsearch features? If not, it seems removing this snake_case API convention from Kibana would be more valuable than keeping it and building tooling to support it.

pgayvallet commented 4 years ago

Moving to a snake_case convention in JS code seems like a non-starter to me

+1

Writing DTOs by hand just to get the naming convention consistent seems like a terrible ROI to me.

+1

This begs the question: do we even plan to have public APIs that give raw access to Elasticsearch features? If not, it seems removing this snake_case API convention from Kibana would be more valuable than keeping it and building tooling to support it.

+100.

rudolf commented 4 years ago

Assuming we keep the API naming convention, I think the best solution is to honour the API convention above the JS naming convention. Although we can introduce renaming layers and typescript support (Saved Objects uses a type-safe renaming function) it still introduces complexity by introducing subtle differences in property names, network requests and the data that's persisted in ES. I'd rather have inconsistent property names than have to wonder whether something is snake case or camel case in each of the different contexts.

I also don't see the value in using the same API conventions as ES other than consistency for consistency's sake. Although consistency is more beautiful, I don't think it would have any impact on API consumers.

mshustov commented 4 years ago

However, taking a cursory glance at our currently available public REST APIs, none of them appear to expose raw Elasticsearch API interfaces in this way.

Is it a full list of Kibana REST API?

Btw there can see inconsistency even within a single endpoint:

  "updated_at": "2019-07-23T00:11:07.059Z",
  "version": "WzQ0LDFd",
  "attributes": {
    "title": "[Flights] Global Flight Dashboard",
    "hits": 0,
    "description": "Analyze mock flight data for ES-Air, Logstash Airways, Kibana Airlines and JetBeats",
    "panelsJSON": ....
    "optionsJSON": "{\"hidePanelTitles\":false,\"useMargins\":true}",
    "version": 1,
    "timeRestore": true,
    "timeTo": "now",
    "timeFrom": "now-24h",
    "refreshInterval": {
      "display": "15 minutes",
      "pause": false,
      "section": 2,
      "value": 900000
    },
    "kibanaSavedObjectMeta": {...
  }
epixa commented 4 years ago

The role APIs accept data for both Kibana and Elasticsearch roles, so it essentially just proxies some portion of the request body to Elasticsearch: https://www.elastic.co/guide/en/kibana/current/role-management-api-put.html

joshdover commented 4 years ago

Saved Objects uses a type-safe renaming function

@rudolf can you point me to this?

rudolf commented 4 years ago

https://github.com/elastic/kibana/blob/bd63596d184b145a5ebffeb91ce8b5e5318aca50/src/core/public/saved_objects/saved_objects_client.ts#L476-L492

An example of where we use it: https://github.com/elastic/kibana/blob/bd63596d184b145a5ebffeb91ce8b5e5318aca50/src/core/public/saved_objects/saved_objects_client.ts#L307-L320

joshdover commented 4 years ago

Current plan:

FrankHassanabad commented 4 years ago

So from earlier comment within most parts of securitySolutions such as detections we did follow the advice here: https://github.com/elastic/kibana/issues/52284#issuecomment-566593049

Where we kept our REST API to be snake_case on a best effort. We are revisiting some of this potentially here: https://github.com/elastic/kibana/issues/81964

Not all layers of the stack and conventions throughout Kibana follows this convention that we build our detections on or do not have public REST API's (some internal ones are using camelCase such as SO based REST ones from earlier) so what we have ended up with today is mixed cases throughout the code base with regards to camelCase and snake_case. In some instances we cannot easily abstract/translate between the snake_case and camelCase due to the nature of proxying things through something such as the SO objects which are mixed between camelCase, snake_case, and hypen-case so we adjust throughout the code base best we can. Hypen-case is more rare but not 0% within the SO objects.

What we have ended up with that are pain points generally are:

Things that would be helpful for us (or at least myself) ...

For our use cases and what is going on, I understand that the JS Eco system is mostly camelCase. If we could get "boundary" conversions automatically for serialization and de-serialization that would be incredibly helpful if people are very concerned about keeping things camelCase. This can be tricky still depending on if at some point you are dealing with mixed SO objects or mixed REST API's as the serialization, de-seralization might or might not be smart enough to know what to auto-covert correctly at this point but I don't think impossible. This might be tricky with passing through a KQL filter as another example, but maybe not impossible.

...or...

Keeping things mixed case and allowing camel + snake_case throughout the code base and discouraging transforms altogether might be another path. It seems like some people are approaching that while others are going the other direction but we adjust depending on where we are within the codebase.

mshustov commented 4 years ago

@kobelb @stacey-gammon considering all the pain points outlined above, what do you think about allowing snake_case format in the code base? Right now we allow it for object properties only https://github.com/elastic/kibana/blob/c7786463183141f08f72deaefc75a5707b55b5b6/packages/elastic-eslint-config-kibana/typescript.js#L142 but not for variables, parameters, etc. which enforces solution teams either to implement those manual conversions or mute linter errors anyway. It definitely will increase entropy in the code base, but we already have the same problem:

Since parts of Kibana and other teams are mixing snake_case and snakeCase as well (probably these same reasons), if we get an object or property of an object that is snake_case we just move forward and turn off any linter errors if they come up. However, the codebase at this point is a mixture of something like 90-95% camelCase and 5-10% snake_case. Some hypen-case but not a whole lot.

We have defined our "schema" within that as mostly camelCase to keep with the alerting convention but even within the alerting SO it does mixed between camelCase and snakeCase which adds to the inconsistency and issues of deciding which ones to keep between which naming type.

Switching to snake_case might be a significant effort. Instead, we could provide guidance when snake_case is preferred: when working with stack API, read/write SO, access config value, etc.

kobelb commented 4 years ago

Long-term, I think we should do what @joshdover previously stated and invest in tooling to do the automatic conversion from our HTTP API standards which use snake_case to our code standards that use camelCase. I haven't seen a compelling reason for us to consider switching the HTTP API standards to use camelCase, except it's easier to do so with our current level of tooling.

Short-term, I'm extremely hesitant to completely remove our eslint protection disallowing snake_case for all code. This moves us further away from where we want to be long-term and allows code using snake_case to proliferate. If a team feels that they are being hindered enough by disabling the eslint rule on a per-file/per-line basis, we can use eslint overrides to remove this rule for a plugin/folder: https://github.com/elastic/kibana/blob/master/.eslintrc.js#L84

mshustov commented 4 years ago

@kobelb JFYI: TS v4.1 adding support for key re-mapping https://devblogs.microsoft.com/typescript/announcing-typescript-4-1/#key-remapping-mapped-types which could help us to automate type conversion

If a team feels that they are being hindered enough by disabling the eslint rule on a per-file/per-line basis, we can use eslint overrides to remove this rule for a plugin/folder: https://github.com/elastic/kibana/blob/master/.eslintrc.js#L84

Then we need to state in our docs that we are okay with such approach.