sling-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Oliver Lietz <apa...@oliverlietz.de>
Subject Re: [jira] [Resolved] (SLING-6569) NPE in DefaultValidationFailure when resource bundle is null
Date Mon, 27 Feb 2017 23:29:38 GMT
On Monday 27 February 2017 11:45:16 Konrad Windszus wrote:
> > On 27 Feb 2017, at 11:28, Oliver Lietz <apache@oliverlietz.de> wrote:
> > 
> > On Monday 27 February 2017 11:00:54 Konrad Windszus wrote:
> >>> On 27 Feb 2017, at 10:54, Oliver Lietz <apache@oliverlietz.de> wrote:
> >>> 
> >>> On Monday 27 February 2017 10:30:32 Konrad Windszus wrote:
> >>>>> On 27 Feb 2017, at 10:28, Oliver Lietz <apache@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/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.
> > 
> > [...]



Mime
View raw message