-
Notifications
You must be signed in to change notification settings - Fork 31
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
feat(webserver): implement content negotiation and deprecated endpoints #1072
feat(webserver): implement content negotiation and deprecated endpoints #1072
Conversation
61e7514
to
654b647
Compare
All of the actual meaningful change in this PR is contained within WebServer, RequestHandler, and AbstractV2RequestHandler classes/interfaces. The rest are essentially all redefining |
To test, pick a v2 endpoint like $ sh smoketest.sh
$ # in another terminal do:
$ https :8181/api/v2.1/discovery Authorization:"Basic $(echo user:pass | base64)" # should get a JSON response
$ https :8181/api/v2.1/discovery Authorization:"Basic $(echo user:pass | base64)" Accept:text/plain # should get the web-client index.html because content negotiation failed
$ https :8181/api/v2.1/discovery Authorization:"Basic $(echo user:pass | base64)" Accept:application/json # should get a JSON response The content negotiation behaviour of falling back to the web-client index.html doesn't seem great at the moment, so I'm looking into how to make that into a |
Generally fixed in the last commit. $ sh smoketest.sh
$ # in another terminal do:
$ https :8181/api/v2.1/discovery Authorization:"Basic $(echo user:pass | base64)" # should get a JSON response, this is implicitly using Accept:*/*
$ https :8181/api/v2.1/discovery Authorization:"Basic $(echo user:pass | base64)" Accept:text/plain # 406 because content negotiation failed (handler only produces application/json)
$ https :8181/api/v2.1/discovery Accept:text/plain #406 because content negotiation failed (handler only produces application/json) - maybe a 401 would make sense here too, but 406 seems fine
$ https :8181/api/v2.1/discovery Authorization:"Basic $(echo user:pass | base64)" Accept:application/json # should get a JSON response
$ https :8181/api/v2.1/discovery Authorization:"Basic $(echo user:pass | base64)" Accept:application/* # should get a JSON response
$ https :8181/api/v2.1/discovery Authorization:"Basic $(echo user:pass | base64)" Accept:*/* # should get a JSON response
$ https :8181/api/v2.1/nosuchhandler # 404
$ https :8181/api/v2.1/nosuchhandler Accept:application/json # 404 The corner case captured by the final two examples seems a little trickier to solve. There doesn't seem to be a nice way to ask the Vert.x Router if there is a matching route for a given request path, it just determines that internally and calls the handler implementation if there is one. Since paths can include parameters it isn't as easy as simply checking the request path (ex. |
Not sure why the itests keep hanging in CI. They run fine on my machine... |
d2a819f
to
b94bf34
Compare
f69f317
to
c1f0124
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.
Everything looks good, thanks for doing this!
I'm wondering about vert.x consumes
since you've included it in the PR along with produces
- is including consumes info in relevant endpoints something that should be done separately?
I can add it on here as well if you'd like. It's only really relevant for |
As an example, I modified the @Override
public List<HttpMimeType> consumes() {
return List.of(HttpMimeType.MULTIPART_FORM, HttpMimeType.URLENCODED_FORM);
} Then we have the following behaviours: Negotiation failure when POSTing JSON:
Success when POSTing urlencoded form:
and success when POSTing multipart form:
|
19e2b79
to
0fb9c42
Compare
src/test/java/io/cryostat/net/web/http/api/v2/RuleGetHandlerTest.java
Outdated
Show resolved
Hide resolved
2012a33
to
9fbeea0
Compare
…duces redefine existing mimeType() method as produces() which provides a List of HttpMimeType. Existing implementations simply wrap their old return values into a singleton List. The default implementation is an empty list, which means that the webserver ignores content negotiation for such handlers (ie. v1 handlers). For compliant handlers (all v2+), content negotiation will be used if the client specifies an Accept request header. If no Accept request header is given then the behaviour is unchanged from before. If the Accept header is given then the handler will only be called if an acceptable content type can be negotiated, falling back to the general 'GET /'-style handler if negotiation fails. There are no such examples yet, but any handlers which produce() multiple mime types should use ctx.getAcceptableContentType() to check what format they should respond with. The basic hook for consumes() on the other end of content negotiation is also included, but there are no implemented examples.
79261a0
to
54fb4cb
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.
LGTM! I will probably resume to #880 after the next release.
Fixes #1016
redefine existing mimeType() method as produces() which provides a List of HttpMimeType. Existing implementations simply wrap their old return values into a singleton List. The default implementation is an empty list, which means that the webserver ignores content negotiation for such handlers (ie. v1 handlers). For compliant handlers (all v2+), content negotiation will be used if the client specifies an Accept request header. If no Accept request header is given then the behaviour is unchanged from before. If the Accept header is given then the handler will only be called if an acceptable content type can be negotiated, falling back to the general 'GET /'-style handler if negotiation fails. There are no such examples yet, but any handlers which produce() multiple mime types should use ctx.getAcceptableContentType() to check what format they should respond with.
The basic hook for consumes() on the other end of content negotiation is also included, but there are no implemented examples.