nf-tower icon indicating copy to clipboard operation
nf-tower copied to clipboard

Fail on error

Open pditommaso opened this issue 6 years ago • 2 comments

What's the benefit of using of disabling exception throwing on validation error and checking explicitly for validation errors, like here?

https://github.com/seqeralabs/nf-tower/blob/1df76e647e004ef3b17a7fb6b3c93f44df39c618/tower-backend/src/main/groovy/io/seqera/tower/service/UserServiceImpl.groovy#L67

I have the following it would be more robust and require less code just using default failOnError: true and checking for the validation error in the catch clause. For example:

https://github.com/seqeralabs/nf-tower/blob/48751aab02e77adb122d8e912a6165a99eff239f/tower-backend/src/main/groovy/io/seqera/tower/service/AccessTokenService.groovy#L55-L64

Then throwing a semantic exception that needs to be handled in the controller

https://github.com/seqeralabs/nf-tower/blob/48751aab02e77adb122d8e912a6165a99eff239f/tower-backend/src/main/groovy/io/seqera/tower/controller/TokenController.groovy#L64-L76

pditommaso avatar Jul 21 '19 15:07 pditommaso

This way we are able to build a custom error message for each violated constraint. There are other use cases too, for example, a service method saves more than one entity and we want to persist the ones which pass validation although some of them could fail (a thrown exception would rollback the entire transaction), but that case isn't present in our code. In my opinion, I wouldn't catch the exception in the service and then rethrow as you did. For me, the ideal solution (bearing in mind our current guideline of handling exceptions in the controller action) would be like this:

  1. Use save(failOnError: true).
  2. Catch the grails.validation.ValidationException in the controller actions.
  3. Build the error message there depending on the type of org.springframework.validation.Errors thrown. The error to string mapping is specified through a resource bundle (which allows i18n capabilities too).

This is the way it's done in the Grails framework, where all the error messages corresponding to each constraint violation are specified in messages files and there's a builtin "tag" to extract the corresponding message of an error. I don't see any standard mechanisms in Micronaut to handle this, but I think we could replicate this behavior if you find it convenient (what's missing is just de 3rd step from the list above).

tcrespog avatar Jul 22 '19 05:07 tcrespog

Hi Paolo. Going back to this subject, I've applied the proposal I told you about on 22 July to handle the validation errors of the workflow tags (still work in progress). You can give it a look in this commit: a839c01efcc19d6567acb500dc0bf4afaf08d3c7. As you can see all custom error messages are specified in resource bundle files following the format <entityName>.<fieldName>.<constraint>. You can even define generic error messages for each constraint defining a message in the bundle with just the constraint name. A validation error would be resolved to the generic message if there's no specific message defined. For example:

nullable=Field {0} cannot be empty

(The field name is automatically passed as the first argument ({0}) at the time the message is resolved).

What do you think about this way to handle entity validation errors? I think we could even refactor ValidationHelper class to leverage this.

tcrespog avatar Aug 21 '19 09:08 tcrespog