Review request for 7192302 Remove JDBCRowSetImpl dependency on java.beans (original) (raw)

Lance Andersen - Oracle Lance.Andersen at oracle.com
Thu Sep 6 13:45:38 UTC 2012

On Sep 6, 2012, at 9:31 AM, Alan Bateman wrote:

On 06/09/2012 14:09, Lance Andersen - Oracle wrote:

:

The latest webrev looks okay except that in one of the constructors you have removed a call to ensure that the connection is established, I'm not sure about the significance of that. This is not needed here and given I have already tested with this removed, I figured I would OK to keep this as part of the change Okay, I'll have to trust you on this one but I am a little bit concerned that it could cause a NPE, say someone creates a JdbcRowSet and invokes a method such as comment, rollback or getAutoCommit without doing an explicit connect.

connect() is typically called to get a connection and it will validate that there is a non-null Connection. I have unit tests which leverage that constructor and run clean with that change.

I agree that commit(), rollback, etc should call checkState() and that is another fix I need to do but the potential for the NPE is there depending on which constructor was used and if you did something silly like making a rollback without doing any work.

I can revert that change if you like but I have not seen any failures under what I would term normal use.

I left those in as a reminder to go back as part of the rest of the Rave clean-up. I would prefer to leave them for now and when I make another pass for Rave, I will get rid of them Okay.

One method that looks like it could be removed too is setConcurrency but I agree that keeping focused on just removing the beans dependency is right for now. I had thought about that but I have to think about this one a bit more as the getConcurrency() is leveraging the value returned from the active ResultSet which is why I did not remove this at this time. Okay although it looks to be just an optimization introduced as part of adding listener events

I am not sure I agree with the code either, just want to spend a bit more time looking at the code flow and see what tests if any we have here

I'm fine with leaving it as it is.

thank you

-Alan

Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen at oracle.com

More information about the core-libs-dev mailing list