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 1 commit
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
9 changes: 4 additions & 5 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 @@ -241,6 +241,10 @@ 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 Expand Up @@ -273,11 +277,6 @@ public boolean covers(String path) {

@Override
public boolean includesProperty(String propertyPath) {
if (skipFilterChecksOnImport) {
// skip the filter checks if requested; assume that the package only includes content
// which matches the filters.
return true;
}
if (!covers(propertyPath)) {
// include all properties that are not covered by any filter. this is to ensure that the ancestor paths
// have at least jcr:primary type.
Expand Down
15 changes: 13 additions & 2 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 @@ -1277,7 +1287,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