search

SORMAS-Foundation / SORMAS-Project

SORMAS (Surveillance, Outbreak Response Management and Analysis System) is an early warning and management system to fight the spread of infectious diseases.
https://sormas.org
GNU General Public License v3.0
293 stars 143 forks source link

Build custom Error Details message for REST API #8934

Closed cazacmarin closed 2 years ago

cazacmarin commented 2 years ago

Situation Description

Currently, Sormas Rest API is handling a small range of errors which can be reported to the front(or service which is interrogating REST endpoints). If appear a scenario where a new specific error needs to be added, it is done in if/else block and those blocks will increase is size. Moreover, they contain only a specific text message.

Feature Description

General epic where this feature can be added is: #7408

As AlexV suggested, more description is inside:

  1. All rest controllers/resources need to extend from extends CustomizedExceptionHandler

  2. And CustomizedExceptionHandler need to extends from public abstract class ResponseEntityExceptionHandler

  3. In Sormas-Rest next dependency should be added:

    org.springframework spring-webmvc 5.3.4

  4. This dependency is linked to ResponseEntityExceptionHandler class

  5. CustomizedExceptionHandler class Will contain all builders for all exceptions. Some examples:

    
@ExceptionHandler(StorageFileNotFoundException.class)
public ResponseEntity<?> handleStorageFileNotFound(StorageFileNotFoundException exc) {
return ResponseEntity.notFound().build();
}

@ExceptionHandler(TokenIsNotValidException.class) public ResponseEntity handleTokenErrors( TokenIsNotValidException exception) {

var errors = Map.of( exception.getEntity().getSimpleName(),
        exception.getEntityId() );

ErrorDetails errorDetails = ErrorDetails
        .builder()
        .messageId( errorMap.get( TOKEN_IS_NOT_VALID_MSG ).getId() )
        .argumentsList( errorMap.get( TOKEN_IS_NOT_VALID_MSG ).getArgumentsList() )
        .message( TOKEN_IS_NOT_VALID_MSG )
        .details( errors )
        .build();
return ResponseEntity.badRequest().body( errorDetails );

}

@ExceptionHandler(EntityNotFoundException.class) public ResponseEntity handleEntityErrors( EntityNotFoundException exception) {

var errors = Map.of( exception.getEntity().getSimpleName(),
        exception.getEntityId() );

ErrorDetails errorDetails = ErrorDetails
        .builder()
        .messageId( errorMap.get(RECORD_NOT_FOUND_MSG ).getId() )
        .argumentsList( errorMap.get(RECORD_NOT_FOUND_MSG ).getArgumentsList() )
        .message(RECORD_NOT_FOUND_MSG )
        .details( errors )
        .build();
return ResponseEntity.badRequest().body( errorDetails );

}

@ExceptionHandler(UsernameNotFoundOrNoLongerActiveException.class) public ResponseEntity handleUsernameNotFoundOrNoLongerActiveErrors( UsernameNotFoundOrNoLongerActiveException exception) {

var errors = Map.of( exception.getEntity().getSimpleName(),
        exception.getParameter() );

ErrorDetails errorDetails = ErrorDetails
        .builder()
        .messageId( errorMap.get( USERNAME_NOT_FOUND_OR_NO_LONGER_ACTIVE ).getId() )
        .argumentsList( errorMap.get( USERNAME_NOT_FOUND_OR_NO_LONGER_ACTIVE ).getArgumentsList() )
        .message( USERNAME_NOT_FOUND_OR_NO_LONGER_ACTIVE )
        .details( errors )
        .build();
return ResponseEntity.status( HttpStatus.NOT_ACCEPTABLE ).body( errorDetails );

}

@ExceptionHandler(UserUnauthorizedException.class) public ResponseEntity handleUserErrors( UserUnauthorizedException exception) {

String email = (exception.getUserEntity() != null) ? (exception.getUserEntity().getEmail()) :
        (exception.getUserDetails() != null ? exception.getUserDetails().getUsername() : "");
String errorClassName = (exception.getUserEntity() != null) ? (exception.getUserEntity().getClass().getSimpleName()) :
        (exception.getUserDetails() != null ? exception.getUserDetails().getClass().getSimpleName() : "");

var errors = Map.of( errorClassName,
        exception.getMessage() );

ErrorDetails errorDetails = ErrorDetails
        .builder()
        .messageId( errorMap.get( USER_UNAUTHORISED_EXCEPTION ).getId() )
        .argumentsList( Arrays.asList( email ) )
        .message( exception.getMessage() )
        .details( errors )
        .build();
return ResponseEntity.status( HttpStatus.NOT_ACCEPTABLE ).body( errorDetails );

}

@ExceptionHandler(SameEntityExistsException.class) public ResponseEntity handleSameEntityExistsErrors( SameEntityExistsException exception) {

var errors = Map.of( exception.getEntity().getSimpleName(),
        exception.getParameter() );

ErrorDetails errorDetails = ErrorDetails
        .builder()
        .messageId( errorMap.get( RECORD_ALREADY_EXISTS ).getId() )
        .argumentsList( errorMap.get( RECORD_ALREADY_EXISTS ).getArgumentsList() )
        .message( RECORD_ALREADY_EXISTS )
        .details( errors )
        .build();
return ResponseEntity.status( HttpStatus.CONFLICT ).body( errorDetails );

}

And some other exceptions which needs to be defined based on scenario.

`Note!`
All the statuses are defined in those builders and can return different HTTP code:
ResponseEntity.badRequest
ResponseEntity.status( HttpStatus.NOT_ACCEPTABLE )
ResponseEntity.notFound()
ResponseEntity.status( HttpStatus.CONFLICT )

6.  ExceptionsTemplate file needs to be created.
It will contain all constants of possible exceptions raised in the system.
Some from them are:

public final static String TOKEN_IS_NOT_VALID_MSG = "Token is not valid"; public final static String RECORD_NOT_FOUND_MSG = "Record not found"; public final static String RECORD_ALREADY_EXISTS = "Record already exists"; public final static String PASS_NOT_MATCHING = "Password not match";


7.  As well this file will contain an errorMap variable where key value data will be stored.
Original exception message is in english language. It will contain key and error code. In case if front app will receive the error message in a english but regional will be set to german, front side will look for error code on their DB. If it is found than the german message will be displayed. If the server version is newer than client version and a new code was added, it will display the code in original english language. (Especially for Angular app it will help)

public static Map<String, ExceptionDao> errorMap = new HashMap<>();

public void populateExceptionMap() { errorMap.put( TOKEN_IS_NOT_VALID_MSG, new ExceptionDao( 1, new ArrayList<>() ) ); errorMap.put( RECORD_NOT_FOUND_MSG, new ExceptionDao( 2, new ArrayList<>() ) ); errorMap.put( RECORD_ALREADY_EXISTS, new ExceptionDao( 3, new ArrayList<>() ) ); errorMap.put( PASS_NOT_MATCHING, new ExceptionDao( 4, new ArrayList<>() ) );

errorMap.put( USER_UNAUTHORISED_EXCEPTION, new ExceptionDao( 5, new ArrayList<>() ) );//1 parameter
8.  Exceptions will be thrown from places where they are required. Some examples:

RoleEntity re = roleRepo.findByDefaultRole( String.valueOf( DefaultRoleName._USER ) ) .orElseThrow( () -> new EntityNotFoundException( RoleEntity.class, String.valueOf( DefaultRoleName._USER ) ) );

RecoveryTokenEntity dbRecoveryToken = recoveryTokenRepo.findOneByUserId( userId ) .orElseThrow( () -> new TokenIsNotValidException( RecoveryTokenEntity.class, resetUserPasswordDto.getRecoveryToken().toString() ) );

if (userRepo.findByEmailIgnoreCase( newUserDto.getEmail() ).isPresent()) { throw new SameEntityExistsException( UserEntity.class, newUserDto.getEmail() ); }

if (!authUtils.matchesPassword( updatableUserPasswordDto.getOldPassword(), user.getPassword() )) { throw new BadCredentialsException( UserEntity.class, USER_PASSWORDS_MISMATCH_OLD_CURRENT ); }

Optional userEntityOptional = userRepo.findByEmailIgnoreCase( email ); if (userEntityOptional.isEmpty() || userEntityOptional.get().getUserStatus() == UserStatus.INACTIVE) { log.info( "attempt to login with invalid or inactive username: {}", email ); throw new UsernameNotFoundOrNoLongerActiveException( UserEntity.class, USERNAME_NOT_FOUND_OR_NO_LONGER_ACTIVE ); }


Etc.

10. Note! This will be done on module based! First module will be like a proof of concept. Then, all other modules will come. In those modules separate tickets needs to be created.
11. New functional will be added to separate controller paths due to other modules interconnection and in order to not brake any things.

### Additional Details

We should no longer use the RolesAllowed annotation in sormas-rest and instead only properly handle the results given by the backend.
MartinWahnschaffe commented 2 years ago

@cazacmarin In sormas-rest we are so far using the ExceptionMapper of jax-rs (https://dennis-xlc.gitbooks.io/restful-java-with-jax-rs-2-0-2rd-edition/content/en/part1/chapter7/exception_handling.html). My first thought on this is that it would make more sense to go on with that approach instead of introducing a spring mechanism. Do you think it would be better to go with the approach you have described above? If so, why?

A small notice on the I18n stuff: I'd say we should use the existing approach based on I18nProperties. Maybe use an enum for the different error codes you mentioned.

cazacmarin commented 2 years ago

@MartinWahnschaffe As investigation took a bit longer, I will put as well 1st part of the investigation, which cannot be implemented, and 2nd one, which can.

Part 1-only to see how the investigation was.

I did some tests, in order to have a clean answer to right approach. General idea is to create builders. And those builders, will populate data from exception accordingly to error code and other details at the moment when exception is thrown. In order to implement it, is needed that all controllers will extend from CustomizedExceptionHandler class witch should extend org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler class. 1st and main problem which I saw: if, we are extending from javax.ws.rs.WebApplicationException in the manner which I described, call to the controller (any methods) will lead to 500 server error with general exception of injection. This one I cannot bypass in any manner.

2nd important problem is the generated output. We definitely need to replace javax.ws.rs.core.Response to org.springframework.http.ResponseEntity. Content generated by jax-rs library, contain much more details than needed. And builder, will be represented in toString format within section reason_phrase. It means that client side need to parse this exception, to extract from it details from toString method which can change in time. (not a good practice). Output generated files are the next one: response-with-javax.ws.rs.core.Response.json.log response-with-org.springframework.http.ResponseEntity.json.log

3rd At mapping exception with jax-rs, sometime we can have mapping exception. And only general message will be thrown. Builder message itself, will not be created. In order to fix it, additional library should be added(and this one needs to be tested to be sure that output exception was created by builder):

<dependency>
            <groupId>org.glassfish.jersey.core</groupId>
            <artifactId>jersey-common</artifactId>
            <version>2.22.2</version>
            <scope>compile</scope>
        </dependency>

Based on that, I can conclude that we need to proceed with spring library.

Part 2 - real implementation

Due to the way how sormas-rest is created and Swagger app how is configured and registering controllers, spring library cannot be used. Finally, I was able to do it with jax-rs. After a lot of issues, solution is the next one:

  1. sormas-api shared module will be used as an intermediate for accessing sormas-rest and sormas-backend project.
  2. sormas-api will contain entire custom CustomizedException mechanism.
  3. Customized error message can be send from both modules ( sormas-rest and sormas-backend)

I created the patch as a proof of concept and added it here: (annotations and inheritance as is, are very important to be there, otherwise, something will not work. #8934_-_Build_custom_Error_Details_message_for_REST_API.zip

Can be used as well translation from server side.

Output, looks like next one: image

ArgumetList var ca be used to customize some exception.

Afterword, if all is agreed, module by module tickets ca be opened and this mechanism can be applied for entire rest-api module.

cazacmarin commented 2 years ago

(moving ticket into wait column as Martin input is required on it. When we will agree that is is good to be implemented, all relevant tickets will be opened for rest APIs and this ticket will be closed.)

MartinWahnschaffe commented 2 years ago

Thanks for diving into this. I looked into your new proposal and the patch and have a few comments:

  1. Could you create and publish a branch for the patch here, so we can add comments to specific aspects of the code more easily?
  2. I don't get when we would need the ExceptionDto with the id and the arguments list. Do you have an example for this?
  3. getAllUuids should no return a bad request, when no data is found. It's still a valid request - there is just no data in the system
  4. In contrast it would make sense that getByUuid returns a 404
  5. I think we should map those error messages to http status codes
  6. Instead of using string constants in the ExceptionsTemplate class I'd suggest to use an enum class and thus also provide I18n
  7. Where are those authentication related constants like TOKEN_IS_NOT_VALID_MSG used? We probably need an exception mapper for authentication related exceptions, right?

All in all I think what we are still missing here is to take a step back to get an overview and define the following:

  1. Which exceptions are thrown from the backend and rest application?
  2. Which exceptions are missing and should be thrown? E.g. *Resource.getByUuid could throw an "EntityNotFoundException" or maybe a similar exception that already exists.
  3. For each of those exceptions we should define how they are mapped. Currently we only have a mapper for ConstraintViolationException and EJBAccessExceptionMapper
  4. This mapping could rely on an enum that defines all the potential ReST error results, including HTTP status code and an I18n message.

@vidi42 @JonasCir I'd also like your input on this. Probably worth to setup a meeting with the 4 of us once you have had a look at this. Maybe also Fredrik?

cazacmarin commented 2 years ago

Hi @MartinWahnschaffe. Some answers to your questions:

cazacmarin commented 2 years ago

image 2nd commit error message. In case if additional fields are still required, please let here a note.

FredrikSchaefer commented 2 years ago

Related to the 2. in the definition need pointed out by @MartinWahnschaffe , I would like to make a start.

When pushing invalid entities, users sometimes only get a 400 Bad Request returned. When you don't have access to the SORMAS server logs, this makes it very hard/impossible to see the cause of the problem. I think it would be very helpful, if instead an explanation of what's wrong with the pushed entity gets returned, so that the sender can find out what to fix. For some scenarios, this is already done (e.g. when not sending some mandatory attributes).

When a sending user does not have sufficient rights, currently only something rather generic gets returned (like 403 Forbidden). Maybe it would help to point out what right is missing? Like that, it would also be easier to distinguish user rights issues from server configuration issues, which might also lead to 403s.

JonasCir commented 2 years ago

In general I think we should get rid of our custom approach and stick with exception mappers. They are quite flexible and directly integrated into the stack.

cazacmarin commented 2 years ago

Based on https://stackoverflow.com/questions/3293599/jax-rs-using-exception-mappers - it is possible to map exception builders. @JonasCir - if you can, it is better to make a PoC as we will be able to compare which solution is better. In link provided, there were a lot of if/else statement, and not a normal, parametrized, with different arguments and if needed with internationalization thrown error.

When you will have a nicely solution as was provided by custom approach which can easy extend and have the same message format for all of the exceptions, please leave here a note, and I will be glad to take a look on it :) Thanks!

JonasCir commented 2 years ago

We can get rid of the if-else chains as jax will go through the super classes of an exception to find a matching handler, so we can start generic and then specialize if necessary. I think we can enforce a common scheme. For example, there is https://www.rfc-editor.org/rfc/rfc7807 which could help. I guess there will be a library or we can define a response entity class of a reasonable format which we serialize.

cazacmarin commented 2 years ago

Agree. All is possible :) - it is matter of time for new investigation. Due to that, I asked to have a branch with a new proposal, and only after, we can raise all pros and cons of both solutions. And only after, we can decide which one is the best one. And win solution will be followed. Hope it make sense for everybody :)

MartinWahnschaffe commented 2 years ago

Dropping this in favor of #10251