Re: [jira] [Resolved] (SLING-6569) NPE in DefaultValidationFailure when resource bundle is null

[Oliver Lietz <ap...@oliverlietz.de>]

On Tuesday 28 February 2017 08:37:53 Konrad Windszus wrote:
> > On 28 Feb 2017, at 00:29, Oliver Lietz <ap...@oliverlietz.de> wrote:
> > 
> > On Monday 27 February 2017 11:45:16 Konrad Windszus wrote:
> >>> On 27 Feb 2017, at 11:28, Oliver Lietz <ap...@oliverlietz.de> wrote:
> >>> 
> >>> On Monday 27 February 2017 11:00:54 Konrad Windszus wrote:
> >>>>> On 27 Feb 2017, at 10:54, Oliver Lietz <ap...@oliverlietz.de> wrote:
> >>>>> 
> >>>>> On Monday 27 February 2017 10:30:32 Konrad Windszus wrote:
> >>>>>>> On 27 Feb 2017, at 10:28, Oliver Lietz <ap...@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/valida
> >>>>>>>> ti
> >>>>>>>> on
> >>>>>>>> /a
> >>>>>>>> pi
> >>>>>>>> /src/main/java/org/apache/sling/validation/spi/DefaultValidationFai
> >>>>>>>> lu
> >>>>>>>> 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.
> 
> Disagree here, no code is forced to call
> ValidationFailure.getMessage(ResourceBundle). Therefore the caller needs to
> handle it gracefully in case the resource bundle is null. The JSR305
> annotation clearly states that the parameter must not be null.
> > Similar situation happens when just a message is
> > missing (the message key is returned – which I don't like because it
> > discloses
> > internal information):
> right, but I don't think these two are comparable
> 
> > we do not throw an exception and validation still
> > works.
> 
> Yes, validation works. It is just that you should not call
> ValidationFailure.getMessage(ResourceBundle) in case you don't have a
> resource bundle. We could think about adding another method which will
> expose the message keys and arguments.
> > * a validation failure can be useful even without human readable message
> 
> Agree and as I said, this works even without handling null gracefully.
> 
> > * no programming errors are hidden by this change
> 
> Don't agree, because your change makes the method return the empty string
> which might hide programming errors like an uninitialized variable of a
> resource bundle variable
> > * if validation result is valid, resource bundle is not used at all
> 
> Yes, and even if it is actually invalid, that does not necessarily lead to
> the usage of this method. Only in our ITs this is the case.
> > There is no reason to break validation on missing resource bundle.
> 
> We never did. It is just that you should not call
> ValidationFailure.getMessage(ResourceBundle) when you don't have the
> resource bunde.

Above validation was meant as the complete process of validation including 
preparation of the response with messages.

Those JSR305 annotations may help API users to write better code, but do not 
make the implementation more robust. I would have preferred a more resilient 
design. So I change ValidationFailure.getMessage(ResourceBundle) to throw a 
meaningful NPE at least when ResourceBundle is null.

O.

> > 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.
> >>> 
> >>> [...]