You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ant.apache.org by Jaikiran Pai <ja...@gmail.com> on 2018/02/09 09:06:37 UTC
Mapped resources NPEs - Potential fix causes a failure in one
specific test
Recently a couple of bugs have been reported around mapped resources
where we end up in NullPointerExceptions in various places[1][2]. As
noted in [1], it needs a fix at a central place and I think I was able
to fix the root cause as part of this commit[3] that I did an hour or so
back. However, there's a specific test case[4] in our antunit test suite
which expects a BuildException to be thrown as part of copy task. That
test was introduced long back as part of [5] to test another similar
NPE. I think with the change I now have, it no longer reaches this place
in the code where the Copy task would end up getting a null from a
mapper, because various other utility classes already handle this
contract and skip that resource. As such, the test completes cleanly
without any NPE and without copying anything, however given that we do
expect a build failure exception to be thrown, this test is now failing [6].
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)?
[1] https://bz.apache.org/bugzilla/show_bug.cgi?id=62076
[2] https://bz.apache.org/bugzilla/show_bug.cgi?id=62086
[3]
https://github.com/apache/ant/commit/e3f5250916dc0d9493b45b2d8fc6efe3a0fd9fda#diff-fabf453d75aaafb0dd9080e12d5dddfcR51
[4]
https://github.com/apache/ant/blob/master/src/tests/antunit/taskdefs/copy-test.xml#L85
[5] https://bz.apache.org/bugzilla/show_bug.cgi?id=39960
[6]
https://builds.apache.org/job/Ant-Build-Matrix-1.9.x-Linux/OS=xenial,jdk=JDK%201.5%20(latest)/lastCompletedBuild/testReport/src.tests.antunit.taskdefs/copy-test_xml/testResourceWithoutName/
-Jaikiran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: Mapped resources NPEs - Potential fix causes a failure in one
specific test
Posted by Jaikiran Pai <ja...@gmail.com>.
Thanks everyone for the inputs. Based on those, I have now updated the
test to not expect a build failure exception and also have updated our
release notes to mention this change in behaviour.
-Jaikiran
On 09/02/18 11:00 PM, Stefan Bodewig wrote:
> Jaikiran,
>
> just to avoid duplicate work, I'm currently running tests on the 1.9.x
> branch where I fix a bunch a FileNameMapper implementations that
> wouldn't handle null sourceFileNames (which could happen for resources
> that retun null for getName) as well as several other places that didn't
> check the return value of mapFileNames properly.
>
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
> For additional commands, e-mail: dev-help@ant.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: Mapped resources NPEs - Potential fix causes a failure in one
specific test
Posted by Jaikiran Pai <ja...@gmail.com>.
Sure Stefan.
-Jaikiran
On 09/02/18 11:00 PM, Stefan Bodewig wrote:
> Jaikiran,
>
> just to avoid duplicate work, I'm currently running tests on the 1.9.x
> branch where I fix a bunch a FileNameMapper implementations that
> wouldn't handle null sourceFileNames (which could happen for resources
> that retun null for getName) as well as several other places that didn't
> check the return value of mapFileNames properly.
>
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
> For additional commands, e-mail: dev-help@ant.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: Mapped resources NPEs - Potential fix causes a failure in one specific test
Posted by Stefan Bodewig <bo...@apache.org>.
Jaikiran,
just to avoid duplicate work, I'm currently running tests on the 1.9.x
branch where I fix a bunch a FileNameMapper implementations that
wouldn't handle null sourceFileNames (which could happen for resources
that retun null for getName) as well as several other places that didn't
check the return value of mapFileNames properly.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: Mapped resources NPEs - Potential fix causes a failure in one
specific test
Posted by Dominique Devienne <dd...@gmail.com>.
On Fri, Feb 9, 2018 at 11:05 AM, Jaikiran Pai <ja...@gmail.com>
wrote:
>
> On 09/02/18 2:47 PM, Dominique Devienne wrote:
>
>> On Fri, Feb 9, 2018 at 10:06 AM, Jaikiran Pai <ja...@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
Re: Mapped resources NPEs - Potential fix causes a failure in one
specific test
Posted by Martin Gainty <mg...@hotmail.com>.
How many times have we seen [] implementation throw StackOverflow?
this is because you are inserting elements into an already overloaded stack
what happens if your stack cannot accomodate the extra element..StackOverflow
refactoring to an implementation of Collection:
ArrayList<T> list=null;
//now i can test the list for null
if(list == null)
and if you absolutely positively need an array
T []someArray = list.toArray()
+1 for Jaikiran patch
Martin
______________________________________________
________________________________
From: Jaikiran Pai <ja...@gmail.com>
Sent: Friday, February 9, 2018 5:05 AM
To: dev@ant.apache.org
Subject: Re: Mapped resources NPEs - Potential fix causes a failure in one specific test
On 09/02/18 2:47 PM, Dominique Devienne wrote:
> On Fri, Feb 9, 2018 at 10:06 AM, Jaikiran Pai <ja...@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.
-Jaikiran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: Mapped resources NPEs - Potential fix causes a failure in one
specific test
Posted by Jaikiran Pai <ja...@gmail.com>.
On 09/02/18 2:47 PM, Dominique Devienne wrote:
> On Fri, Feb 9, 2018 at 10:06 AM, Jaikiran Pai <ja...@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.
-Jaikiran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: Mapped resources NPEs - Potential fix causes a failure in one
specific test
Posted by Dominique Devienne <dd...@gmail.com>.
On Fri, Feb 9, 2018 at 10:06 AM, Jaikiran Pai <ja...@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.
If OTOH it was another IO-related exception, it might still be fine, but as
long as there's a visible trace of the IO exception, like an error message
(in normal or at least verbose mode).
So except in the latter case, and there's no message at any log level, I'm
fine with what you propose to no longer throw. My $0.02, based on your
message only. --DD
Re: Mapped resources NPEs - Potential fix causes a failure in one
specific test
Posted by Gintautas Grigelionis <g....@gmail.com>.
+1
Gintas
2018-02-09 21:16 GMT+01:00 Jan Matèrne (jhm) <ap...@materne.de>:
> > What this specific test failure shows, though, is that we now no longer
> > are able to distinguish between a resource that we could copy but can't
> > because it doesn't provide a name and the case of "all resources have
> > been stripped out by mappers as "I don't know how to handle it".
>
> * @return an array of strings if the rule applies to the source file,
> or
> * null if it does not.
> */
> String[] mapFileName(String sourceFileName);
>
> That means for me:
> - can't handle: return null
> - no result: return new String[]{}
>
>
> Jan
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
> For additional commands, e-mail: dev-help@ant.apache.org
>
>
AW: Mapped resources NPEs - Potential fix causes a failure in one specific test
Posted by "Jan Matèrne (jhm)" <ap...@materne.de>.
> What this specific test failure shows, though, is that we now no longer
> are able to distinguish between a resource that we could copy but can't
> because it doesn't provide a name and the case of "all resources have
> been stripped out by mappers as "I don't know how to handle it".
* @return an array of strings if the rule applies to the source file,
or
* null if it does not.
*/
String[] mapFileName(String sourceFileName);
That means for me:
- can't handle: return null
- no result: return new String[]{}
Jan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: Mapped resources NPEs - Potential fix causes a failure in one specific test
Posted by Stefan Bodewig <bo...@apache.org>.
On 2018-02-09, Jaikiran Pai wrote:
> That test was introduced long back as part of [5] to test another
> similar NPE. I think with the change I now have, it no longer reaches
> this place in the code where the Copy task would end up getting a null
> from a mapper, because various other utility classes already handle
> this contract and skip that resource.
I completely agree with your fix.
What this specific test failure shows, though, is that we now no longer
are able to distinguish between a resource that we could copy but can't
because it doesn't provide a name and the case of "all resources have
been stripped out by mappers as "I don't know how to handle it".
Unfortunately I don't see any way to distinguish between "all resources
have been excluded by mappers" and "all resources have been up to date"
and certainly not how to figure out a resource didn't have a name when
we want to allow such resources if followed by a merge mapper.
I'm fine with removing the test but would want us to document the change
in behavior. Where a resource with a null name used to cause a build
exception it will now be silently ignored by copy.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org