-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[HTTP] Versioned API router designs #151596
Changes from 1 commit
649c587
9d09f48
02f41b2
bd02291
e110070
5b80d9c
f70b66e
8b02ffd
313dd75
2c16429
c352f4c
fe9b46f
661d5f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,7 @@ import type { IRouter, RequestHandlerContextBase } from '@kbn/core-http-server'; | |
import type { VersionHTTPToolkit } from './version_http_toolkit'; | ||
|
||
interface MyCustomContext extends RequestHandlerContextBase { | ||
fooService: { create: (value: string) => Promise<void> }; | ||
fooService: { create: (value: string, id: undefined | string, name?: string) => Promise<void> }; | ||
} | ||
const vtk = {} as unknown as VersionHTTPToolkit; | ||
const router = {} as unknown as IRouter<MyCustomContext>; | ||
|
@@ -21,26 +21,62 @@ const versionedRouter = vtk.createVersionedRouter({ router }); | |
// @ts-ignore unused variable | ||
const versionedRoute = versionedRouter | ||
.post({ | ||
path: '/api/my-app/foo/{name?}', | ||
path: '/api/my-app/foo/{fooId?}/{name?}', // <= design mistake, {name?} should be a query parameter, but it has been released... | ||
options: { timeout: { payload: 60000 } }, | ||
}) | ||
// First version of the API, accepts { foo: string } in the body | ||
.addVersion( | ||
{ version: '1', validate: { body: schema.object({ foo: schema.string() }) } }, | ||
{ | ||
version: '1', | ||
validate: { | ||
params: schema.object({ | ||
fooId: schema.maybe(schema.string({ minLength: 10, maxLength: 13 })), | ||
name: schema.maybe(schema.string({ minLength: 2, maxLength: 50 })), | ||
}), | ||
body: schema.object({ foo: schema.string() }), | ||
}, | ||
}, | ||
async (ctx, req, res) => { | ||
await ctx.fooService.create(req.body.foo); | ||
await ctx.fooService.create(req.body.foo, req.params.fooId, req.params.name); | ||
return res.ok({ body: { foo: req.body.foo } }); | ||
} | ||
) | ||
// Second version of the API, accepts { fooName: string } in the body | ||
// BREAKING CHANGE: { foo: string } => { fooString: string } in body | ||
.addVersion( | ||
{ | ||
version: '2', | ||
path: '/api/my-app/foo/{id?}', // Update the path to something new | ||
validate: { body: schema.object({ fooName: schema.string() }) }, | ||
path: '/api/my-app/foo/{id?}/{name?}', // Update "fooId" => "id", this is not a breaking change! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is it not a breaking change? Our code won't be able to access There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. breaking changes refer to changes which break clients, we can always adapt our code to deal with "internal" breaking changes like this. So a client will still construct exactly the same query whether we map that path variable to |
||
validate: { | ||
params: schema.object({ | ||
id: schema.maybe(schema.string({ minLength: 10, maxLength: 13 })), | ||
name: schema.maybe(schema.string({ minLength: 2, maxLength: 50 })), | ||
}), | ||
body: schema.object({ fooString: schema.string() }), | ||
}, | ||
}, | ||
async (ctx, req, res) => { | ||
await ctx.fooService.create(req.body.fooString, req.params.id, req.params.name); | ||
return res.ok({ body: { fooName: req.body.fooString } }); | ||
} | ||
) | ||
// BREAKING CHANGES: | ||
// 1. Move {name?} from params to query (hopefully an uncommon change) | ||
// 2. Enforce min/max length on fooString | ||
.addVersion( | ||
{ | ||
version: '3', | ||
path: '/api/my-app/foo/{id?}', // Breaking change to the path, we move "name" to the query | ||
validate: { | ||
query: schema.object({ | ||
name: schema.maybe(schema.string({ minLength: 2, maxLength: 50 })), | ||
}), | ||
params: schema.object({ | ||
id: schema.maybe(schema.string({ minLength: 10, maxLength: 13 })), | ||
}), | ||
body: schema.object({ fooString: schema.string({ minLength: 0, maxLength: 1000 }) }), | ||
}, | ||
}, | ||
async (ctx, req, res) => { | ||
await ctx.fooService.create(req.body.fooName); | ||
return res.ok({ body: { fooName: req.body.fooName } }); | ||
await ctx.fooService.create(req.body.fooString, req.params.id, req.query.name); | ||
return res.ok({ body: { fooName: req.body.fooString } }); | ||
} | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside: What would the path be if
fooId
isn't defined?/api/my-app/foo/undefined/name
or/api/my-app/foo//name
or something else?