-
Notifications
You must be signed in to change notification settings - Fork 685
cbor content negotiation #35509
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
base: master
Are you sure you want to change the base?
cbor content negotiation #35509
Conversation
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.
Pull request overview
This pull request adds CBOR content negotiation support by moving the AcceptHeaderMatcher class to the public API and integrating it into the search request handling flow. The changes enable clients to request CBOR format responses via the HTTP Accept header.
Key Changes:
- Moved
AcceptHeaderMatcherfrom internal package tocom.yahoo.netand made it public - Integrated Accept header parsing in
SearchHandlerto automatically set CBOR format when preferred - Updated comments in
JsonRendererto reflect that format determination now includes Accept header logic
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vespajlib/src/main/java/com/yahoo/net/AcceptHeaderMatcher.java | Changed package from com.yahoo.document.restapi.resource to com.yahoo.net and visibility from package-private to public |
| vespajlib/src/test/java/com/yahoo/net/AcceptHeaderMatcherTest.java | Updated package declaration to match the new location |
| vespajlib/abi-spec.json | Added public API specification for the newly exposed AcceptHeaderMatcher class |
| vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java | Added import for the relocated AcceptHeaderMatcher class |
| container-search/src/main/java/com/yahoo/search/handler/SearchHandler.java | Added setFormatFromAcceptHeader method to determine response format from Accept header when no explicit format parameter is provided |
| container-search/src/main/java/com/yahoo/search/rendering/JsonRenderer.java | Updated comments to clarify that format is now set by SearchHandler based on both query parameter and Accept header |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
container-search/src/main/java/com/yahoo/search/handler/SearchHandler.java
Show resolved
Hide resolved
e218d00 to
095f525
Compare
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.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
container-search/src/main/java/com/yahoo/search/rendering/CborRenderer.java
Outdated
Show resolved
Hide resolved
container-search/src/test/java/com/yahoo/search/handler/SearchHandlerTest.java
Show resolved
Hide resolved
container-search/src/main/java/com/yahoo/search/rendering/CborRenderer.java
Show resolved
Hide resolved
33741fe to
4bd9490
Compare
4bd9490 to
cae71ab
Compare
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.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
container-search/src/test/java/com/yahoo/search/rendering/JsonRendererTestCase.java
Show resolved
Hide resolved
cae71ab to
6d25b2f
Compare
|
fyi @vekterli im in yo repo stealin yo code |
|
Question: should we care about setting Vary: Accept? |
johansolbakken
left a comment
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.
Nice, this looks good to me😊
bjorncs
left a comment
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.
LGTM 👍
I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.