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.ander...@oracle.com
