Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 router implementation #153543
[HTTP] Versioned router implementation #153543
Changes from 17 commits
30c31e1
52aff31
e5a80bc
1e88fac
1d28474
b191455
3b74e08
d720fdf
2124cae
5a76452
dc6b41b
1890a96
51f63db
256a47f
78a9e59
02f7eef
a2601b2
929b3ef
7422334
9a751f2
f9241f6
879575b
f76be86
c83fdea
83d1d48
fd84e23
3a7dfb7
2bbc777
91d54bf
82a2147
3e79036
ae1a918
991984d
e7f3b6d
d0363a3
4ebba29
ff4c755
2970540
0cf0319
9e2e542
9ae403e
abb110d
7abb5da
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I expect most of the code in here will be implementing the actual API specification and making sure versioned routes adhere to it.
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.
Thinking out loud: we may want to just default to the latest version instead.
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.
Using
406
seems smart. I'd just want to confirm that we're totally fine using it here, as we're not per-say performing content negociation with like standard headers. But tbh I actually like it.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.
Thinking out loud: depending on whether we use semver and have some concept of fallback, we may want more logic than just direct get from the map. But we can't do better than that right now.
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.
💯
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.
So, my opinion regarding your thoughts:
First, I have to admit I missed that too when we were discussing about this. Kibana requests are instantiated directly with their validation schemas, which is a problem for us here...
Now,
I think the correct approach would be to have our own implementation of
KibanaRequest
that would wrap thisCoreKibanaRequest
. It would delegates (or 'proxy') most of the prop accesses to the underlyingCoreKibanaRequest
, except forparams
,query
andbody
. That would be the correct way to address the problem here.That being said, and even if what you did is more of a workaround/hack, I'd say it would be fine-ish if we want to keep it that way.
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.
Yeah, I was thinking about using a JS
Proxy
for this purpose. But I know it introduces a performance overhead for access. Manually wrapping felt like overhead.I'm happy to go the Proxy route if we feel performance does not matter (I think it is about 1/3 the speed of normal access). I concluded that it is better to leave our performance budget as intact as possible.
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.
I wouldn't use a proxy, just a boring, manually maintained wrapper with
get
-based accessors.But, again, just allowing to override the properties as you did is perfectly fine unless we find a reason to want more.
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.
:oh_no_you_didnt:
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.
😄 gotta love structural typing