> On 27 Feb 2017, at 11:28, Oliver Lietz <apa...@oliverlietz.de> wrote: > > On Monday 27 February 2017 11:00:54 Konrad Windszus wrote: >>> On 27 Feb 2017, at 10:54, Oliver Lietz <apa...@oliverlietz.de> wrote: >>> >>> On Monday 27 February 2017 10:30:32 Konrad Windszus wrote: >>>>> On 27 Feb 2017, at 10:28, Oliver Lietz <apa...@oliverlietz.de> 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/validation >>>>>> /a >>>>>> pi >>>>>> /src/main/java/org/apache/sling/validation/spi/DefaultValidationFailure >>>>>> .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. 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. > > [...] >
