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

[Versioned APIs] PoC versioned routes system design #149943

Closed
wants to merge 24 commits into from

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Jan 31, 2023

Summary

Investigation into the work required to adopt (for consumers) or create (for us) a versioning system for our API routes.

Notes

  • This only focusses on the server-side declaration of routes
  • Do not assume that we will necessarily be doing this work

@jloleysens jloleysens added discuss Feature:http Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Jan 31, 2023
@jloleysens jloleysens self-assigned this Jan 31, 2023
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initial comments and questions

Method extends RouteMethod = 'get'
> = (
route: RouteConfig<P, Q, B, Method> & { version: Version },
handler: RequestHandler<P, Q, B, Context, Method>
Copy link
Contributor

@TinaHeiligers TinaHeiligers Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you given any thought to how we could use a versioned header rather than in the route config? Luke and I were briefly chatting about extending RequestHandler.Context to include the version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that could make sense!

No strong opinions here, I was think of a way to tie validation+handler to a version, so that's the function of passing the version value in here. I'm not convinced this is the correct API. I intend to document a few different approaches -- very much WIP.

@jloleysens jloleysens added the Epic:VersionedAPIs Kibana Versioned APIs label Feb 14, 2023
@jloleysens jloleysens changed the title [Versioned APIs] PoC versioned HTTP router design [Versioned APIs] PoC versioned routes system design Feb 14, 2023
Comment on lines 64 to 76
const versionedRoute = vtk
.defineRoute(myRouter.post, { path: '/api/my-plugin/my-route', options: {} })
.addVersion(
'1',
{
validation: { body: schema.object({ n: schema.number({ min: 0, max: 1 }) }) },
},
async (ctx, req, res) => {
logger.info(String(ctx.test));
logger.info(String(req.body.n));
return res.ok();
}
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very close to the design we came with for the initial versioned API RFC we presented last year.

The main difference is that my design was more tightly coupling the (unversioned) router and the versioned one (as the versioning API was accessible via something like core.http.createRouter().versioned.createRoute.

Overall I think fully separating the two is more elegant.

I would still avoid passing the router for each route definition.

Something like

const versionedApi = vtk.createVersionedAPI(router);
const versionedRoute = versionedApi
    .defineRoute({ method: 'POST', path: '...', blabla })
    .addVersion(same params as your example);

// fluent API ok, but also usable later
versionedRoute.addVersion(...)

(Still need to figure out what the best way to propagate/specify the method is, probably)

Note that the main risk I see with this approach is that we will be 'wrapping' our http router as any other non-Core consumer would, meaning that any capabilities required by the versioning API will have to be exposed publicly on our router. (I don't think this will be an issue, but it's definitely worth mentioning)

Copy link
Contributor

@rudolf rudolf Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like Pierre's suggestions 👍

I would use named params for addVersion similar to defineRoute

const versionedRoute = versionedApi
.defineRoute({
  method: 'POST',
  path: '...', blabla 
})
.addVersion({
  version: '1',
  validate: {...},
  options: {},
}, () => /*handler*/);

Copy link
Contributor

@rudolf rudolf Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is to something like

const casesRouter = core.http.createRouter('/api/cases');
const casesV1Router = vtk.createVersionedRouter(casesRouter, 'v1')
casesV1Router.get( /*api identical to what we have today*/)

Not sure I prefer it... it makes it easier to see everything that's included in V1 of the API, but harder to compare implementations between versions of the same endpoint. Probably the latter is more useful.

So I think I prefer starting with the route and then adding all versions to it instead of this example where we start with the version and then add routes. But thought I'll add it to the list of options.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's definitely another option.

My feeling is that it's most common to group per endpoint ( endpoint1 -> v1/v2/3 ) than per version ( v1 -> endpoint 1/2/3). I could be wrong though.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @jloleysens

@jloleysens jloleysens closed this Feb 20, 2023
jloleysens added a commit that referenced this pull request Feb 22, 2023
## Summary

This PR contains the initial designs for our versioned router API. This
contribution contains only types, any implementation will come in later
PRs.

Previous PR #149943

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Ahmad Bamieh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Epic:VersionedAPIs Kibana Versioned APIs Feature:http Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants