output plugin core

[dventimiglia]

I created a new plugin that generates different output (xml, json, etc.). In order to hook into the HenPlus rendering, I had to make changes to the "core" HenPlus code. In concert, I added the plugin code itself. Subsequently, I divided those changes into two branches, one built on top of the other. This pull request is the result of the first branch, which in my repo is dav-output-plugin-core.

The changes in that branch, and in this pull request, tell the following narrative.

1. Though the plugin may add additional commands ("porcelain" in git parlance), essentially it relies on HenPlus's built-in property mechanisms.
2. A new HenPlus property ("output") governs the selection of the output type.
3. A new EnumeratedPropertyHolder sub-type adds "meta-data", associating an output type ("table") with a ResultSetRenderer sub-type.
4. Instead of hard-coding ResultSetRenderer, SQLCommand looks up the new "output" property, gets the meta-data, and uses Java Reflection to instantiate the appropriate ResultSetRenderer sub-type.

To that end, it has these changes (which map to commits).

1. SQLCommand needs access to the main HenPlus PropertyRegistry, hence it needs access to the main HenPlus instance. Therefore, change the Command interface to support setting a HenPlus instance.
2. PluginCommand already gets a HenPlus instance. Therefore, change PluginCommand to pass its instance on to daughter commands it manages, via the change from 1. above.
3. Different renderers are implemented as ResultSetRenderer sub-types. Therefore, change that class's internal member variables from 'private' to 'protected'.
4. Likewise, so support a new EnumeratedPropertyHolder sub-type, so that we can add meta-data, change that class's internal member variables from 'private' to 'protected'.
5. Since plugins that use this code will add new enumerated properties ("xml", "json", etc.) by installing a new EnumeratedPropertyHolder (see 4. above), they should save the old PropertyHolder (that had, say, just "table" as a possible value). Therefore, change the PropertyRegistry so that when unregistering a property, it returns the old PropertyHolder (so that it can be saved).
6. Command instances (like SQLCommand) will need to access HenPlus and its PropertyRegistry (see 1. and 2. above). But, HenPlus instances don't make their internal PropertyRegistry available. Therefore, add to HenPlus an accessor for its PropertyRegistry.
7. Create the new EnumeratedPropertyHolder sub-type (see 4. above).
8. Create and register the new property we discussed above, "output", give it just the one value "table" and make its meta-data point to just the original ResultSetRenderer.
9. In SQLCommand, use all the new stuff from above, plus Java Reflection, to create the necessary renderer class.

After these set of changes, HenPlus should continue to work essentially the same way. The only user-visible change should be a new property, "output", whose value is "table." Without a plugin to add new possible values, "table" is the only choice. It maps to the stock table output rendering that HenPlus has always had.

[neurolabs]

Took a first look - sorry for the delay. Very tidy, thanks for that. I will have to try it out and grasp the big picture.

[neurolabs]

Also took a look at the plugin implementation. Do you think it would be possible for the OutputTypeCommand to extend AbstractPropertyCommand, so it would not need to summon a command via the dispatcher in #execute ? And if that works, the plugin does not need the HenPlus instance, but could use an interface for accessing only the PropertyRegistry.

What do you think?

[dventimiglia]

Let me address these suggestions separately.

Make OutputTypeCommand extend AbstractPropertyCommand

Yes, I could certainly do that. In fact, I thought about it before. I just confirmed for myself that this would work.

Make OutputTypeCommand use an interface for accessing only the PropertyRegistry.

The question I couldn't answer at the time was this. How would OutputTypeCommand implement the AbstractPropertyCommand.getRegistry method? PropertyCommand and SessionPropertyCommand can implement this method because in their constructors they're both passed a reference to the HenPlus PropertyRegistry when they're created in HenPlus.initializeCommands (in fact, PropertyCommand also gets a reference to the main HenPlus instance for good measure). But, "plugins" are Command instances that are created inside PluginCommand via Java Reflection, using a no-argument constructor. One approach would be to change PluginCommand so that it uses Reflection to check for the presence of a constructor that takes a PropertyRegistry and if it exists, invoke that one instead.

Make OutputTypeCommand rely on AbstractPropertyCommand.execute rather than summon a command via the dispatcher

The problems I had when I tried this were these.

• You have to implement getSetCommand with a genuine, unique String command value. It can't be null, it can't be the empty string, and it can't be something that's already registered (like "set-property").
• That's why I made it "set-output." I don't really want a new command (all I want the plugin to do is add extra values for the "output" property), but I have to put something there.
• If I make it "set-output" but don't override the execute method, I get the behavior in AbstractPropertyCommand. That works, but I get exactly the behavior of the "set-property" command. For example "set-output" by itself lists all properties, not just the "output" property. Basically, "set-output" becomes an alias to "set-property", but I find that behavior confounding to the user.
• If I make it "set-output" but then do override the execute method, I find I have to re-implement much of the functionality of AbstractPropertyCommand.execute, which is laborsome.

Ultimately, the behavior I want is this.

• Don't register a new command at all. Alas, I don't have that option.
• Since I have to register some new command, at least register one that only operates on the "output" property.
• Since it only operates on the "output" property, calls to "set-output" shouldn't have to include "output" in the command string (that'd be redundant).
• Calls to "reset-output" with no arguments should reset the "output" property to "table." It shouldn't print the usage, the way that "reset-property" does.

Maybe I'm overlooking something and there's a "right" way to do this. I'm certainly open to suggestions.

Proposal

How about a compromise? How about I do extend AbstractPropertyCommand, but keep the existing implementation of the execute method? I'll have to make the reflection changes to PluginCommand described above, but at least the Command interface and HenPlus class won't have to change. How does that sound?

If you're amenable to this approach, I'll make the necessary changes and change the history so that the changes to Command and HenPlus won't appear in the pull request (but new changes to PluginCommand and OutputTypeCommand will appear).

Warm regards, David

[dventimiglia]

Hey, I rebased-away the interface changes that I'd made to Command.java and AbstractCommand.java. In particular, the setHenPlus, getHenPlus, and unregister methods that I had added to the interface, are now gone, reverting Command.java back to how it was originally.

But, as I said in the above comment, there still has to be some way to get either the HenPlus instance and/or the PropertyRegistry into the OutputTypeCommand plugin. For now, I elected still to pass in the HenPlus instance, rather than a PropertyRegistry. PluginCommand already has a HenPlus instance handy (though it'd be trivial to get the PropertyRegistry from it). Moreover, at present I'm still using the command dispatcher in OutputTypeCommand.execute, which still calls for having access to the HenPlus instance.

In any case, without Command.java interface methods to rely on, PluginCommand now uses Reflection to look for a constructor it can use to pass in the HenPlus instance. It also uses Reflection to look for an "unregister" method it can call on "plug-out."

Hopefully, these changes mean that this PR has a smaller "footprint" upon HenPlus core. Granted, there still are the changes to SQLCommand, PluginCommand, etc., but at least the Command/AbstractCommand changes are gone.

What do you think? Is this a worthwhile direction?

Warm regards, David

P.S. I think you also asked me to create a tag #41, is that correct?

[neurolabs]

Sorry for leaving you hanging on your update.

Generally, I am happy with the thoughts you have put into this. And since this codebase is not in best shape anyways, and I don't have the time to quarrel further, I would just merge the current state.

Which head from your repo is the one I should merge? Can I just merge output-plugin-plugin, or do you want to add the plugin implementation on top of output-plugin-core?