ant-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dominique Devienne <ddevie...@gmail.com>
Subject Re: Mapped resources NPEs - Potential fix causes a failure in one specific test
Date Fri, 09 Feb 2018 10:33:07 GMT
On Fri, Feb 9, 2018 at 11:05 AM, Jaikiran Pai <jai.forums2013@gmail.com>
wrote:

>
> On 09/02/18 2:47 PM, Dominique Devienne wrote:
>
>> On Fri, Feb 9, 2018 at 10:06 AM, Jaikiran Pai <jai.forums2013@gmail.com>
>> wrote:
>>
>> I need some inputs on how we should go about this specific change/test?
>>> Should this test continue to expect a exception or is it fine to expect
>>> that target to complete cleanly (without copying anything)?
>>>
>>> What's the source of the NPE?
>> If it's an implementation detail, then it's fine to no longer throw IMHO.
>>
> We have FileNameMapper interface which has a:
>
> String[] mapFileName(String sourceFileName);
>
> API. The contract/javadoc of this API says:
>
>      * Returns an array containing the target filename(s) for the
>      * given source file.
>      *
>      * <p>if the given rule doesn't apply to the source file,
>      * implementation must return null....
>
> We have an implementation of this interface, the IdentityMapper, whose
> implementation so far, IMO, wasn't following this contract. If it was
> passed a null source file name, that implementation would return an
> String[] with one element and the contained element in that array would be
> null.
>
> Callers of this API are expected to understand that the API itself might
> return null, so they used to just check the return value and not its
> contents. They would then go and start working on these individual elements
> in the array and start running into NPEs in a bunch of places.
>
> The change I made was to the implementation of IdentityMapper, so that it
> returns null instead of an array with one null element, when the input to
> it is null. That way, the callers' logic of checking the return value for
> null, is enough for them to skip such resources.
>
> So to me, this does look like something that we should take care off in
> that specific test, so that it no longer expects a build exception to be
> thrown.


Agreed. +1. Thanks for the details and "walking me through the code". --DD

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message