This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
Summary: | Databinding error due to incorrect logic in CachedRowsetDataprovider to check for cachedrowset execution | ||
---|---|---|---|
Product: | obsolete | Reporter: | _ krystyna <krystyna> |
Component: | visualweb | Assignee: | John Baker <jbaker> |
Status: | VERIFIED FIXED | ||
Severity: | blocker | CC: | alexpetrov, blaha, jayashri, jbaker, lfitzgerald, wjprakash |
Priority: | P2 | ||
Version: | 6.x | ||
Hardware: | Other | ||
OS: | All | ||
Issue Type: | DEFECT | Exception Reporter: | |
Attachments: |
First project
First project Second Project Diff that uses a member variable |
Description
_ krystyna
2007-10-19 23:11:48 UTC
Created attachment 51344 [details]
First project
Created attachment 51345 [details]
First project
Created attachment 51346 [details]
Second Project
When this fails for me, the sql statement is: customerRowSet.setCommand("SELECT * FROM APP.CUSTOMER"); When it works, the sql statement selects individual columns. Ah, I see what Lark is seeing-- look at attached Second Project session bean _init: personRowSet.setDataSourceName("java:comp/env/jdbc/TRAVEL_ApacheDerby_1"); personRowSet.setCommand("SELECT * FROM TRAVEL.PERSON"); personRowSet.setTableName("PERSON"); personRowSet1.setDataSourceName("java:comp/env/jdbc/TRAVEL_ApacheDerby"); personRowSet1.setCommand("SELECT ALL TRAVEL.PERSON.PERSONID, \n TRAVEL.PERSON.NAME, \n TRAVEL.PERSON.JOBTITLE, \n TRAVEL.PERSON.FREQUENTFLYER, \n TRAVEL.PERSON.LASTUPDATED \nFROM TRAVEL.PERSON"); personRowSet1.setTableName("PERSON"); } Actually, for my project , personRowset1 that fails in the second project with a "Select ALL" but it has the funky \n's. After a little bit of debug, this problem seems to be caused by David VC checkin 1.2 2007-09-19 21:54:00 +0000 davidvc Issue 106854: Rowset passes the TCK on JDK 6. We will consider this an extension of the JDK 6 version of the RI, which also happens to run on Java 5 and Java 1.4, as long as introspection is not used. Looks like the bug was not discovered for about a month Here is my analysis During execution if a component which was bound to any component that does not request the cachedRowsetDataprovider.rowCount (so cachedrowset is executed), but directly asks for value from first row of database then this problem could occur. (There was an Array out of bound exception thrown, but it was eaten up somewhere) If a component such as table, list or dropdown happens to execute the cacheRowsetDataprovider.rowCount first, which in turn executes the cachedrowset correctly. In this case every thing works good. Now here the problem, when the component that directly asks for the cacheRowsetDataprovider.getValue(), depends on checkExecuted() method to execute the cachedRowset before fetching the data This how checkExecuted() method works in a bizzare way try{ getCachedRowSet().isBeforeFirst() } catch (SQLException e) { getCachedRowSet().execute(); getCachedRowSet().first(); } If an exception is thrown from CachedRowset while it asks for cachedRowset.isBeforeFirst() , then it executes the CachedRowset. Unfortnately, after David VC checkin on 2007-09-19, the exception is not thrown. Here is Davids chages // If the row set metadata has been set, even though we never // executed a command, then this is OK too. This is a cached // row set and can be initialized without executing a command. if (!isExecuted() && this.rowSetMD == null) { throw new SQLException(rb.getString("EXECUTE_NEVER_CALLED")); //NOI18N } So the cachedrowset is never executed and no data is obtained. Since the information is cached after that, none of the components after that, will get any data from the same cachedrowset. Her is a fix I have in the cacheRowsetDataprovider. But I'm not sure if this is a correct logic, but seems to work. I'll leave it up to David VC to decide on this Index: src/com/sun/data/provider/impl/CachedRowSetDataProvider.java =================================================================== RCS file: /cvs/visualweb/dataprovider/runtime/library/src/com/sun/data/provider/impl/CachedRowSetDataProvider.java,v retrieving revision 1.6 diff -u -r1.6 CachedRowSetDataProvider.java --- src/com/sun/data/provider/impl/CachedRowSetDataProvider.java 2 Oct 2007 07:16:03 -0000 1.6 +++ src/com/sun/data/provider/impl/CachedRowSetDataProvider.java 20 Oct 2007 01:58:51 -0000 @@ -1320,7 +1320,10 @@ protected void checkExecute() throws SQLException { if (!Beans.isDesignTime() && getCachedRowSet() != null) { try { - getCachedRowSet().isBeforeFirst(); + if (!getCachedRowSet().isBeforeFirst()) { + getCachedRowSet().execute(); + getCachedRowSet().first(); + } } catch (SQLException e) { getCachedRowSet().execute(); getCachedRowSet().first(); Yes, that is a bizarre way that it checked to see if it was executed. I also believe that your approach is a little odd because it *depends* upon an exception being thrown. Note that in the RI, as far as I can tell from inspecting the code, beforeFirst() will *not* throw an exception even if the rowset has not been executed. Actually, a CachedRowSet should be completely valid just from being populated manually without executing a command. I'd like to understand *why* we're even calling checkExecute() - what in our logic makes us "lazily" execute like this - should we explicitly execute()? And if we haven't executed, then there are no rows. Anyway, there is a better solution, *if* you assme the CRS is our implementation. There is a method isExecuted(). CachedRowSet rs = getCachedRowSet(); boolean executed; if ( rs instanceof CachedRowSetXImpl ) { executed = ((CachedRowSetXImpl)rs).isExecuted(); } else if ( rs.size() == 0 ) // Assume this row set should be executed executed = false; } catch ( Exception e ) { executed = false; } } if ( ! executed ) { rs.execute(); rs.first(); } In this way, in most cases we'll use a sane approach, and use a less sane approach only if the rowset is not a CachedRowSetXImpl (which as far as I know never happens, but yo unever know). David, checking for exception was the original code which failed because of your change (See the diff I attached originally). I changed it to check if if(!getCachedRowSet().isBeforeFirst()) { getCachedRowSet().execute(); getCachedRowSet().first(); } which worked. May be your solution is better to check ((CachedRowSetXImpl)rs).isExecuted(); I tested the a version of the proposed change in desc9 but execution fails. I'll do some debugging Here's the diff of the changes I tried @@ -1318,14 +1314,27 @@ * <p>Check if rowset, if so, execute if necessary.</p> */ protected void checkExecute() throws SQLException { + boolean executed = false; + CachedRowSet rs = null; if (!Beans.isDesignTime() && getCachedRowSet() != null) { try { - getCachedRowSet().isBeforeFirst(); - } catch (SQLException e) { - getCachedRowSet().execute(); - getCachedRowSet().first(); + rs = getCachedRowSet(); + + if (rs instanceof CachedRowSetXImpl) { + executed = ((CachedRowSetXImpl) rs).isExecuted(); + } else if (rs.size() == 0) // Assume this row set should be executed + { + executed = false; + } + } catch (Exception e) { + executed = false; } - } + + if (!executed) { + rs.execute(); + rs.first(); + } + } } /** We agreed that this was P1 however my update yesterday did not take. John, in your change again you are depending on exception to be thrown from getCachedRowSet().isBeforeFirst(). The bug occurs because it never throws exception after Davids change. Probably you should try something like protected void checkExecute() throws SQLException { boolean executed = false; CachedRowSet rs = null; if (!Beans.isDesignTime() && getCachedRowSet() != null) { rs = getCachedRowSet(); if (rs instanceof CachedRowSetXImpl) { executed = ((CachedRowSetXImpl) rs).isExecuted(); } else if (rs.size() == 0) // Assume this row set should be executed { executed = false; } } catch (Exception e) { executed = false; } if (!executed) { rs.execute(); rs.first(); } } thanks Winston. After making changes, runtime is working for CachedRowSet bound to StaticText. Also, I tested the same application with text field and Table using the same dataprovider and also using a second dataprovider. With these changes also I didn't see any exceptions in the server log file. I'll send a jar for testing Here's the new diff: Index: src/com/sun/data/provider/impl/CachedRowSetDataProvider.java =================================================================== RCS file: /cvs/visualweb/dataprovider/runtime/library/src/com/sun/data/provider/impl/CachedRowSetDataProvider.java,v retrieving revision 1.6 diff -u -r1.6 CachedRowSetDataProvider.java --- src/com/sun/data/provider/impl/CachedRowSetDataProvider.java 2 Oct 2007 07:16:03 -0000 1.6 +++ src/com/sun/data/provider/impl/CachedRowSetDataProvider.java 21 Oct 2007 00:12:03 -0000 @@ -58,7 +58,6 @@ import java.sql.Types; import java.text.MessageFormat; import java.util.ArrayList; -import java.util.Arrays; import java.util.Iterator; import java.util.List; import java.util.Locale; @@ -78,18 +77,15 @@ import javax.sql.rowset.spi.SyncProviderException; import com.sun.data.provider.DataProviderException; import com.sun.data.provider.FieldKey; -import com.sun.data.provider.FilterCriteria; import com.sun.data.provider.RowKey; import com.sun.data.provider.RefreshableDataProvider; import com.sun.data.provider.RefreshableDataListener; -import com.sun.data.provider.SortCriteria; import com.sun.data.provider.TableCursorVetoException; -import com.sun.data.provider.TableDataFilter; import com.sun.data.provider.TableDataProvider; -import com.sun.data.provider.TableDataSorter; import com.sun.data.provider.TransactionalDataListener; import com.sun.data.provider.TransactionalDataProvider; import com.sun.sql.rowset.CachedRowSetX; +import com.sun.sql.rowset.CachedRowSetXImpl; import com.sun.sql.rowset.SyncResolverX; /** @@ -1318,15 +1314,31 @@ * <p>Check if rowset, if so, execute if necessary.</p> */ protected void checkExecute() throws SQLException { - if (!Beans.isDesignTime() && getCachedRowSet() != null) { - try { - getCachedRowSet().isBeforeFirst(); - } catch (SQLException e) { - getCachedRowSet().execute(); - getCachedRowSet().first(); + boolean executed = false; + + try { + CachedRowSet rs = getCachedRowSet(); + if (!Beans.isDesignTime() && rs != null) { + + if (rs instanceof CachedRowSetXImpl) { + executed = ((CachedRowSetXImpl) rs).isExecuted(); + } else if (rs.size() == 0) // Assume this row set should be executed + { + executed = false; + } + + + if (!executed) { + rs.execute(); + rs.first(); + } } + + } catch (Exception e) { + executed = false; } } + This code seems to me the right approach. John, a small modification suggestion. The executed=false inside the catch block has no meaning. So you may want to log the exception, otherwise, the exception gets eaten up. Good catch, forgot to print the exceptions I sent a new jar to QA for testing Forgot to mention that after printing exceptions instead of swallowing them, no exceptions occurred while testing execution. btw, there are a few places in CachedRowSetDataProvider where exceptions are swallowed, so updates are needed also I've been offline and am trying to catch up. Do we actually have permission to check in a change at this point? What's the cutoff date? Regarding the change: I know I was the one who suggested this, but the more I think of it, the more I really don't like depending on a rowset with no rows to imply that the rowset was not executed. If the rowset really is empty, it will get executed over and over and over again. I'm attaching a diff that does it using a member variable that tracks whether we've executed, so we don't have to ask the rowset. This builds - I'll send out a jar file to John and QA for testing. Created attachment 51371 [details]
Diff that uses a member variable
Marking this bug beta2fix. This was deemed a p1 showstopper since simple databinding is broken. Check this beta2 fix process: http://wiki.netbeans.org/wiki/view/NB6Beta2Rules And I've tested this latest jar on beta2 build NetBeans IDE 6.0 Beta 2 (Build 200710211001) and it looks good. I'm not really comfortable with the latest changes by relying on a property. At runtime when using dataprovider APIs and changes to field keys or database columns change ( a column is added or removed ) I think using the property may be risky and prefer to make small changes. If others disagree then I think this dataprovider jar needs at least 2 days testing, including testing the database tutorials and Sample applications. Krys, can you please try the TwoPageCrud sample that is bundled How useful is use case when database table is binded to textfield? I guess in the most cases the table will be used. I'm downgrading to P2 since the binding whole DB table to textfield isn't common use case. I agree this should have more testing. To be honest, I think that we are *sorely* lacking in automated testing of data binding, but I guess we're just not resourced to create these tests. I don't actually understand what your concern is, John, about "using a property" and how this is related to adding and removing columns. I created a private member variable, not a property, and it is used to capture some internal state for the data provider. How is this related to properties and the changing of columns? David David, I mis-read/interpreted the change was to use a property, not a private member variable. My concern was that if there was some underlying programmatic change in the application code or even the database that would prevent the rowset from being re-executed (if "executed" were set to true when it should be false) I'll come up with some use cases tomorrow. If no automated testing, then at least we should have a test spec with use cases. The result of the original scenario "Try different order of operations..." depends on order of jsf-tags in Page1.jsp: a). <webuijsf:textField... <webuijsf:table... No data will be shown for JSF components. b). <webuijsf:table... <webuijsf:textField... Data will be shown for Table and Text Field. The bug is reproduced on: Product Version: NetBeans IDE 6.0 Beta 2 (Build 200710212201) Java: 1.6.0_03; Java HotSpot(TM) Client VM 1.6.0_03-b02 System: Windows XP version 5.1 running on x86 & Product Version: NetBeans IDE Dev (Build 200710220000) Java: 1.6.0_03; Java HotSpot(TM) Client VM 1.6.0_03-b02 System: Windows XP version 5.1 running on x86 I believe John's concern is - will the "executed" flag will be correctly reset to false, if some programmatic change happens to the underlying cachedRowset or some user action happens in the designer. For example, if in the Session Bean user re-sets the cahcedRowset "command" property, now the fetched infomation in the CachedRowset is invalid. Dataprovider might receive an event that the underlying CachedRowset has changed. In that case the "executed" flag should be reset to false, so that next request for info will fetch the info corresponding to the new "command". Another scenario is, user might refresh the designer which in turn calls the CachedRowsetDataprovider.refresh(). In this case also, I think, "executed" must be reset to false, so that fresh information will be fetched from database in the next request. These scenarios used to work earlier. Frankly, I don't know how earlier logic worked. OK, thanks Winston for your comments. John, I have to focus on planning this week, so if you are available, please feel free to take this issue. I think we should definitely test what happens when an important property change happens. I know that you can register a property change listener with CachedRowSetXImpl, but I'm not sure how it's currently being used. I noticed for instance, even before my change to the rowset, that if you remove a column from the query, the columns don't change in the table bound to the query unless you restart NB. I'm not sure if that's still happening. David From use cases, testing the latest proposed change Testing latest changes, the Sample application, SinglePageCrudTable is working, which is good news. Once this issue here is resolved then if time permits, it would be good to restore this sample CacheRowSetDataProvider is partially fixed Checking in visualweb/dataprovider/runtime/library/src/com/sun/data/provider/impl/CachedRowSetDataProvider.java; /cvs/visualweb/dataprovider/runtime/library/src/com/sun/data/provider/impl/CachedRowSetDataProvider.java,v <-- CachedRowSetDataProvider.java new revision: 1.7; previous revision: 1.6 done refresh() still needs fixing - covered by issue 119753 fix went into trunk for fcs, not beta2 v. |