Re: Mapped resources NPEs - Potential fix causes a failure in one specific test
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-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
Re: Mapped resources NPEs - Potential fix causes a failure in one specific test
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-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
Re: Mapped resources NPEs - Potential fix causes a failure in one specific test
+1
Gintas
2018-02-09 21:16 GMT+01:00 Jan Matèrne (jhm) :
> > 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-unsubscr...@ant.apache.org
> For additional commands, e-mail: dev-h...@ant.apache.org
>
>
AW: Mapped resources NPEs - Potential fix causes a failure in one specific test
> 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-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org
Re: Mapped resources NPEs - Potential fix causes a failure in one specific test
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-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
Re: Mapped resources NPEs - Potential fix causes a failure in one specific test
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-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
Re: Mapped resources NPEs - Potential fix causes a failure in one specific test
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 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 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 > 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. * * 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-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
Re: Mapped resources NPEs - Potential fix causes a failure in one specific test
On Fri, Feb 9, 2018 at 11:05 AM, Jaikiran Pai wrote: > > On 09/02/18 2:47 PM, Dominique Devienne wrote: > >> On Fri, Feb 9, 2018 at 10:06 AM, Jaikiran Pai >> 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. > * > * 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
On 09/02/18 2:47 PM, Dominique Devienne wrote: On Fri, Feb 9, 2018 at 10:06 AM, Jaikiran Pai 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. * * 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-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
Re: Mapped resources NPEs - Potential fix causes a failure in one specific test
On Fri, Feb 9, 2018 at 10:06 AM, Jaikiran Pai 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
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-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
