search

zalando / problem-spring-web

A library for handling Problems in Spring Web MVC
MIT License
1.03k stars 125 forks source link

Add localization using MessageSource #518

Closed Opa- closed 3 years ago

Opa- commented 4 years ago

Hi there 😃

I was interested in introducing this library on the current project I work for. After some digging on others issues, there might be a gap between what this library is for and what I want to do. My use cases would be to:

The library covers the first two cases. For the third one, I've read both https://github.com/zalando/problem-spring-web/issues/115#issuecomment-334917572 and https://github.com/zalando/problem-spring-web/pull/118 and I still do not understand why we wouldn't use the Problem to address end-user validation ?

For the last one, it works out of the box for violation messages because the generation of such messages are not handled by the library. For custom Problem, I managed to localize some fields using some Aspect and reflection but this is not very clean and this is additional code I'd have to maintain inside a dedicated library to be used by all our different applications. Here's a little snippet of what I did to enable localization using MessageSource on title and detail fields while doing my POC:

@Aspect
@Component
public class DummyAspectException {
    private final MessageSource messageSource;

    public DummyAspectException(MessageSource messageSource) {
        this.messageSource = messageSource;
    }

    @AfterThrowing(value = "within(@org.springframework.web.bind.annotation.RestController *)", throwing = "throwable")
    public void localize(JoinPoint thisJoinPoint, Throwable throwable) {
        if (throwable instanceof AbstractThrowableProblem) {
            AbstractThrowableProblem throwableProblem = (AbstractThrowableProblem) throwable;
            localizeField(throwableProblem, "title", throwableProblem.getTitle());
            localizeField(throwableProblem, "detail", throwableProblem.getDetail());
        }
    }

    private void localizeField(AbstractThrowableProblem throwableProblem, String fieldName, String messageSourceKey) {
        String localizedField;
        try {
            localizedField = messageSource.getMessage(messageSourceKey, null, LocaleContextHolder.getLocale());
        } catch (NoSuchMessageException ignore) {
            return;
        }
        Field field = ReflectionUtils.findField(throwableProblem.getClass(), fieldName);
        field.setAccessible(true);
        ReflectionUtils.setField(field, throwableProblem, localizedField);
    }
}

It works, and apart the fact that it's not clean (aspect and reflection never are I guess 😆), the thing that worries me the most is that you clearly specified in other issues that Problem is not meant to be used as end-user validation. Would that make sense to use and extend the Problem library for front-end display purposes (for both end-user validation on forms and "single error display") or am I missing something and I should look to other libraries ?

whiskeysierra commented 4 years ago

For the third one, I've read both #115 (comment) and #118 and I still do not understand why we wouldn't use the Problem to address end-user validation ?

Those comments are explicitly about ConstraintViolationProblem, not about all problems in general.

Opa- commented 4 years ago

Would you be open to introduce something else as part of the library for end-user validation ? If I understood correctly the problem is more about the semantic of ConstraintViolationProblem. A great target output IMHO would be something like the json described in the readme of this library: https://github.com/jirutka/spring-rest-exception-handler

{
    "type": "http://example.org/errors/validation-failed",
    "title": "Validation Failed",
    "status": 422,
    "detail": "The content you've send contains 2 validation errors.",
    "errors": [{
        "field": "title",
        "message": "must not be empty"
    }, {
        "field": "quantity",
        "rejected": -5,
        "message": "must be greater than zero"
    }]
}

And regarding the localization of Problems in general, are you interested in adding this to the spring library of Problem ?

whiskeysierra commented 4 years ago

Form validation is topic with a level of complexity that is easily underestimated. It's also not really the scope of this library.

Having said that, the design of advice traits allows to plug that in from the outside. If you write your own library for that, based on problem spring web, I'm happy to advertise it here, every chance I get.

On Thu, 10 Sep 2020, 15:21 Vincent Letarouilly, notifications@github.com wrote:

Would you be open to introduce something else as part of the library for end-user validation ? If I understood correctly the problem is more about the semantic of ConstraintViolationProblem. A great target output IMHO would be something like the json described in the readme of this library: https://github.com/jirutka/spring-rest-exception-handler

{ "type": "http://example.org/errors/validation-failed", "title": "Validation Failed", "status": 422, "detail": "The content you've send contains 2 validation errors.", "errors": [{ "field": "title", "message": "must not be empty" }, { "field": "quantity", "rejected": -5, "message": "must be greater than zero" }] }

And regarding the localization of Problems in general, are you interested in adding this to the spring library of Problem ?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/zalando/problem-spring-web/issues/518#issuecomment-690284621, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADI7HP6PE6OL2TDP4IIYUDSFDHG3ANCNFSM4REVQB6Q .

whiskeysierra commented 4 years ago

What do you say? Would that be acceptable?

Opa- commented 4 years ago

I understand you want to keep this library within a certain scope but on the other hand I do think there's some reasons to add these features to this library.

Regarding constraint validation errors:

Regarding localization

Also by adding such features, I don't think we'd need to introduce new dependencies (i.e. MessageSource is part of Spring-Boot-Web).

I'd say that it all depends on what scope you want this library to cover and that it is fully understandable that you do not want it to cover more than the actual scope 😉

Opa- commented 4 years ago

Hi @whiskeysierra , do you have any thoughts about my previous message ?

whiskeysierra commented 4 years ago 
* The [RFC 7807](https://tools.ietf.org/html/rfc7807) (on page 3) does allow it and describes something very similar the existing `ConstraintViolationProblem` or to the JSON structure I mentioned above

Similar, but not the same. It's totally valid to have an extension to the library that provides what you need.

* The validation is fully handled by Jackson & Spring so I don't really get what level of complexity this would bring to the library

Validation yes, but localization isn't, at least not fully. We'd need to perform that manually for each problem.

* The [RFC 7807](https://tools.ietf.org/html/rfc7807) (on page 4) mention that the `title` can change for localization purposes

Which is valid for user-facing problems, but all of the builtin problems of this library are meant for a) developers and b) API consumers (machines). End-user specific problems require a lot more thought, hence why this library gives you all the tools and infrastructure that you need to built that on top.

Opa- commented 3 years ago

Which is valid for user-facing problems, but all of the builtin problems of this library are meant for a) developers and b) API consumers (machines). End-user specific problems require a lot more thought, hence why this library gives you all the tools and infrastructure that you need to built that on top.

Ok that does make sense to keep this library dev/machines oriented. I'll let you know if I setup a library based on this one to tackle l10n & end-user validation 😉 Thanks for clarifying 👍