anchela commented on code in PR #157:
URL:
https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/157#discussion_r1109589399
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/AssemblerProvider.java:
##########
@@ -99,9 +99,9 @@ private VaultPackageAssembler lazyConstruct(@NotNull
BundleSlingInitialContentEx
private void addPathFilterSetToAssemblerFilter(@NotNull PathEntry
pathEntry, @NotNull VaultPackageAssembler assembler) {
ImportMode importMode;
if (pathEntry.isOverwrite()) {
- importMode = ImportMode.UPDATE;
+ importMode = ImportMode.REPLACE;
} else {
- importMode = ImportMode.MERGE;
+ importMode = ImportMode.MERGE_PROPERTIES;
Review Comment:
is this change absolutely required for the task at hand?
do you have a test illustrating the effect?
i am not opposed but want to avoid introducing new problems.
maybe @kwin can also provide some input to make sure we understand any
potential unwanted side-effects
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/AssemblerProvider.java:
##########
@@ -99,9 +99,9 @@ private VaultPackageAssembler lazyConstruct(@NotNull
BundleSlingInitialContentEx
private void addPathFilterSetToAssemblerFilter(@NotNull PathEntry
pathEntry, @NotNull VaultPackageAssembler assembler) {
ImportMode importMode;
if (pathEntry.isOverwrite()) {
- importMode = ImportMode.UPDATE;
+ importMode = ImportMode.REPLACE;
Review Comment:
@niekraaijmakers , i had to lookup the definition of pathEntry.isOverwrite()
which points to
```
public abstract class ImportOptions {
/**
* Specifies whether imported nodes should overwrite existing nodes.
* NOTE: this means the existing node will be deleted and a new node
* will be created in the same location.
* @return true to overwrite nodes, false otherwise
*/
public abstract boolean isOverwrite();
```
while i agree that ImportMode.REPLACE is probably the better match (see
https://jackrabbit.apache.org/filevault/apidocs/org/apache/jackrabbit/vault/fs/api/ImportMode.html),
i am wondering why we had UPDATE before. do you recall that?
i am bit concerned that changing it will cause regressions. in any case it
would be great to have some dedicated tests for that illustrating the
difference in behavior.
specially since there is probably not a 1:1 match with filevault.... e.g.
there is also PathEntry.isPropertyOverwrite and isOverwrite is defined to only
affect nodes.... while i believe that the fv-REPLACE affects both nodes and
props, no?
##########
src/main/java/org/apache/sling/feature/cpconverter/ContentPackage2FeatureModelConverter.java:
##########
@@ -328,7 +328,6 @@ private void secondPass(@NotNull Collection<VaultPackage>
orderedContentPackages
// finally serialize the Feature Model(s) file(s)
aclManager.addRepoinitExtension(assemblers,
featuresManager);
-
bundleSlingInitialContentExtractor.addRepoInitExtension(assemblers,
featuresManager);
Review Comment:
hm..... correct me if i am wrong.... looking at the whole class it seems to
me that you are removing the only place where the
`bundleSlingInitialContentExtractor` is actually being invoked making changes
to the sling initial content.
am i missing something?
or phrased differently:
what is left after removing that statement is
- line 293: bundleSlingInitialContentExtractor.reset()
- line 342: bundleSlingInitialContentExtractor.reset()
- line 528:
setBundleSlingInitialContentExtractor(BundleSlingInitialContentExtractor)
invoked from the launcher.
and what is the reset doing now? and why do you still need it here?
it is effectively used in `DefaultEntryHandlersManager` which passes it to
`BundleEntryHandler`, no? why still referencing it in the converter?
it might all be ok and needed but in that case i believe a bit of comment
explaining why you are resetting it, might help. alternatively, leave that all
to the handlersmanager.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
