Skip to content

Conversation

@clayreimann
Copy link
Contributor

  • Provide hook for building custom responses in JsonApiEndpoint

- Provide hook for building custom responses in JsonApiEndpoint
@coveralls
Copy link
Collaborator

Coverage Status

Coverage decreased (-0.04%) to 71.589% when pulling f4e8f83 on add-failure-reason-to-ElideResponse into c421a63 on master.


private static Response build(ElideResponse response) {
return Response.status(response.getResponseCode()).entity(response.getBody()).build();
protected Response build(ElideResponse response) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does the failureReason get used? I see it being set, but never logged or added to a (presumably verbose?) response.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or is the plan that arbitrary endpoints could leverage this functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll be using it internally. Right now error cause is opaque to consumers of Elide unless you override a bunch of stuff.

tx.flush(requestScope);

ElideResponse response = buildResponse(responder.get());
ElideResponse response = buildResponse(responder.get(), null);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use an Optional here.

return new ElideResponse(responseCode, body, failureReason);
} catch (JsonProcessingException e) {
return new ElideResponse(HttpStatus.SC_INTERNAL_SERVER_ERROR, e.toString());
return new ElideResponse(HttpStatus.SC_INTERNAL_SERVER_ERROR, e.toString(), e);
Copy link
Member

Choose a reason for hiding this comment

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

What about graphQL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no real analog to ElideResponse in GraphQL because we're leveraging GraphQLJava. The Elide class drives JsonAPI and is ignored by GraphQL, also the GraphQLEndpiont can't be customized in the way that JsonApiEndpoint can.

@aklish
Copy link
Member

aklish commented Aug 8, 2018

Would be nice to have a single test for both JSON-API and GraphQL.

* @param body returned body string
*/
public ElideResponse(int responseCode, String body) {
public ElideResponse(int responseCode, String body, Throwable failureReason) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep the original public constructor too and deprecate it.

@clayreimann clayreimann force-pushed the add-failure-reason-to-ElideResponse branch from 0564449 to c8ebfbf Compare August 8, 2018 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants