Bug 134308

Why should we make it generic? Do you know a client for this API in 6.5?

Yes, there is a client.  The E/R team from Egypt wants to be able to attach the action "Create E/R diagram" to a
connection.  I told them I would try and add this, otherwise their UI is a lot clunkier.

Ah, I see. Better not to put it in dbapi then. Not a good idea to have external (not in the NetBeans repository)
friends, because then you can't control them.

Hi, Andrei.  If I don't put it in dbapi, this means creating it as an extension to the (stable) Database Explorer API,
since it's part of the db module.  Are you OK with this? (I am still learning the lay of the land in terms of how APIs
are done).

Created attachment 63701 [details]
Patch for proposed change

Please review this addition to the Database API that allows modules to register actions with database connection nodes.

From the architectural spec use case:

----------------

This feature allows modules to provide useful features to users by registering one or more actions with a Database
Explorer connection node.

The module registers an implementation of ConnectionActionProvider in its layer file under the folder
Databases/ConnectionActionProvider. The provider's getActions() method is invoked by the Database Explorer whenever a
connection node needs to know what actions to display. This method is passed in the DatabaseConnection for the current
selected connection, and if you want you can choose which actions, if any, you want to make available for this connection.

Here is an example registration of an ActionProvider:

    <folder name="Databases">
        <folder name="ConnectionActionProvider">
            <file name="MyConnectionActionProvider.instance">
                <attr name="instanceCreate" methodvalue="org.netbeans.testdriver.MyConnectionActionProvider.getDefault"/>
                <attr name="instanceOf" stringvalue="org.netbeans.api.db.explorer.ConnectionActionProvider"/>
            </file>
        </folder>
    </folder>
    

The action can obtain the current DatabaseConnection by implementing CookieAction and specifying DatbaseConnection.class
in the cookieClasses() method. Then in the enable() and performAction() methods of the action, you can call
Node.getCookie(DatabaseConnection.class) to get the DatabaseConnection. You can then use the connection to decide
whether or not the action is enabled and when executing the action. 

-------------------

The patch is attached.  Please don't be confused by the references to RootNodeActionXXX - this is a pre-existing feature
implemented as a friend API.  I renamed it from things like ActionProvider and ActionLoader to RootNodeActionProvider
and RootNodeActionLoader to make a clear distinction between this existing feature and the new support for adding an
action to a Connection node.

Created attachment 63713 [details]
Updated patch includes new interface ConnectionActionProvider

Apologies, the patch was missing the new file defining the new interface ConnectionActionProvider

Created attachment 63716 [details]
Updated patch - missing META-INF changes

Two things:

- I updated the patch to include a change to META-INF/services for dbapi, to support the name change from DbAction* to
RootNodeAction*

- I noticed this issue was originally to support adding an action to *any* DB Explorer node.  What I implemented is only
for the Connection node.  I think this is a good thing - it keeps things focused, and it's not clear to me why you would
want to add actions to other nodes.  If a specific requirement comes in, we can revisit this.

I was hoping that it will not only allow me to customize connection node, it will also allow me to add table and
recreate table, since there may be some extension create command. In my case it will be external table in Axion, may be
in Oracle they also have concept of external table an extension to standard CREATE semantics.

For other nodes, there are lot of possibilities, e.g., one may need bit more than what "Add Column" provides, and may be
 alter table or alter column is also a possibility. If I get the support to add action for "Tables" node, that will be
good enough for me, for now.

In general I observed the DDL capabilities are very limited, specifically no ALTER support, DROP does not have CASCADE,
CREATE does not allow GENERATED column etc. But again these can be done through the sql editor, but people who wanted to
build application/components just leveraging the DBExplorer (like me) will be happy to see it getting richer. 

Thinking about this further, it's not clear what we would make available on those other nodes, as there are no existing
APIs in the DB Explorer API for things like Column, Table, etc.  If an action is called on a Table node, for example,
what context would we make available?  I'd have to invent a Table class, a Column class, a View class, etc., etc., that
has the information you need.  Starting to sound a bit like a metadata model.

So making it generically available requires more thought, and this late in the game (I have three working days left for
this release) I think it's just not going to happen.

That said, many of the issues you raise are things we should fix in the DB Explorer proper, not as one-off features
provided by another module.  We definitely do want to add things like ALTER COLUMN, ALTER TABLE, cascade support, etc. 
If you need this to, rather than provide it in a separate module, perhaps you can help us make improvements there as
part of our next release? 

Agree, we requires more thought on this for sure. Also agree that they will be best suited for DBExplore. 

PH01: Method getActions() add elements directly to those retrieved from superimplementation. I don't know whether this
is by design or not, but it could be very dangerous. BTW there seem to be possible null pointer dereference at the
beginning of DatabaseNodeInfo.getActions.
PH02: I think some unit test would be very useful for future maintenance.

Thanks for your review, Petr

PH01: Method getActions() add elements directly to those retrieved from superimplementation. I don't know whether this
is by design or not, but it could be very dangerous. 

Do you mean dangerous because I'm assuming it's OK to add to a list someone else gave me, and I have no control over how
the superimplementation implements the list or what it's doing with it simultaneously?

If so, good point, I'll create a new list.  If you mean something else, please let me know.

BTW there seem to be possible null pointer dereference at the
beginning of DatabaseNodeInfo.getActions.

OK, thanks for catching this.

PH02: I think some unit test would be very useful for future maintenance.

OK.  

>>Do you mean dangerous because I'm assuming it's OK to add to a list someone else gave me, and I have no control over
how the superimplementation implements the list or what it's doing with it simultaneously?

Exactly.

Created attachment 63782 [details]
Updated patch includes changes recommended by Petr Hejl

Y01 Please use getLookup() instead of getCookie
Y02 Do not implement Node.Cookie by any class in your APIs
Y03 Please write a test to show how all the API pieces work together.

I am assuming this new API will be public.

Didn't have time to get to this, will revisit in next release

Reassigned to new owner.

Any will to push this forward?

Invaluable ideas . Just to add my thoughts , if people is requiring to merge two images , my kids encountered a tool here http://goo.gl/Ms4ebj