RowSetMigrationReviewPage

Review page for Row Set Migration

This page is used to manage the review of the spec for row set migration.

The value of this approach is that it allows me to track all the issues separately and to drive them to resolution. For me email reviews although nice and informal are almost impossible to track to conclusion. Inline comments to the main document muddy up the document and are also difficult to track to conclusion.

The review process works like this:

  • Add a comment header of the format "Comment <initials>-<number> (<status>)" (see below for examples)
  • A discussion then can take place under that comment header
  • Each comment will have a status at the top, with the status set to Approved, Accepted PDU (pending doc update) or Unresolved
  • If necessary, we do more rounds of discussion until the item is marked as Approved

Commenter Initials Key

Contents


CRM-1 - Functional changes (Resolved)

Historically we've used implementation class for properties (rather than interface) to ensure that Insync can model the properties. Need to ensure that this works with interfaces.|Unresolved

Suggest talking this over with Sandip

DavidVanCouvering: Thanks, I didn't think of that. That seems like a real issue, and is in direct conflict with our desire to avoid the use of extensions. It seems odd to me that our model requires declaration of classes, when modularity and pluggability requires use of interfaces... I'll talk to Sandip.

DavidVanCouvering: Resolution: It needs to be a class. I updated the spec to address this.

CRM-2 - Functional Changes (Resolved)

Need to ensure that none of our generated code (or tutorial examples) use API calls not present in the standard CachedRowSet interface.

DavidVanCouvering: What generated code are you referring to? I already talk about the code we generate as part of a project. Are there pieces of code checked into NetBeans that are generated from VWP? I do mention that the tutorials will need to be updated in the "External Impact" secion.

DavidVanCouvering: Resoltion: I'll track down our generated code and examples and make sure they are spec-compliant. For starters, they can't be using CachedRowSetXImpl or CachedRowSetX any more...

PB-1 (Unresolved)

Changing the implementation to private CachedRowSet personRowSet = new com.sun.sql.rowset.CachedRowSetImpl(); means that we will still have to bundle the RI with every project. This does not solve one of the goals. We should change the implementation to private CachedRowSet personRowSet = new CachedRowSet(); for new projects. The user can change the code AND add the RI to project classpath if needed.

DavidVanCouvering: This is a problem with the RowSet spec. There is no factory pattern or other way to do dependency injection. You have to hardcode in a constructor for a specific implementation. CachedRowSet is an interface, not a class, so you can't construct with it.

Once the RI in the JRE contains all the extensions we require, we will no longer have to bundle it -- this is a short-term issue I think


PB2 (Unresolved)

NOTE that the type of the variable is not the implementation class but the interface. This is very important, as it by default requires any code that uses the rowset to stick with the standard interface methods. This is an important principle: we want our user code to be standards-compliant, and not make use of proprietary extensions, even if those extensions are part of the RI.

They can "opt in" to this by setting a property in the project properties window called "Allow Use of RowSet Extensions" When a checks this property, the declaration and getter/setter pair is changed to be of type CachedRowSetImpl

Project property seems too prominent place for this. Can we just assume that the user will either change the return type (manually, via refactoring)? Or even better, the user will cast the type to CachedRowSetImpl when needed - that way only code using the special features will depend on the implementation. When the user decides to use the RI API than RI should also be added to project libraries. Admittedly this would be possible to do if we expose the switch as a project property. I would be willing to compromize "ease of use" for this feature since we do not want users to use it too often.

DavidVanCouvering: I like the idea of just letting the user cast where they want to us the extensions, or otherwise manually fix their code. This makes it even more explicit that the user is opting in to use proprietary extensions. I'll go with that approach unless there are any objections.

DavidVanCouvering: Resolution: Modified the spec to let users opt in by modifying the code by hand. This saves a lot fo complexity and time.

PB3 (Unresolved)

Migration of existing projects

If the project makes use of extensions, then errors will be displayed when the project is compiled or when the user view the file that uses the extension. They can then either fix the code to not use the extension, or enable the use of extensions

Does the tool itself generate any code using these extensions? If so it would be better to wrap these calls into ((CachedRowSetImpl)getPersonRowset).foo() rather then producing code that does not compile. If this kind of refactoring turns out to be too complicated maybe we should just keep CachedRowSetImpl getPersonRowset() in existing projects. In case the user wrote the code using these extensions it may be OK to just display compilation errors, but I am sure. Maybe we should just keep the RI dependency in any migrated project to keep things simple and only remove it for new project.

DavidVanCouvering: No, as far as I know the tool does not generate any code using these extensions. I think the only exposure to the extensions is visually through the property editor and the navigator, which display extended properties and events.

Yes, keeping the RI dependency on any migrated project could simplify things, but I am concerned that then the user wouldn't know that they have painted themselves into a proprietary dependency corner. Compilation errors would make them aware of their situation...

DavidVanCouvering: Proposed Resolution: This section has been removed. Existing projects will continue to use the old CachedRowSetX for the existing code that was already generated. If new rowsets are added, they will use the standard CachedRowSet. We will not attempt to migrate existing projects to use the standard interfaces. If people want to, they can migrate by hand.

JWB-1 (Resolved)

When a user wants to use a proprietary rowset instead of the standard, I can see the rationale for changing at the project level (if there are many rowsets to change) but this introduces hard design changes for the Project properties dialog. We don't want to have to implement a VW-specific Project properties dialog and also, adding an entry for rowsets may not be appropriate if a project doesn't use any rowsets (unnecessary entry in the dialog).

An alternative to using the Project properties, there could be an action on a rowset bean in the Outline window that could allow a user to change the rowset extension. If there are many rowsets then this could be inconvenient but I think changing the extension from the individual rowset is more appropriate and specific to VW projects.

DavidVanCouvering - Resolution: See PB2, I think just requiring them to cast when they want to use extensions makes sense. Of course, this also hinges on whether we can even declare the instance variable as an interface because by doing so the project can't inspect its properties...

JWB-2 (Resolved)

Migrating projects that use the rave proprietary rowset to instead use the standard rowset is a nice feature for 6.0. However, I think this might be too big of a task to implement for the 6.0 timeframe. Instead we could provide a whitepaper explaining how to move to the standard rowset

DavidVanCouvering You may be right. But I'm thinking of the power user with 20 pages and 80 rowsets. Do we really want them to have to manually fix all their references to CachedRowSetXImpl? I would say we should have this as a task, but put it at the end of the list of things to do. Get it working first, provide automated migration second.

DavidVanCouvering Proposed resolution: don't force existing projects to use the standard, and continue to support CachedRowSetX for existing projects migrating to NB6. New projects will use the standard, however.

JD-1 (Resolved)

I assume that CachedRowSetImpl would be based on the version in the (updated) RI, modified as necessary?

DavidVanCouvering CachedRowSetImpl is the implementation class for CachedRowSet in the RI, so I think the answer to your question is "yes".

JD-2 (Resolved)

I think most of our proprietary code is in new methods in CachedRowSetX and the implementation. How many new methods are defined there? (Sorry, I haven't done a comparison.)

DavidVanCouvering This is something the RI team is looking at, and will be generating a report in the next few weeks. But to reiterate, for NB6 we are going to really work to not make use of *any* extensions to the standard (beyond using the hardcoded constructor, which is required by the spec). We'll see how feasible that is, but I'm hopeful given that in our code the only reference to non-standard methods is to make use of a PropertyChangeNotifier on CachedRowSet, when property notification is not supported in the standard. If we can live without that, then I think we're good to go.

JD-3 (Unresolved)

Is this affected by the incompatibilities between Java 5 and Java 6 in java.sql.Connection? Will there be different versions of the RowSet library for each source level?

DavidVanCouvering I talked to Lance about this. The interface CachedRowSetX extends CachedRowSet which extends ResultSet, which has had changes to it in Java 6. So if we were to build our CachedRowSetX interface using JDK 6, then apps built with JDK 5 would fail when they tried to reference CachedRowSetX, because the new JDBC 4 methods would not be available.

My understanding after talking to Lance is that as long as we use JDK 5 to build CachedRowSetX, we should be fine. But I'm going to test and make sure. In particular, since we are *generating* code that is then compiled by the developer, we may run into problems when they compile with JDK 6. But I'll test this and see.

JWB-3 (Unresolved)

Any progress on discussing with Sandip (Insync) on this limitation : "property inspector needs to work with classes instead of interfaces. "  ?

DavidVanCouvering Sorry, I thought I answered this. It has to be a class, so we will expose the property as a CachedRowSetImpl rather than CachedRowSet. Check the updated spec, I think I describe how this is done.

It turns out I can use a BeanInfo class to control what properties and events are exposed. This means I can hide the properties and events on the CachedRowSetImpl class that are extensions to the standard CachedRowSet. This is goodness, as this means we can guarantee that our users are using the standard and are not locking into a particular implementation.

JWB-4 (Unresolved)

For the property sheet and any dialog prompts, have all use cases been determined and if so, is any UI work determined (e.g. properties and/or dialog prompts, such as mentioned in PB-2 ?

DavidVanCouvering There is no UI work required, no property sheet changes. The property sheet gets all its information through introspection and the BeanInfo class.

Not logged in. Log in, Register

By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo