MODE-1791 - Added the "readonly" attribute to the connector hierarchy and implemented support for it in the base connector class #688
Conversation
| <xs:annotation> | ||
| <xs:documentation>Flag which indicates whether the external source is readonly or supports writing as well. By default, a source is read-only.</xs:documentation> | ||
| <xs:documentation>Flag which indicates whether the external source is readonly or supports writing as well. By default, a source support both reads and writes.</xs:documentation> |
There was a problem hiding this comment.
I originally chose default value of "true" (meaning read-only) so that the connector was safe by default, and would not modify any of the files under the location. Because connectors are used only for federation, preventing modification seemed at the time to be a prudent and not terribly inappropriate default.
Do we not want that "safe-by-default" behavior?
There was a problem hiding this comment.
I'm fine with the safe-by-default being true. However, if that is the case, each connector implementation that supports writes, will have to make sure to change the default value from the base class to "false", as long as it's using the checkConnectorWritable method which will throw an exception if the connector is read-only.
There was a problem hiding this comment.
I guess this is the downside of making it generic - it might be difficult to decide what should be the default. And once the default has been coded in the JSON Schema and AS7 XSD, then won't all connector configurations that want to support writes have to use readonly="false"?
The other option is to have the default for readonly be false like you have it here. Perhaps that is best, even though it's not "safest".
There was a problem hiding this comment.
My original thought behind making it "false" as default, was that because of the existence of the ReadonlyConnector abstract class, it would be much easier for read-only connectors to be implemented,
as opposed to making sure a writable connector overrides the flag somehow.
There was a problem hiding this comment.
Right. But this is dealing with writable connectors that are to be configured as read-only. I think this boils down to whether writeable connectors should be writable by default or read-only by default. I can certainly imagine that some writeable connectors (e.g., JCR) would be frequently used in a writable manner. My thinking, however, was that people configuring them should explicitly make the choice to allow ModeShape to update content in another system. I guess I'm still leaning in that direction but am by no means certain.
There was a problem hiding this comment.
Fair enough (I'm fine with either setting being the default)
There was a problem hiding this comment.
Actually, the more I think about this, the more I think we're overcomplicating things with the "read-only" flag. If a connector is writable, what are the chances/cases when a client would want it configured in "read-only" mode ? (I'm starting to doubt the usefulness of this flag I think)
There was a problem hiding this comment.
I still think it's very worthwhile - I absolutely think it should be possible to prevent ModeShape from modifying content in external systems. For example, imagine a web application exposing some files on the file system but not wanting to allow any modification/changes to those files. (This is actually done on JBoss.org, though using 2.x.)
Now, I might be able to be convinced that this shouldn't be built-into all connectors, but instead built into those connectors that make sense. If that's what we want, then I would push for it to be added to the FileSystemConnector.
Perhaps we need additional feedback. I'll post to the forums.
There was a problem hiding this comment.
It's worth mentioning that the FS connector has/had this flag. This PR removed it and moved it upwards.
But I think you're right: the fact that we support custom properties, means that any time a connector would need such a flag, it would just add it and implement it in whatever way it sees fit. The configuration/setting mechanism would set it on the connector if present.
|
Can you take a look at this again and rebase? This no longer is a straight merge since the changes for MODE-1805 have been merged. |
… and implemented support for it in the base connector class. Also. updated the JSON schema to support it.
…itableConnector base class which now contains the flag and by moving the read-only checks to the FederatedDocumentStore.
|
Updated the commits after rebasing & merging and update the code which now has a WritableConnector base class. |
|
Rebased and merged into the '3.1.x' branch, then cherry-picked into the 'master' branch. |
Also, updated the JSON schema to support it.
This should be merged into 3.1.x and cherry-picked into master.