Skip to content

JCRVLT-810 skip filter validation #378

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.

Sign up for GitHub

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

Open
joerghoh wants to merge 11 commits into
base: master
Choose a base branch
from
Open

JCRVLT-810 skip filter validation #378

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ba52eff
JCRVLT-810 initial approach
joerghoh Jul 23, 2025
13245e5
JCRVLT-810 add integration tests
joerghoh Jul 23, 2025
0216501
add failing test
joerghoh Jul 25, 2025
a0d6f3a
Merge branch 'master' into JCRVLT-810
joerghoh Jul 28, 2025
f032f82
JCRVLT-810 move the check to the DocViewImporter
joerghoh Jul 29, 2025
d8faa69
improve javadoc
joerghoh Aug 3, 2025
71d7eb6
skip checks on more occassions of importing a package
joerghoh Aug 3, 2025
e712424
JCRVLT-810 use a non-binary package for test-content
joerghoh Sep 7, 2025
737ae4b
Update vault-core/src/main/java/org/apache/jackrabbit/vault/fs/io/Imp…
joerghoh Sep 10, 2025
abe7a41
PR feedback
joerghoh Sep 10, 2025
e16806c
Update vault-core/src/main/java/org/apache/jackrabbit/vault/fs/io/Imp…
joerghoh Sep 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions ...ation-tests/src/main/java/org/apache/jackrabbit/vault/packaging/integration/ImportIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,4 +445,55 @@ public void testEnhancedFileAggregatePackageWithIntermediateSaves() throws IOExc
assertPropertyExists("/testroot/tika/config.xml/jcr:content/jcr:data");
assertProperty("/testroot/tika/config.xml/jcr:content/jcr:mimeType", "text/xml");
}

@Test
public void testSkipFilterChecksOnImport_disabled() throws Exception {
ImportOptions opts = getDefaultOptions();
Importer importer = new Importer(opts);
try (Archive archive = getFileArchive("/test-packages/outside-filters.zip")) {
archive.open(true);
importer.run(archive, admin.getRootNode());
admin.save();
}
assertNodeExists("/tmp/foo");
assertPropertyMissing("/tmp/foo/bar/tobi/excludeddProp");
Copy link
Copy Markdown
Member

@kwin kwin Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Please add some assertions what happens to nodes/properties below filter roots not contained in the package.

}

@Test
public void testSkipFilterChecksOnImport_enabled() throws Exception {
ImportOptions opts = getDefaultOptions();
opts.setSkipFilterChecksOnImport(true);
Importer importer = new Importer(opts);
try (Archive archive = getFileArchive("/test-packages/outside-filters.zip")) {
archive.open(true);
importer.run(archive, admin.getRootNode());
admin.save();
}
assertNodeExists("/tmp/foo");
assertPropertyExists("/tmp/foo/bar/tobi/excludedProp");
}

@Test
public void testSkipFilterChecksOnImport_enabled_mergeWithExisting() throws Exception {
ImportOptions opts = getDefaultOptions();
opts.setSkipFilterChecksOnImport(true);
Node tmp= admin.getRootNode().addNode("tmp");
Node tobi = tmp.addNode("foo").addNode("bar").addNode("tobi");
tobi.setProperty("excludedProp", "existingvalue");
tobi.setProperty("excludedProp-foo", "should-survive");
admin.save();
Importer importer = new Importer(opts);
try (Archive archive = getFileArchive("/test-packages/outside-filters.zip")) {
archive.open(true);
importer.run(archive, admin.getRootNode());
admin.save();
}
assertNodeExists("/tmp/foo");
assertPropertyExists("/tmp/foo/bar/tobi/excludedProp");
// import has overwritten existing property
assertProperty("/tmp/foo/bar/tobi/excludedProp", "excluded property");
// but an existing property which is not overwritten by content in the package remains available
assertProperty("/tmp/foo/bar/tobi/excludedProp-foo", "should-survive");
}

}
3 changes: 3 additions & 0 deletions ...tegration-tests/src/main/resources/test-packages/outside-filters.zip/META-INF/MANIFEST.MF
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Manifest-Version: 1.0
Created-By: 11.0.21 (Eclipse Adoptium)

77 changes: 77 additions & 0 deletions ...tion-tests/src/main/resources/test-packages/outside-filters.zip/META-INF/vault/config.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
<vaultfs version="1.1">
<!--
Defines the content aggregation. The order of the defined aggregates
is important for finding the correct aggregator.
-->
<aggregates>
<!--
Defines an aggregate that handles nt:file and nt:resource nodes.
-->
<aggregate type="file" title="File Aggregate"/>

<!--
Defines an aggregate that handles file/folder like nodes. It matches
all nt:hierarchyNode nodes that have or define a jcr:content
child node and excludes child nodes that are nt:hierarchyNodes.
-->
<aggregate type="filefolder" title="File/Folder Aggregate"/>

<!--
Defines an aggregate that handles nt:nodeType nodes and serializes
them into .cnd notation.
-->
<aggregate type="nodetype" title="Node Type Aggregate" />

<!--
Defines an aggregate that defines full coverage for certain node
types that cannot be covered by the default aggregator.
-->
<aggregate type="full" title="Full Coverage Aggregate">
<matches>
<include nodeType="rep:AccessControl" respectSupertype="true" />
<include nodeType="rep:Policy" respectSupertype="true" />
<include nodeType="cq:Widget" respectSupertype="true" />
<include nodeType="cq:EditConfig" respectSupertype="true" />
<include nodeType="cq:WorkflowModel" respectSupertype="true" />
<include nodeType="vlt:FullCoverage" respectSupertype="true" />
<include nodeType="mix:language" respectSupertype="true" />
<include nodeType="sling:OsgiConfig" respectSupertype="true" />
</matches>
</aggregate>

<!--
Defines an aggregate that handles nt:folder like nodes.
-->
<aggregate type="generic" title="Folder Aggregate">
<matches>
<include nodeType="nt:folder" respectSupertype="true" />
</matches>
<contains>
<exclude isNode="true" />
</contains>
</aggregate>

<!--
Defines the default aggregate
-->
<aggregate type="generic" title="Default Aggregator" isDefault="true">
<matches>
<!-- all -->
</matches>
<contains>
<exclude nodeType="nt:hierarchyNode" respectSupertype="true" />
</contains>
</aggregate>

</aggregates>

<!--
defines the input handlers
-->
<handlers>
<handler type="folder"/>
<handler type="file"/>
<handler type="nodetype"/>
<handler type="generic"/>
</handlers>
</vaultfs>
25 changes: 25 additions & 0 deletions ...c/main/resources/test-packages/outside-filters.zip/META-INF/vault/definition/.content.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?xml version="1.0" encoding="UTF-8"?>
<jcr:root xmlns:vlt="http://www.day.com/jcr/vault/1.0" xmlns:jcr="http://www.jcp.org/jcr/1.0" xmlns:nt="http://www.jcp.org/jcr/nt/1.0"
jcr:created="{Date}2011-06-07T14:31:49.179-07:00"
jcr:createdBy="admin"
jcr:description=""
jcr:lastModified="{Date}2011-06-07T14:31:49.179-07:00"
jcr:lastModifiedBy="admin"
jcr:primaryType="vlt:PackageDefinition"
buildCount="1"
cqVersion="5.3"
group="my_packages"
lastUnwrapped="{Date}2011-06-07T14:31:49.179-07:00"
lastUnwrappedBy="admin"
lastWrapped="{Date}2011-06-07T14:31:49.179-07:00"
lastWrappedBy="admin"
name="tmp"
version="">
<filter jcr:primaryType="nt:unstructured">
<f0
jcr:primaryType="nt:unstructured"
mode="replace"
root="/tmp"
rules="[]"/>
</filter>
</jcr:root>
6 changes: 6 additions & 0 deletions ...tion-tests/src/main/resources/test-packages/outside-filters.zip/META-INF/vault/filter.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
<workspaceFilter version="1.0">
<filter root="/tmp">
<exclude pattern="^.*/excludedProp.*" matchProperties="true"/>
</filter>
</workspaceFilter>
12 changes: 12 additions & 0 deletions ...n-tests/src/main/resources/test-packages/outside-filters.zip/META-INF/vault/nodetypes.cnd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<'sling'='http://sling.apache.org/jcr/sling/1.0'>
<'nt'='http://www.jcp.org/jcr/nt/1.0'>

[sling:Folder] > nt:folder
- * (undefined)
- * (undefined) multiple
+ * (nt:base) = sling:Folder version

[sling:OrderedFolder] > sling:Folder
orderable
+ * (nt:base) = sling:OrderedFolder version

19 changes: 19 additions & 0 deletions ...-tests/src/main/resources/test-packages/outside-filters.zip/META-INF/vault/properties.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?xml version="1.0" encoding="utf-8" standalone="no"?>
<!DOCTYPE properties SYSTEM "http://java.sun.com/dtd/properties.dtd">
<properties>
<comment>FileVault Package Properties</comment>
<entry key="createdBy">admin</entry>
<entry key="name">tmp</entry>
<entry key="lastModified">2011-06-07T14:31:49.179-07:00</entry>
<entry key="lastModifiedBy">admin</entry>
<entry key="created">2011-06-07T14:31:49.418-07:00</entry>
<entry key="buildCount">1</entry>
<entry key="version"/>
<entry key="dependencies"/>
<entry key="packageFormatVersion">2</entry>
<entry key="description"/>
<entry key="group">my_packages</entry>
<entry key="lastWrapped">2011-06-07T14:31:49.179-07:00</entry>
<entry key="lastWrappedBy">admin</entry>
<entry key="path">/etc/packages/my_packages/tmp</entry>
</properties>
16 changes: 16 additions & 0 deletions ...egration-tests/src/main/resources/test-packages/outside-filters.zip/jcr_root/.content.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?xml version="1.0" encoding="UTF-8"?>
<jcr:root xmlns:sling="http://sling.apache.org/jcr/sling/1.0" xmlns:jcr="http://www.jcp.org/jcr/1.0" xmlns:rep="internal"
jcr:mixinTypes="[rep:AccessControllable]"
jcr:primaryType="rep:root"
sling:resourceType="sling:redirect"
sling:target="/welcome.html">
<home/>
<rep:policy/>
<jcr:system/>
<etc/>
<apps/>
<libs/>
<content/>
<var/>
<tmp/>
</jcr:root>
5 changes: 5 additions & 0 deletions ...tion-tests/src/main/resources/test-packages/outside-filters.zip/jcr_root/tmp/.content.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<?xml version="1.0" encoding="UTF-8"?>
<jcr:root xmlns:sling="http://sling.apache.org/jcr/sling/1.0" xmlns:jcr="http://www.jcp.org/jcr/1.0"
jcr:primaryType="sling:OrderedFolder">
<foo/>
</jcr:root>
5 changes: 5 additions & 0 deletions ...-tests/src/main/resources/test-packages/outside-filters.zip/jcr_root/tmp/foo/.content.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<?xml version="1.0" encoding="UTF-8"?>
<jcr:root xmlns:sling="http://sling.apache.org/jcr/sling/1.0" xmlns:jcr="http://www.jcp.org/jcr/1.0"
jcr:primaryType="sling:OrderedFolder">
<bar/>
</jcr:root>
5 changes: 5 additions & 0 deletions ...ts/src/main/resources/test-packages/outside-filters.zip/jcr_root/tmp/foo/bar/.content.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<?xml version="1.0" encoding="UTF-8"?>
<jcr:root xmlns:sling="http://sling.apache.org/jcr/sling/1.0" xmlns:jcr="http://www.jcp.org/jcr/1.0"
jcr:primaryType="sling:OrderedFolder">
<tobi/>
</jcr:root>
5 changes: 5 additions & 0 deletions ...c/main/resources/test-packages/outside-filters.zip/jcr_root/tmp/foo/bar/tobi/.content.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<?xml version="1.0" encoding="UTF-8"?>
<jcr:root xmlns:sling="http://sling.apache.org/jcr/sling/1.0" xmlns:jcr="http://www.jcp.org/jcr/1.0"
jcr:primaryType="sling:OrderedFolder"
excludedProp="excluded property"
/>
Binary file added vault-core/.DS_Store
View file
Open in desktop
Binary file not shown.
13 changes: 13 additions & 0 deletions vault-core/src/main/java/org/apache/jackrabbit/vault/fs/config/DefaultWorkspaceFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,11 @@ public class DefaultWorkspaceFilter implements Dumpable, WorkspaceFilter {
*/
private ImportMode importMode;

/**
* skip filter checks when importing content
*/
private boolean skipFilterChecksOnImport = false;

/**
* Add a #PathFilterSet for nodes items.
* @param set the set of filters to add.
Expand Down Expand Up @@ -232,6 +237,14 @@ public void setImportMode(ImportMode importMode) {
this.importMode = importMode;
}

public void setSkipFilterChecksOnImport(boolean skipFilterChecksOnImport) {
this.skipFilterChecksOnImport = skipFilterChecksOnImport;
}

public boolean getSkipFilterChecksOnImport() {
Copy link
Copy Markdown
Member

@kwin kwin Jul 29, 2025

Choose a reason for hiding this comment

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

If this only affects import I would rather move it to ImportOptions and also clarify that this only affects checks against package items not about existing repo items.

Copy link
Copy Markdown
Contributor Author

@joerghoh joerghoh Aug 3, 2025

Choose a reason for hiding this comment

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

ImportOptions already has such a method; i clarified the javadoc as recommended

Copy link
Copy Markdown
Member

@kwin kwin Sep 5, 2025

Choose a reason for hiding this comment

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

No need to add new methods/fields to WorkspaceFilter then, just use the one from ImportOptions directly (i.e. pass as boolean argument to DocViewImporter

Copy link
Copy Markdown
Member

@kwin kwin Sep 8, 2025

Choose a reason for hiding this comment

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

Those methods should not be added to DefaultWorkspaceFilter either. Rather pass that as dedicated boolean flag to the importer.

Copy link
Copy Markdown
Contributor Author

@joerghoh joerghoh Sep 8, 2025

Choose a reason for hiding this comment

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

you mean extending the (already quite long) constructor of the DocViewImporter by this flag? That would make the patch much more complicated, as I would need to pass through that parameter through a series of other methods.

Copy link
Copy Markdown
Member

@kwin kwin Sep 8, 2025

Choose a reason for hiding this comment

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

I see these methods only being used once each....

Copy link
Copy Markdown
Contributor Author

@joerghoh joerghoh Sep 9, 2025

Choose a reason for hiding this comment

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

I tried to backtrack from the constructor of the DocViewImporter, which methods would need to be changed, and the list is getting long; where annotated with ... I stopped (line numbers might not be 100% accurate, as my eclipse behaves sometimes a bit strange (eclipse-platform/eclipse.platform.ui#3039).

But it shows, that passing this parameter through the entire call chain(s) would be quite an effort, and for that reason I augmented the DefaultWorkspaceFilter.

DocViewImporter.java (l249)
  -> AbstractArtifactHandler:importDocView (l170)
     -> AbstractArtifactHandler:importDocView (l166)
        -> FileArtifactHandler:accept (l231)
           -> ...
        -> FileArtifactHandler:importDocView (l236)
           -> FileArtifactHandler::accept (l231) -> see above
     -> GenericArtifactHandler:accept (l88)
        -> AbstractArtifactHandler:accept (l126)
           -> ...
        -> AbstractArtifactHandler:accept (l136)
           -> ...
        -> Importer:commit (l1085)
           -> Importer:commit (l1058)
              -> Importer:commit (recursion)
              -> Importer:run (l537)
                 -> ...

Copy link
Copy Markdown
Member

@kwin kwin Sep 9, 2025

Choose a reason for hiding this comment

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

But both FileArtifactHandler:accept and GenericArtifactHandler:accept have access to the ImportOptions which expose that already, so it needs to be passed along DocViewImporter, AbstractArtifactHandler:importDocView and FileArtifactHandler:importDocView which seems acceptable to me. Right now it really is duplicate in ImporterOptions and WorkspaceFilter.

Copy link
Copy Markdown
Member

@kwin kwin Sep 9, 2025

Choose a reason for hiding this comment

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

I can try to take a stab at it.

Copy link
Copy Markdown
Contributor Author

@joerghoh joerghoh Sep 10, 2025

Choose a reason for hiding this comment

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

I went through and changed them ... a bit less changes than I actually anticipated.

return this.skipFilterChecksOnImport;
}

/**
* {@inheritDoc}
*/
Expand Down
2 changes: 1 addition & 1 deletion vault-core/src/main/java/org/apache/jackrabbit/vault/fs/config/package-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
* limitations under the License.
*/

@Version("2.9.0")
@Version("2.10.0")
package org.apache.jackrabbit.vault.fs.config;

import org.osgi.annotation.versioning.Version;
21 changes: 17 additions & 4 deletions vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
import org.apache.jackrabbit.vault.fs.api.IdConflictPolicy;
import org.apache.jackrabbit.vault.fs.api.ImportInfo.Info;
import org.apache.jackrabbit.vault.fs.api.ImportInfo.Type;
import org.apache.jackrabbit.vault.fs.config.DefaultWorkspaceFilter;
import org.apache.jackrabbit.vault.fs.api.ImportMode;
import org.apache.jackrabbit.vault.fs.api.ItemFilterSet;
import org.apache.jackrabbit.vault.fs.api.NodeNameList;
Expand Down Expand Up @@ -225,6 +226,11 @@ public class DocViewImporter implements DocViewParserHandler {
*/
private final boolean isSnsSupported;

/**
* set true if filter checks can be skipped on import
*/
private boolean skipFilterChecksOnImport = false;
Comment thread
joerghoh marked this conversation as resolved.

/**
* Creates a new importer that will imports the
* items below the given root.
Expand Down Expand Up @@ -270,9 +276,13 @@ public DocViewImporter(Node parentNode, String rootNodeName,
for (Artifact a : artifacts.values(ArtifactType.HINT)) {
hints.add(rootPath + a.getRelativePath());
}

stack = new StackElement(parentNode, parentNode.isNew());
npResolver = new DefaultNamePathResolver(parentNode.getSession());

if (wspFilter instanceof DefaultWorkspaceFilter) {
skipFilterChecksOnImport = ((DefaultWorkspaceFilter) wspFilter).getSkipFilterChecksOnImport();
}
}

/**
Expand Down Expand Up @@ -1014,7 +1024,8 @@ private void removeReferences(@NotNull Node node) throws ReferentialIntegrityExc
// TODO: is this faster than using sysview import?
// set new primary type (but never set rep:root)
String newPrimaryType = ni.getPrimaryType().orElseThrow(() -> new IllegalStateException("Mandatory property 'jcr:primaryType' missing from " + ni));
if (importMode == ImportMode.REPLACE && !"rep:root".equals(newPrimaryType) && wspFilter.includesProperty(PathUtil.append(node.getPath(), JcrConstants.JCR_PRIMARYTYPE))) {
final boolean allowedByFilter = (skipFilterChecksOnImport || wspFilter.includesProperty(PathUtil.append(node.getPath(), JcrConstants.JCR_PRIMARYTYPE)));
if (importMode == ImportMode.REPLACE && !"rep:root".equals(newPrimaryType) && allowedByFilter) {
String currentPrimaryType = node.getPrimaryNodeType().getName();
if (!currentPrimaryType.equals(newPrimaryType)) {
vs.ensureCheckedOut();
Expand Down Expand Up @@ -1157,7 +1168,8 @@ private void removeReferences(@NotNull Node node) throws ReferentialIntegrityExc
// add the protected properties
for (DocViewProperty2 p : ni.getProperties()) {
String qualifiedPropertyName = npResolver.getJCRName(p.getName());
if (p.getStringValue().isPresent() && PROTECTED_PROPERTIES_CONSIDERED_FOR_NEW_NODES.contains(p.getName()) && wspFilter.includesProperty(nodePath + "/" + qualifiedPropertyName)) {
final boolean allowedByFilter = (skipFilterChecksOnImport || wspFilter.includesProperty(nodePath + "/" + qualifiedPropertyName));
if (p.getStringValue().isPresent() && PROTECTED_PROPERTIES_CONSIDERED_FOR_NEW_NODES.contains(p.getName()) && allowedByFilter) {
attrs = new AttributesImpl();
attrs.addAttribute(Name.NS_SV_URI, "name", "sv:name", ATTRIBUTE_TYPE_CDATA, qualifiedPropertyName);
attrs.addAttribute(Name.NS_SV_URI, "type", "sv:type", ATTRIBUTE_TYPE_CDATA, PropertyType.nameFromValue(p.getType()));
Expand Down Expand Up @@ -1277,7 +1289,8 @@ private boolean setUnprotectedProperties(@NotNull EffectiveNodeType effectiveNod
// add properties
for (DocViewProperty2 prop : ni.getProperties()) {
String name = npResolver.getJCRName(prop.getName());
if (prop != null && !isPropertyProtected(effectiveNodeType, prop) && (overwriteExistingProperties || !node.hasProperty(name)) && wspFilter.includesProperty(node.getPath() + "/" + name)) {
final boolean allowedByFilter = (skipFilterChecksOnImport || wspFilter.includesProperty(node.getPath() + "/" + name));
if (prop != null && !isPropertyProtected(effectiveNodeType, prop) && (overwriteExistingProperties || !node.hasProperty(name)) && allowedByFilter) {
// check if property is allowed
try {
modified |= prop.apply(node);
Expand Down
Loading