[#BATCH-589] StepExecutionResourceProxy's toString() leads to NPE

Spring Batch

StepExecutionResourceProxy's toString() leads to NPE

Created: 15/Apr/08 03:32 PM Updated: 07/Aug/08 10:07 AM

Component/s:

Core

Affects Version/s:

1.0.1

Fix Version/s:

1.0.1

Time Tracking:

Original Estimate:

0.02d

Remaining Estimate:

0.02d

Time Spent:

Not Specified

Description

« Hide

Since it is possible to call toString before the delegate is set, toString will throw a NPE.

For example, when setResource() is invoked on FlatFileItemReader
public void setResource(Resource resource) throws IOException {
this.resource = resource;
path = resource.toString();
if (path.length() > 50) {
path = path.substring(0, 20) + "..." + path.substring(path.length());
}
}

you end up with a NPE when you try to get the path.

All

Comments

Work Log

Change History

FishEye

Builds

Sort Order:

[ Permlink | « Hide ]

Robert Kasanicky added a comment - 16/Apr/08 03:13 AM

Added assert in the toString method to get informative exception instead of NPE and removed the toString usage from FlatFileItemWriter#setResource (assuming the resource path is known at the time when resource is set just doesn't work).

[ Permlink | « Hide ]

Gregory Kick added a comment - 16/Apr/08 07:19 AM

I strongly disagree with this solution. It is nearly impossible to determine if and when toString is called. For example, somebody that uses AOP logging would be at a very high risk of accidently failing with exception.

I would like to see this issue reopened and toString implemented in such a way as to not throw an exception. E.g.

return (delegate == null) ? "(" + filePattern + ")" : delegate.toString();

IMHO, trading one exception for another is the least effective resolution.

[ Permlink | « Hide ]

Robert Kasanicky added a comment - 16/Apr/08 07:33 AM

All the methods in StepExecutionProxyResource throw exception if the resource is used before delegate is set, the reasoning I see is that you are in trouble if you are trying to use a proxy that proxies nothing.

However on a second look, I agree toString shouldn't really be throwing exceptions - reopening.

[ Permlink | « Hide ]

Gregory Kick added a comment - 16/Apr/08 07:49 AM

Yes. I think the distinction is that all methods defined in the Resource interface should throw exception. Since toString is defined by Object, I wouldn't put it under that umbrella.

[ Permlink | « Hide ]

Robert Kasanicky added a comment - 16/Apr/08 08:14 AM

toString now returns filePattern as suggested when delegate is null.

[ Permlink | « Hide ]

Gregory Kick added a comment - 16/Apr/08 10:10 AM

it doesn't look like the FlatFileItemReader got changed back. Can we put that back? it was very useful.

[ Permlink | « Hide ]

Robert Kasanicky added a comment - 16/Apr/08 10:32 AM

I think how FlatFileItemReader used toString was a bug - when used with StepExecutionProxyResource you'd get the file pattern instead of the delegate.toString (you don't want that right?). The string was used only for putting together an exception message when line couldn't be parsed - resource.getDescription seems to be meant for such circumstances.

[ Permlink | « Hide ]

Gregory Kick added a comment - 16/Apr/08 10:42 AM

Ah, you're right. I just looked at the diff and the change you made makes perfect sense. Thanks for working on this Robert.

[ Permlink | « Hide ]

Dave Syer added a comment - 07/Aug/08 10:07 AM

Assume closed as resolved and released