Skip to content
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

Expose failure reason in ElideResponse #690

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

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.

@@ -121,7 +121,10 @@ public Response delete(
return build(elide.delete(path, jsonApiDocument, getUser.apply(securityContext)));
}

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.

@@ -212,7 +212,7 @@ protected ElideResponse handleRequest(boolean isReadOnly, Object opaqueUser,
}
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.

} 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.


/**
* Constructor.
*
* @param responseCode HTTP response code
* @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.

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.

5 participants