JCRVLT-810 skip filter validation

[joerghoh]

When controlling both package creation, transport and import checking the filters in import can be time-consuming activity. For that reason it should be possible to skip the validation of the filters, as it will always match. In my test case this reduced the package import time by about 25%.

Right now it only skips the validation of properties, but we could think about skipping the validation of node paths as well. Given that in my case we had around 10 properties per node in average, skipping the filters for adding nodes will just add some small benefit on top.

[joerghoh]

I moved the skip logic out of the workspace filter class, and into the DocViewImporter.

[kwin]

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.

[joerghoh]

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

[kwin]

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

[kwin]

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

[joerghoh]

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.

[kwin]

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

[joerghoh]

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

[kwin]

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.

[kwin]

I can try to take a stab at it.

[joerghoh]

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

[kwin]

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

[kwin]

Looks much better now, I think long term it makes sense to pass the full ImportOptions object to the ArtifactHandlers and also use that as argument for the DocViewImporter constructor, however that can be done outside this PR.

I am still wondering how the skipFilterChecksOnImport affect
a) node/property deletions
b) direct data serializations of files/folders

[joerghoh]

Right now this does only affect the case when properties are added to a new node or updated on an existing node, the codepath for the deletion of properties is not affected. So for that I don't see anything which changes the result of a package installation.

Also it is only supposed to work under the assumption, that you control both the creation and the consumption of the package, and that in both cases the same filter rules are used. In other words: when matching the content of the package to the filters, the result is expected to be always true.

[kwin]

Would be nice to cover that in a test case as well. Not only that deletion of properties no longer contained in the package still works but also that nodes below filter roots not contained in the package are still removed. Also this makes the name skipFilterChecksOnImport a bit misleading as actually the filter check is only skipped in very few cases during import.

[kwin]

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