Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MODE-1791 - Added the "readonly" attribute to the connector hierarchy and implemented support for it in the base connector class #688

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -525,6 +525,7 @@ public void validateParameter( String parameterName,
.setAllowExpression(false)
.setAllowNull(true)
.setFlags(AttributeAccess.Flag.RESTART_NONE)
.setDefaultValue(new ModelNode(false))
.build();


Expand Down
Expand Up @@ -367,9 +367,9 @@
<xs:documentation>The number of seconds which an external node should be held in the workspace cache. If not specified, the effective TTL is deferred to the workspace cache configuration. If negative, an entry will be cached forever.</xs:documentation>
</xs:annotation>
</xs:attribute>
<xs:attribute name="readonly" type="xs:boolean" use="optional" default="true">
<xs:attribute name="readonly" type="xs:boolean" use="optional" default="false">
<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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough (I'm fine with either setting being the default)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

</xs:annotation>
</xs:attribute>
<xs:attribute name="queryable" type="xs:boolean" use="optional" default="true">
Expand Down
Expand Up @@ -167,8 +167,11 @@ public void shouldExtractTextFromPdfFilePdfContext() throws Exception {
}

public static String randomString(int length) {
//write a text only header to make sure Tika Mimetype detector doesn't get confused...
String header = "this is a text file ";
StringBuilder rndStringBuilder = new StringBuilder(length);
for (int i = 0; i < length; i++) {
rndStringBuilder.append(header);
for (int i = 0; i < length - header.length(); i++) {
rndStringBuilder.append(CHARS.charAt(RANDOM.nextInt(CHARS.length())));
}
return rndStringBuilder.toString();
Expand Down
Expand Up @@ -199,8 +199,6 @@
<local-cache name="federatedRepository">
<!-- ModeShape requires transactions -->
<transaction mode="NON_XA"/>
<!-- Use a cache with file-backed write-through storage. File-backed storage is simple, but not necessarily the fastest. -->
<file-store passivation="false" path="modeshape/store/federatedRepository" relative-to="jboss.server.data.dir" purge="false"/>
</local-cache>
</cache-container>
<cache-container name="binary-cache-container" default-cache="binary-fs">
Expand Down Expand Up @@ -313,7 +311,6 @@
<text-extractor name="tika-extractor" classname="tika" module="org.modeshape.extractor.tika"/>
</text-extractors>
<cache-binary-storage data-cache-name="binary-fs" metadata-cache-name="binary-fs-meta" cache-container="binary-cache-container"/>
<!--<db-binary-storage data-source-jndi-name="java:jboss/datasources/ModeshapeBinaryDS"/>-->
</repository>
<repository name="preconfiguredRepository">
<indexing rebuild-upon-startup="never"/>
Expand Down
Expand Up @@ -27,6 +27,7 @@
import java.io.File;
import javax.annotation.Resource;
import javax.jcr.Node;
import javax.jcr.RepositoryException;
import javax.jcr.query.Query;
import org.jboss.arquillian.container.test.api.Deployment;
import org.jboss.arquillian.junit.Arquillian;
Expand All @@ -41,6 +42,7 @@
import org.modeshape.jcr.api.federation.FederationManager;
import static junit.framework.Assert.assertEquals;
import static junit.framework.Assert.assertNotNull;
import static junit.framework.Assert.fail;

/**
* Integration test which verifies that various external sources are correctly set-up via the JBoss AS subsystem.
Expand Down Expand Up @@ -85,6 +87,19 @@ public void shouldHaveFileSystemSourceConfigured() throws Exception {
assertNotNull(otherSession.getNode("/projection1"));
}

@Test
public void shouldNotAllowWritesIfConfiguredAsReadonly() throws Exception {
Session defaultSession = repository.login();
Node projection1 = defaultSession.getNode("/projection1");
try {
projection1.addNode("test", "nt:file");
defaultSession.save();
fail("Write operation should not be possible if connector is readonly");
} catch (RepositoryException e) {
//expected
}
}

@Test
public void shouldHaveGitSourceConfigured() throws Exception {
Session session = repository.login();
Expand Down
Expand Up @@ -51,6 +51,7 @@
import org.modeshape.jcr.federation.spi.DocumentChanges;
import org.modeshape.jcr.federation.spi.DocumentReader;
import org.modeshape.jcr.federation.spi.DocumentWriter;
import org.modeshape.jcr.federation.spi.WritableConnector;
import org.modeshape.jcr.value.BinaryKey;
import org.modeshape.jcr.value.BinaryValue;
import org.modeshape.jcr.value.Name;
Expand Down Expand Up @@ -105,7 +106,7 @@
* </tr>
* </table>
*/
public class FileSystemConnector extends Connector {
public class FileSystemConnector extends WritableConnector {

private static final String FILE_SEPARATOR = System.getProperty("file.separator");
private static final String DELIMITER = "/";
Expand Down Expand Up @@ -143,13 +144,6 @@ public class FileSystemConnector extends Connector {
private String directoryAbsolutePath;
private int directoryAbsolutePathLength;

/**
* A boolean flag that specifies whether this connector should only read existing files and directories on the file system and
* should never write, update, or create files and directories on the file systems. This is set via reflection and defaults to
* "<code>false</code>".
*/
private boolean readonly = false;

/**
* A boolean flag that specifies whether this connector should add the 'mix:mimeType' mixin to the 'nt:resource' nodes to
* include the 'jcr:mimeType' property. If set to <code>true</code>, the MIME type is computed immediately when the
Expand Down Expand Up @@ -389,12 +383,8 @@ protected boolean isExcluded( File file ) {
* @throws DocumentStoreException if the file is expected to be writable but is not or is excluded, or if the connector is
* readonly
*/
protected void checkWritable( String id,
File file ) {
if (readonly) {
String msg = JcrI18n.fileConnectorIsReadOnly.text(getSourceName(), id, file.getAbsolutePath());
throw new DocumentStoreException(id, msg);
}
protected void checkFileNotExcluded( String id,
File file ) {
if (isExcluded(file)) {
String msg = JcrI18n.fileConnectorCannotStoreFileThatIsExcluded.text(getSourceName(), id, file.getAbsolutePath());
throw new DocumentStoreException(id, msg);
Expand Down Expand Up @@ -486,7 +476,7 @@ public String getDocumentId( String path ) {
@Override
public boolean removeDocument( String id ) {
File file = fileFor(id);
checkWritable(id, file);
checkFileNotExcluded(id, file);
// Remove the extra properties at the old location ...
extraPropertiesStore().removeProperties(id);
// Now remove the file (if it is there) ...
Expand All @@ -501,7 +491,7 @@ public void storeDocument( Document document ) {
DocumentReader reader = readDocument(document);
String id = reader.getDocumentId();
File file = fileFor(id);
checkWritable(id, file);
checkFileNotExcluded(id, file);
File parent = file.getParentFile();
if (!parent.exists()) {
parent.mkdirs();
Expand Down Expand Up @@ -557,11 +547,11 @@ public String newDocumentId( String parentId,
@Override
public void updateDocument( DocumentChanges documentChanges ) {
String id = documentChanges.getDocumentId();

Document document = documentChanges.getDocument();
DocumentReader reader = readDocument(document);

File file = fileFor(id);
checkWritable(id, file);

//if we're dealing with the root of the connector, we can't process any moves/removes because that would go "outside" the connector scope
if (!isRoot(id)) {
Expand Down
Expand Up @@ -1052,7 +1052,12 @@ final AbstractJcrNode addChildNode( Name childName,

//If there isn't a desired key, check if the document store doesn't require a certain key format (this is especially used by federation)
if (desiredKey == null) {
String documentStoreKey = session().repository().documentStore().newDocumentKey(key().toString(), childName, childPrimaryNodeTypeName);
String documentStoreKey = null;
try {
documentStoreKey = session().repository().documentStore().newDocumentKey(key().toString(), childName, childPrimaryNodeTypeName);
} catch (Exception e) {
throw new RepositoryException(e);
}
if (documentStoreKey != null) {
desiredKey = new NodeKey(documentStoreKey);
}
Expand Down
Expand Up @@ -110,6 +110,7 @@ public String newDocumentKey( String parentKey,
}
Connector connector = connectors.getConnectorForSourceKey(sourceKey(parentKey));
if (connector != null) {
checkConnectorIsWritable(connector);
String parentDocumentId = documentIdFromNodeKey(parentKey);
String newChildId = connector.newDocumentId(parentDocumentId, documentName, documentPrimaryType);
if (!StringUtil.isBlank(newChildId)) {
Expand All @@ -128,6 +129,7 @@ public SchematicEntry storeDocument( String key,
}
Connector connector = connectors.getConnectorForSourceKey(sourceKey(key));
if (connector != null) {
checkConnectorIsWritable(connector);
EditableDocument editableDocument = replaceNodeKeysWithDocumentIds(document);
connector.storeDocument(editableDocument);
}
Expand All @@ -143,6 +145,7 @@ public void updateDocument( String key,
} else {
Connector connector = connectors.getConnectorForSourceKey(sourceKey(key));
if (connector != null) {
checkConnectorIsWritable(connector);
EditableDocument editableDocument = replaceNodeKeysWithDocumentIds(document);
String documentId = documentIdFromNodeKey(key);
SessionNode.NodeChanges nodeChanges = sessionNode.getNodeChanges();
Expand Down Expand Up @@ -319,6 +322,7 @@ public boolean remove( String key ) {
}
Connector connector = connectors.getConnectorForSourceKey(sourceKey(key));
if (connector != null) {
checkConnectorIsWritable(connector);
boolean result = connector.removeDocument(documentIdFromNodeKey(key));
connectors.externalNodeRemoved(key);
return result;
Expand Down Expand Up @@ -578,4 +582,11 @@ private EditableDocument replaceNodeKeysWithDocumentIds( Document document ) {
return writer.document();
}

private void checkConnectorIsWritable( Connector connector ) throws ConnectorException {
if (connector.isReadonly()) {
throw new ConnectorException(JcrI18n.connectorIsReadOnly.text(connector.getSourceName()));
}
}


}
Expand Up @@ -56,6 +56,9 @@
/**
* SPI of a generic external connector, representing the interface to an external system integrated with ModeShape. Since it is
* expected that the documents are well formed (structure-wise), the {@link FederatedDocumentWriter} class should be used.
*
* This is the base class for {@link WritableConnector} and {@link ReadOnlyConnector} which is what connector implementations
* are expected to implement.
*
* @author Horia Chiorean (hchiorea@redhat.com)
*/
Expand Down Expand Up @@ -315,6 +318,13 @@ public void shutdown() {
*/
public abstract String getDocumentId( String path );

/**
* Indicates if the connector instance has been configured in read-only mode.
*
* @return {@code true} if the connector has been configured in read-only mode, false otherwise.
*/
public abstract boolean isReadonly();

/**
* Returns a document representing a single child reference from the supplied parent to the supplied child. This method is
* called when there are an unknown number of children on a node.
Expand Down
Expand Up @@ -24,7 +24,6 @@
package org.modeshape.jcr.federation.spi;

import org.infinispan.schematic.document.Document;
import org.modeshape.jcr.JcrI18n;
import org.modeshape.jcr.cache.DocumentStoreException;
import org.modeshape.jcr.value.Name;

Expand All @@ -38,29 +37,32 @@ public abstract class ReadOnlyConnector extends Connector {

@Override
public final boolean removeDocument( String id ) {
String msg = JcrI18n.connectorIsReadOnly.text(getSourceName(), id);
throw new DocumentStoreException(id, msg);
//this should never really be called, because FederatedDocumentStore performs the check
throw new UnsupportedOperationException("Connector is readonly");
}

@Override
public final void storeDocument( Document document ) {
DocumentReader reader = readDocument(document);
String id = reader.getDocumentId();
String msg = JcrI18n.connectorIsReadOnly.text(getSourceName(), id);
throw new DocumentStoreException(id, msg);
//this should never really be called, because FederatedDocumentStore performs the check
throw new UnsupportedOperationException("Connector is readonly");
}

@Override
public final void updateDocument( DocumentChanges documentChanges ) {
String documentId = documentChanges.getDocumentId();
String msg = JcrI18n.connectorIsReadOnly.text(getSourceName(), documentId);
throw new DocumentStoreException(documentId, msg);
//this should never really be called, because FederatedDocumentStore performs the check
throw new UnsupportedOperationException("Connector is readonly");
}

@Override
public String newDocumentId( String parentId,
Name newDocumentName,
Name newDocumentPrimaryType ) {
throw new DocumentStoreException(JcrI18n.connectorIsReadOnly.text(getSourceName(), newDocumentName));
//this should never really be called, because FederatedDocumentStore performs the check
throw new UnsupportedOperationException("Connector is readonly");
}

@Override
public boolean isReadonly() {
return true;
}
}
@@ -0,0 +1,46 @@
/*
* ModeShape (http://www.modeshape.org)
* See the COPYRIGHT.txt file distributed with this work for information
* regarding copyright ownership. Some portions may be licensed
* to Red Hat, Inc. under one or more contributor license agreements.
* See the AUTHORS.txt file in the distribution for a full listing of
* individual contributors.
*
* ModeShape is free software. Unless otherwise indicated, all code in ModeShape
* is licensed to you under the terms of the GNU Lesser General Public License as
* published by the Free Software Foundation; either version 2.1 of
* the License, or (at your option) any later version.
*
* ModeShape is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this software; if not, write to the Free
* Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
* 02110-1301 USA, or see the FSF site: http://www.fsf.org.
*/

package org.modeshape.jcr.federation.spi;

/**
* A specialized abstract {@link Connector} class that is support both reads and writes. In addition, this class has a {@code readonly}
* flag allowing clients to configure a writable connector (external source) as read-only. In this case, all of the write operations
* will throw an exception.

* @author Horia Chiorean (hchiorea@redhat.com)
*/
public abstract class WritableConnector extends Connector {

/**
* A flag which indicates whether a connector allows both write & read operations, or only read operations. If a connector
* is read-only, any attempt to write content (add/update/delete etc) will result in an exception being raised.
*/
private boolean readonly = false;

@Override
public boolean isReadonly() {
return readonly;
}
}
Expand Up @@ -98,7 +98,7 @@ fileConnectorNamespaceIgnored = The file system external source "{0}" does not s
couldNotStoreProperties = The external source "{0}" cannot store these extra properties on node "{1}": {2}
couldNotStoreProperty = The external source "{0}" cannot store the extra property "{2}" on node "{1}"
couldNotGetMimeType = The external source "{0}" cannot get the MIME type on node "{1}": {2}
connectorIsReadOnly = The external source "{0}" is read-only and cannot update or create the node "{1}".
connectorIsReadOnly = The external source "{0}" is read-only and therefore does not support adding/updating and removing nodes.

rootNodeHasNoParent = The root node has no parent node
rootNodeIsNotProperty = The root path "/" refers to the root node, not a property
Expand Down
Expand Up @@ -325,6 +325,11 @@
"type" : "integer",
"description" : "The optional number of seconds which an external node should be held in the workspace cache. If not specified, the effective TTL is deferred to the workspace cache configuration. If negative, an entry will be cached forever."
},
"readonly" : {
"type" : "boolean",
"default" : false,
"description" : "Optional flag which indicates if an external source should support both writing and reading, or just reading. This flag is ignored for read-only sources and makes sense only for writable sources."
},
"projections" : {
"type" : "array",
"items" : {
Expand Down
Expand Up @@ -39,6 +39,7 @@
import org.modeshape.jcr.SingleUseAbstractTest;
import org.modeshape.jcr.api.Session;
import org.modeshape.jcr.api.federation.FederationManager;
import org.modeshape.jcr.federation.spi.ConnectorException;
import static org.hamcrest.core.Is.is;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertThat;
Expand Down
Expand Up @@ -38,19 +38,19 @@
import org.infinispan.schematic.document.EditableDocument;
import org.modeshape.jcr.JcrNtLexicon;
import org.modeshape.jcr.api.nodetype.NodeTypeManager;
import org.modeshape.jcr.federation.spi.Connector;
import org.modeshape.jcr.federation.spi.DocumentChanges;
import org.modeshape.jcr.federation.spi.DocumentReader;
import org.modeshape.jcr.federation.spi.DocumentWriter;
import org.modeshape.jcr.federation.spi.PageKey;
import org.modeshape.jcr.federation.spi.Pageable;
import org.modeshape.jcr.federation.spi.WritableConnector;
import org.modeshape.jcr.value.Name;
import org.modeshape.jcr.value.Property;

/**
* Mock implementation of a {@code Connector} which uses some hard-coded, in-memory external nodes to validate the Connector SPI.
*/
public class MockConnector extends Connector implements Pageable {
public class MockConnector extends WritableConnector implements Pageable {
public static final String DOC1_LOCATION = "/doc1";
public static final String DOC2_LOCATION = "/doc2";

Expand Down