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

Use Zalando Problem for all json responses #6404

Closed
ecostanzi opened this Issue Sep 25, 2017 · 11 comments

Comments

Projects
None yet
5 participants
@ecostanzi
Contributor

ecostanzi commented Sep 25, 2017

Overview of the issue

I think it could be useful to choose a unique json format to handle API errors.

The exception handling uses zalando problem through the ExceptionTranslator.

{
    "type": "http://www.jhipster.tech/problem/contraint-violation",
    "title": "Method argument not valid",
    "status": 400,
    "message": "error.validation",
    "fieldErrors": [
        {
            "objectName": "todoListDTO",
            "field": "name",
            "message": "NotNull"
        }
    ]
}

The EntityResource controllers return the error details in the HTTP headers, e.g.

x-jtodoapp-error →A new todoList cannot already have an ID

the AccountResource has its own logic, for example for wrong credentials

{
    "AuthenticationException": "Bad credentials"
}

the Http401UnauthorizedEntryPoint returns the spring json default format

{
    "timestamp": "2017-09-25T10:06:18.672+0000",
    "status": 401,
    "error": "Unauthorized",
    "message": "Access Denied",
    "path": "/api/todo-items"
}

I'm wondering if there is some reason why the exception translator and zalando problem (or the ErrorVM before it) aren't use globally.

Motivation for or Use Case

Most of the APIs we generate with jhipster are exposed to external applications (both web and mobile). We noticed that even documenting these different behaviours it is still quite confusing for those who have to interact with the APIs.

Suggest a Fix

Exceptions could be used in all the generated controllers and in the accountResource, so that all the errors are handled by the ExceptionTranslator.
The Http401UnauthorizedEntryPoint could then use the object mapper to serialize a Problem object in the response body.

JHipster Version(s)

4.8.2

  • Checking this box is mandatory (this is just to show you read everything)

@cbornet cbornet self-assigned this Sep 25, 2017

@cbornet

This comment has been minimized.

Show comment
Hide comment
@cbornet

cbornet Sep 25, 2017

Member

I'm precisely working on this 😄
@henri-tremblay

Member

cbornet commented Sep 25, 2017

I'm precisely working on this 😄
@henri-tremblay

@ecostanzi

This comment has been minimized.

Show comment
Hide comment
@ecostanzi

ecostanzi Sep 25, 2017

Contributor

Good! :) Looking forward for it

Contributor

ecostanzi commented Sep 25, 2017

Good! :) Looking forward for it

@henri-tremblay

This comment has been minimized.

Show comment
Hide comment
@henri-tremblay

henri-tremblay Sep 25, 2017

Contributor

@cbornet Good, I'll let you work :-) I'm currently fighting with a bug in Problem serialization (sometimes, the status serializer isn't found at deserialization. I don't know why). Now #6409

Contributor

henri-tremblay commented Sep 25, 2017

@cbornet Good, I'll let you work :-) I'm currently fighting with a bug in Problem serialization (sometimes, the status serializer isn't found at deserialization. I don't know why). Now #6409

@henri-tremblay

This comment has been minimized.

Show comment
Hide comment
@henri-tremblay

henri-tremblay Sep 26, 2017

Contributor

One thing I have noticed before I forgot. If it can be of any use.

ConstraintViolationProblem is not serialized directly. It is transformed into a DefaultProblem. Which means we have one layer of indirection to get to the violations (parameters.fieldErrors instead of violations). My question is why not returning ConstraintViolationProblem? I don't think the ConstraintViolationProblemModule is used in any way because of that.

Contributor

henri-tremblay commented Sep 26, 2017

One thing I have noticed before I forgot. If it can be of any use.

ConstraintViolationProblem is not serialized directly. It is transformed into a DefaultProblem. Which means we have one layer of indirection to get to the violations (parameters.fieldErrors instead of violations). My question is why not returning ConstraintViolationProblem? I don't think the ConstraintViolationProblemModule is used in any way because of that.

@henri-tremblay

This comment has been minimized.

Show comment
Hide comment
@henri-tremblay

henri-tremblay Sep 26, 2017

Contributor

Oh... I know there was 2 things. So, the second thing I noticed is that a ThrowableProblem contains a stack trace which is then serialized. For security reasons, we avoid featuring internal implementations. But now it's all there in front of us. Is there a way to fix that? Maybe the ExceptionTranslator could squeeze the stacktrace?

Contributor

henri-tremblay commented Sep 26, 2017

Oh... I know there was 2 things. So, the second thing I noticed is that a ThrowableProblem contains a stack trace which is then serialized. For security reasons, we avoid featuring internal implementations. But now it's all there in front of us. Is there a way to fix that? Maybe the ExceptionTranslator could squeeze the stacktrace?

@atomfrede

This comment has been minimized.

Show comment
Hide comment
@atomfrede

atomfrede Sep 26, 2017

Member

The Jackson problem module has a property with stacktrace. We usually enable it on our dev environment and disable it on all others. Think we could do it too.

Member

atomfrede commented Sep 26, 2017

The Jackson problem module has a property with stacktrace. We usually enable it on our dev environment and disable it on all others. Think we could do it too.

@cbornet

This comment has been minimized.

Show comment
Hide comment
@cbornet

cbornet Sep 26, 2017

Member

The stacktrace should be disabled by default . @henri-tremblay do you have stacktraces in your responses ?

Member

cbornet commented Sep 26, 2017

The stacktrace should be disabled by default . @henri-tremblay do you have stacktraces in your responses ?

@cbornet

This comment has been minimized.

Show comment
Hide comment
@cbornet

cbornet Sep 26, 2017

Member

My question is why not returning ConstraintViolationProblem ?

Because it doesn't include the objectName field that we use. I think it could be added by PR to zalando's project.

Member

cbornet commented Sep 26, 2017

My question is why not returning ConstraintViolationProblem ?

Because it doesn't include the objectName field that we use. I think it could be added by PR to zalando's project.

cbornet added a commit to cbornet/generator-jhipster that referenced this issue Sep 26, 2017

Refactor AccountResource, UserResource and EntityResource to use Problem
Throw exceptions that will be treated by the ExceptionTranslator instead of returning ResponseEntity with an unchecked body
See jhipster#6404
@henri-tremblay

This comment has been minimized.

Show comment
Hide comment
@henri-tremblay

henri-tremblay Sep 26, 2017

Contributor

@cbornet Ok. Again something I don't get it seems. In integration tests, the stack trace is there. But indeed, it's not when calling the API on the running app. So all good (but I don't know why...)

About objectName, they used to have it but removed it on purpose. See zalando/problem-spring-web#86

Contributor

henri-tremblay commented Sep 26, 2017

@cbornet Ok. Again something I don't get it seems. In integration tests, the stack trace is there. But indeed, it's not when calling the API on the running app. So all good (but I don't know why...)

About objectName, they used to have it but removed it on purpose. See zalando/problem-spring-web#86

cbornet added a commit to cbornet/generator-jhipster that referenced this issue Sep 26, 2017

Refactor AccountResource, UserResource and EntityResource to use Problem
Throw exceptions that will be treated by the ExceptionTranslator instead of returning ResponseEntity with an unchecked body
See jhipster#6404

cbornet added a commit to cbornet/generator-jhipster that referenced this issue Sep 26, 2017

Refactor AccountResource, UserResource and EntityResource to use Problem
Throw exceptions that will be treated by the ExceptionTranslator instead of returning ResponseEntity with an unchecked body
See jhipster#6404

cbornet added a commit to cbornet/generator-jhipster that referenced this issue Sep 26, 2017

Refactor AccountResource, UserResource and EntityResource to use Problem
Throw exceptions that will be treated by the ExceptionTranslator instead of returning ResponseEntity with an unchecked body
See jhipster#6404
@cbornet

This comment has been minimized.

Show comment
Hide comment
@cbornet

cbornet Sep 26, 2017

Member

The concerns in zalando/problem-spring-web#86 seem legit.
That said if we want to remove FieldErrorVM and use violations directly, we also need the code that we use for translation (based on the user selected language, not the browser one). Maybe we can PR for that.

Member

cbornet commented Sep 26, 2017

The concerns in zalando/problem-spring-web#86 seem legit.
That said if we want to remove FieldErrorVM and use violations directly, we also need the code that we use for translation (based on the user selected language, not the browser one). Maybe we can PR for that.

cbornet added a commit to cbornet/generator-jhipster that referenced this issue Oct 2, 2017

Refactor AccountResource, UserResource and EntityResource to use Problem
Throw exceptions that will be treated by the ExceptionTranslator instead of returning ResponseEntity with an unchecked body
See jhipster#6404

cbornet added a commit to cbornet/generator-jhipster that referenced this issue Oct 2, 2017

Refactor AccountResource, UserResource and EntityResource to use Problem
Throw exceptions that will be treated by the ExceptionTranslator instead of returning ResponseEntity with an unchecked body
See jhipster#6404

cbornet added a commit to cbornet/generator-jhipster that referenced this issue Oct 2, 2017

@cbornet cbornet referenced this issue Oct 2, 2017

Merged

Use Problem Spring Security integration #6459

3 of 4 tasks complete

cbornet added a commit to cbornet/generator-jhipster that referenced this issue Oct 3, 2017

Refactor AccountResource, UserResource and EntityResource to use Problem
Throw exceptions that will be treated by the ExceptionTranslator instead of returning ResponseEntity with an unchecked body
See jhipster#6404

cbornet added a commit to cbornet/generator-jhipster that referenced this issue Oct 4, 2017

Refactor AccountResource, UserResource and EntityResource to use Problem
Throw exceptions that will be treated by the ExceptionTranslator instead of returning ResponseEntity with an unchecked body
See jhipster#6404

cbornet added a commit to cbornet/generator-jhipster that referenced this issue Oct 4, 2017

Refactor AccountResource, UserResource and EntityResource to use Problem
Throw exceptions that will be treated by the ExceptionTranslator instead of returning ResponseEntity with an unchecked body
See jhipster#6404
@cbornet

This comment has been minimized.

Show comment
Hide comment
@cbornet

cbornet Oct 5, 2017

Member

Fixed by #6459 and #6411

Member

cbornet commented Oct 5, 2017

Fixed by #6459 and #6411

@cbornet cbornet closed this Oct 5, 2017

danielpetisme added a commit to danielpetisme/generator-jhipster that referenced this issue Oct 11, 2017

danielpetisme added a commit to danielpetisme/generator-jhipster that referenced this issue Oct 11, 2017

Refactor AccountResource, UserResource and EntityResource to use Problem
Throw exceptions that will be treated by the ExceptionTranslator instead of returning ResponseEntity with an unchecked body
See jhipster#6404

@jdubois jdubois added this to the 4.10.0 milestone Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment