On Monday 27 February 2017 11:45:16 Konrad Windszus wrote: > > On 27 Feb 2017, at 11:28, Oliver Lietz <[email protected]> wrote: > > > > On Monday 27 February 2017 11:00:54 Konrad Windszus wrote: > >>> On 27 Feb 2017, at 10:54, Oliver Lietz <[email protected]> wrote: > >>> > >>> On Monday 27 February 2017 10:30:32 Konrad Windszus wrote: > >>>>> On 27 Feb 2017, at 10:28, Oliver Lietz <[email protected]> wrote: > >>>>> > >>>>> On Monday 27 February 2017 08:00:14 Konrad Windszus wrote: > >>>>>> Hey, I don't really think the change for that in > >>>>>> https://svn.apache.org/viewvc/sling/trunk/bundles/extensions/validati > >>>>>> on > >>>>>> /a > >>>>>> pi > >>>>>> /src/main/java/org/apache/sling/validation/spi/DefaultValidationFailu > >>>>>> re > >>>>>> .j > >>>>>> ava ?r1=1734530&r2=1784472&pathrev=1784472 was good. The > >>>>>> resourceBundle > >>>>>> parameter is marked as @Nonnull. If you give a null here the return > >>>>>> value is useless (because the key cannot be resolved against the > >>>>>> MessageBundle). Your change makes it harder to detect such > >>>>>> programming > >>>>>> errors during development, because you no longer throw a (noisy) > >>>>>> exception, but rather fall back to a IMHO useless default (empty > >>>>>> string) > >>>>>> which is rather unexpected. > >>>>>> > >>>>>> What is the reason for that change? > >>>>> > >>>>> Hi Konrad, > >>>>> > >>>>> checking for null allows validation even if resource bundle is > >>>>> missing. > >>>>> I don't think validation should stop working just because human > >>>>> readable > >>>>> message is missing. > >>>> > >>>> Yes I agree, but then your code should not call that specific method. > >>> > >>> Do you mean validation should stop working when messages are not > >>> present? > >>> > >>>> Where exactly in your code is it called with ResourceBundle = null? > >>> > >>> It's in ValidationPostResponseCreator. > >> > >> This is test code only. If this cannot rely on a real ResourceBundle > >> (which > >> previous to your move to PaxExam was the case), then we should rather > >> modify the ValidationPostResponseCreator to deal with that. But I would > >> really like to validate in the IT that the right english translations are > >> provided (therefore PaxExam should provide the slingi18n bundle and > >> therefore also the right resource bundle). > > > > The faster Pax Exam-based test discloses a situation which can also happen > > in production and Validation should handle it gracefully. We can log a > > message (warn) in case resource bundle is missing of course. > > The tests itself are not modified at all and still check the validation > > message. > > The ValidationPostResponseCreator is test code only and does not properly > protect against null values in the resource bundle (although it would be > its obligation). That is the bug which needs to be fixed. For every other > client using the ValidationFailure.getMessage(null) should also lead to an > exception, because that is definitely an invalid (i.e. not-supported) use > case. In the ValidationPostResponseCreator we should just throw an > exception if the resource bundle can not be retrieved. > > If the tests now always succeed this means that the resource bundle is > actually never null, because in case it would be null, the validation > failure messages would not be as expected.
As I said before, a resource bundle could be null in production also and we should handle it gracefully. Similar situation happens when just a message is missing (the message key is returned – which I don't like because it discloses internal information): we do not throw an exception and validation still works. * a validation failure can be useful even without human readable message * no programming errors are hidden by this change * if validation result is valid, resource bundle is not used at all There is no reason to break validation on missing resource bundle. O. > Do you want me to put the > correct null handling in place for the ValidationPostResponseCreator or do > you take care of that? Thanks, > Konrad > > > O. > > > > [...]
