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.