output plugin core #40
Conversation
|
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. |
| @@ -93,6 +95,20 @@ protected int argumentCount(final String command) { | |||
| public void handleCommandline(final CommandLine line) { | |||
|
|
|||
| } | |||
|
|
|||
| @Override | |||
| public HenPlus getHenPlus() { | |||
There was a problem hiding this comment.
This can be protected, only needed in subclasses
There was a problem hiding this comment.
I killed the interface changes to Command.java and AbstractCommand.java. I.e., the getHenPlus, setHenPlus, and unregister methods that I originally had added, are now gone. Instead of PluginCommand expecting those to be interface methods (which is the way I had it before), now PluginCommand cautiously uses Java Reflection to grope around on the plugin Command implementation, looking for a constructor it can use to pass in a HenPlus ref, and looking for an unregister method on "plug-out."
Your point is well-taken regarding these methods being protected, but I think the need to change it has been obviated since I removed the methods altogether anyway.
|
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? |
|
Let me address these suggestions separately. Make OutputTypeCommand extend AbstractPropertyCommandYes, 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 dispatcherThe problems I had when I tried this were these.
Ultimately, the behavior I want is this.
Maybe I'm overlooking something and there's a "right" way to do this. I'm certainly open to suggestions. ProposalHow 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, |
|
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, P.S. I think you also asked me to create a tag #41, is that correct? |
First, look for a 1-arg constructor on the proposed plugin, that takes a HenPlus instance. If it exists, invoke that. If it doesn't, go the usual route and call the no-arg constructor. This way, PluginCommand can try to pass on its reference to the HenPlus instance to the daughter plugin Command, if that Command has a constructor capable of receiving it. This gets around having to change the Command interface with a setHenPlus method. It also uses Reflection to look for an "unregister" method it can call on "plug-out." Note that when this occurs, it swallows any exception. This is by design. The principle we're adhering to is "do no harm." The contract for plugins and the Command interface doesn't require an "unregister" method, so we shouldn't make a federal case out of it if it doesn't exist or we can't call it.
For sub-classing purposes, it's helpful to make member variables available by setting their access modifiers to 'protected'. I thought about surgically just changing from private-->protected only those members that I needed, but concluded it's cleaner and less confusing just to change them all. Naturally, there's a trade-off of convenience versus coupling here. An alternative approach to leaking the implementation of ResultSetRenderer into its sub-classes would be to provide accessors. I decided that'd be overboard.
For sub-classing purposes
We want plugins to be able to register new PropertyHolders in place of old PropertyHolders. To do that, they'll have to unregister the old one, first. When that happens, plugins should (if they're well-behaved) remember the old PropertyHolder so they can restore it when they themselves are unregistered. This change sets up this sequence of events by returning the unregistered PropertyHolder to the caller, who can (and should) then squirrel it away for safe-keeping.
Plugin Command objects that want to, say, establish new values for the "output" property, will need access to the "global" property store, held by HenPlus. This accessor method grants that.
This is a crucial infrastructure component for supporting more complex behavior around properties, commands, and plugins. Where we're headed is toward enabling plugins (i.e. Command sub-classes) that can manipulate properties. Moreover, to be useful they should be able to manipulate properties that have a richer structure. That's the goal of this particular change. The idea is to create a property that groups together other data. Essentially, it's a "complex" property, with internal structure. There are certainly other approaches for building in rich properties, but I felt this was a modest, incremental one. At the end of the day, the MetaDataPropertyHolder is just an EnumeratedPropertyHolder. It's just that each enumerated property value can have additional data associated with it. This will prove useful in subsequent changes, as we'll see. In particular, where this is useful for the specific use-case that I'm considering is this. I would like to have simple, fairly descriptive property values for different output renderings (e.g., "xml", "json", "text", "csv", etc.), but then each will have to be associated (somehow) with a classname that can be loaded. A plain EnumeratedPropertyHolder could of course handle the strings "xml", "json", etc., of course. But, the question would be, where would we store the corresponding classnames? That's where this new class comes in. It stores them as "meta-data".
We're adding a new system property called "output" and implementing it with a MetaDataPropertyHolder. Recall that a MetaDataPropertyHolder is just an EnumeratedPropertyHolder, except that it can store additional meta-data along with each enumerated value. We use that feature to associate a "renderer-class" meta-data value with the "table" enumerated value (the only and default value, initially), which is set to the stock ResultSetRenderer we all know and love. The reason we're doing this is in anticipation of plugins that can add to the available output types and establish other values for "renderer-class." The current change places HenPlus's original, default renderer-class, ResultSetRenderer, on an equal footing with any potential additional renderers.
Now that the stock ResultSetRenderer has been set as the "renderer-class" meta-data element for the "table" output type, we can switch to a Dependency-Injection model for creating the ResultSetRenderer (sub-class). Rather than hard-code it, it now uses reflection to instantiate a sub-type, which is determined by the "renderer-class" meta-data value.
|
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? |
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.
To that end, it has these changes (which map to commits).
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.