anchela commented on code in PR #157:
URL:
https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/157#discussion_r1109772898
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/SlingInitialContentBundleEntryMetaDataCollector.java:
##########
@@ -68,6 +80,20 @@ class SlingInitialContentBundleEntryMetaDataCollector {
this.contentPackage2FeatureModelConverter =
contentPackage2FeatureModelConverter;
this.newBundleFile = newBundleFile;
this.jarFile = context.getJarFile();
+
+ this.defaultContentXmlFile = new File(this.basePath,
"defaultContentXml.xml");
+ writeDefaultXMLFile();
+ }
+
+ private void writeDefaultXMLFile() {
Review Comment:
i would recommend to slightly refactor this method and incorporate the new
File statement as well..... for example
```
this.defaultContentXmlFile = createDefaultXMLFile(basePath);
}
private static @NotNull File createDefaultXMLFile(@NotNull String
basePath) throws ConverterException {
File f = new File(basePath, "defaultContentXml.xml");
try(OutputStream outputStream = new FileOutputStream(f)){
IOUtils.copy(Objects.requireNonNull(CL.getResourceAsStream("default-content.xml")),
outputStream);
} catch (FileNotFoundException e) {
throw new ConverterException("Default Content XML file not found
in classpath!", e);
} catch (IOException e) {
throw new ConverterException("Could not prepare the default
content XML file for sling initial content!", e);
}
return f;
}
```
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/SlingInitialContentBundleEntryMetaDataCollector.java:
##########
@@ -68,6 +80,20 @@ class SlingInitialContentBundleEntryMetaDataCollector {
this.contentPackage2FeatureModelConverter =
contentPackage2FeatureModelConverter;
this.newBundleFile = newBundleFile;
this.jarFile = context.getJarFile();
+
+ this.defaultContentXmlFile = new File(this.basePath,
"defaultContentXml.xml");
+ writeDefaultXMLFile();
+ }
+
+ private void writeDefaultXMLFile() {
+ try(OutputStream outputStream = new
FileOutputStream(defaultContentXmlFile)){
+
IOUtils.copy(Objects.requireNonNull(CL.getResourceAsStream("default-content.xml")),
outputStream);
+ } catch (FileNotFoundException e) {
+ throw new RuntimeException("Default Content XML file not found in
classpath!", e);
Review Comment:
i would recommend to not throw a `RuntimeException` but rather a
`ConverterException` here. the `BundleSlingInitialContentExtractor` method that
deals with the meta-data-collector already throws `ConverterException`.
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/SlingInitialContentBundleEntryMetaDataCollector.java:
##########
@@ -106,10 +132,54 @@ Set<SlingInitialContentBundleEntryMetaData>
collectFromContextAndWriteTmpFiles()
}
}
}
-
+
return collectedSlingInitialContentBundleEntries;
}
+
+ Set<SlingInitialContentBundleEntryMetaData> getDefaultContentXmlEntries()
throws UnsupportedEncodingException {
+
+ createDefaultContentXMLFiles();
+ return defaultContentXmlEntries;
+
+ }
+
+ private void createDefaultContentXMLFiles() throws
UnsupportedEncodingException {
+
+ for(String repositoryPath:
collectedSlingInitialContentBundleEntries.stream()
+ .map(SlingInitialContentBundleEntryMetaData::getRepositoryPath)
+ .collect(Collectors.toList())){
+ addDefaultSlingContentXMLAndParents(repositoryPath);
+ }
+ }
+
+ private void addDefaultSlingContentXMLAndParents(String repositoryPath)
throws UnsupportedEncodingException {
+ if(StringUtils.countMatches(repositoryPath, '/') < 2){
+ //we don't add the primary type of level 1 and 2.
+ return;
+ }
+
+ String parentPath = StringUtils.substringBeforeLast(repositoryPath,
"/");
+ //if we already got an entry with the path defined, we skip ahead.
+ if( !alreadyFound(collectedSlingInitialContentBundleEntries,
parentPath) &&
+ !alreadyFound(defaultContentXmlEntries, parentPath)
+ ){
+ final PathEntry pathEntryValue =
getPathEntryFromRepositoryPath(repositoryPath);
+ if(pathEntryValue != null){
+ defaultContentXmlEntries.add(new
SlingInitialContentBundleEntryMetaData(defaultContentXmlFile, pathEntryValue,
parentPath + "/" + DOT_CONTENT_XML));
+ }
+
+ }
+ addDefaultSlingContentXMLAndParents(parentPath);
+
+ }
+
+ private boolean alreadyFound(Set<SlingInitialContentBundleEntryMetaData>
set, String parentPath){
Review Comment:
i believe `alreadyFound` can be static.
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/SlingInitialContentBundleEntryMetaDataCollector.java:
##########
@@ -68,6 +80,20 @@ class SlingInitialContentBundleEntryMetaDataCollector {
this.contentPackage2FeatureModelConverter =
contentPackage2FeatureModelConverter;
this.newBundleFile = newBundleFile;
this.jarFile = context.getJarFile();
+
+ this.defaultContentXmlFile = new File(this.basePath,
"defaultContentXml.xml");
+ writeDefaultXMLFile();
+ }
+
+ private void writeDefaultXMLFile() {
+ try(OutputStream outputStream = new
FileOutputStream(defaultContentXmlFile)){
Review Comment:
nitpicking: missing spaces after try and before {
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/SlingInitialContentBundleEntryMetaDataCollector.java:
##########
@@ -106,10 +132,54 @@ Set<SlingInitialContentBundleEntryMetaData>
collectFromContextAndWriteTmpFiles()
}
}
}
-
+
return collectedSlingInitialContentBundleEntries;
}
+
+ Set<SlingInitialContentBundleEntryMetaData> getDefaultContentXmlEntries()
throws UnsupportedEncodingException {
+
+ createDefaultContentXMLFiles();
+ return defaultContentXmlEntries;
+
+ }
+
+ private void createDefaultContentXMLFiles() throws
UnsupportedEncodingException {
+
+ for(String repositoryPath:
collectedSlingInitialContentBundleEntries.stream()
+ .map(SlingInitialContentBundleEntryMetaData::getRepositoryPath)
+ .collect(Collectors.toList())){
+ addDefaultSlingContentXMLAndParents(repositoryPath);
+ }
+ }
+
+ private void addDefaultSlingContentXMLAndParents(String repositoryPath)
throws UnsupportedEncodingException {
Review Comment:
add @NotNull annotation to the repositoryPath param
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/SlingInitialContentBundleEntryMetaDataCollector.java:
##########
@@ -106,10 +121,54 @@ Set<SlingInitialContentBundleEntryMetaData>
collectFromContextAndWriteTmpFiles()
}
}
}
-
+
return collectedSlingInitialContentBundleEntries;
}
+
+ Set<SlingInitialContentBundleEntryMetaData> getDefaultContentXmlEntries()
throws UnsupportedEncodingException {
+
+ createDefaultContentXMLFiles();
+ return defaultContentXmlEntries;
+
+ }
+ private void createDefaultContentXMLFiles() throws
UnsupportedEncodingException {
+
+ for(String repositoryPath:
collectedSlingInitialContentBundleEntries.stream()
+ .map(SlingInitialContentBundleEntryMetaData::getRepositoryPath)
+ .collect(Collectors.toList())){
+ addDefaultSlingContentXMLAndParents(repositoryPath);
+ }
+ }
+
+ private void addDefaultSlingContentXMLAndParents(String repositoryPath)
throws UnsupportedEncodingException {
+
+ if(StringUtils.countMatches(repositoryPath, '/') < 2){
Review Comment:
also: could it be that the repositorypath contains a trailing / ? then you
would actually continue.....
but similar to roberts finding: what is different with level 1 and 2? that
looks brittle....
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/SlingInitialContentBundleEntryMetaDataCollector.java:
##########
@@ -68,6 +80,20 @@ class SlingInitialContentBundleEntryMetaDataCollector {
this.contentPackage2FeatureModelConverter =
contentPackage2FeatureModelConverter;
this.newBundleFile = newBundleFile;
this.jarFile = context.getJarFile();
+
+ this.defaultContentXmlFile = new File(this.basePath,
"defaultContentXml.xml");
+ writeDefaultXMLFile();
+ }
+
+ private void writeDefaultXMLFile() {
+ try(OutputStream outputStream = new
FileOutputStream(defaultContentXmlFile)){
+
IOUtils.copy(Objects.requireNonNull(CL.getResourceAsStream("default-content.xml")),
outputStream);
+ } catch (FileNotFoundException e) {
+ throw new RuntimeException("Default Content XML file not found in
classpath!", e);
+ } catch (IOException e) {
+ throw new RuntimeException("Could not prepare the default content
XML file for sling initial content!", e);
Review Comment:
same: don't throw runtime-exception
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/SlingInitialContentBundleEntryMetaDataCollector.java:
##########
@@ -106,10 +132,54 @@ Set<SlingInitialContentBundleEntryMetaData>
collectFromContextAndWriteTmpFiles()
}
}
}
-
+
return collectedSlingInitialContentBundleEntries;
}
+
+ Set<SlingInitialContentBundleEntryMetaData> getDefaultContentXmlEntries()
throws UnsupportedEncodingException {
Review Comment:
annotate return value with @NotNull
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/SlingInitialContentBundleEntryMetaDataCollector.java:
##########
@@ -106,10 +132,54 @@ Set<SlingInitialContentBundleEntryMetaData>
collectFromContextAndWriteTmpFiles()
}
}
}
-
+
return collectedSlingInitialContentBundleEntries;
}
+
+ Set<SlingInitialContentBundleEntryMetaData> getDefaultContentXmlEntries()
throws UnsupportedEncodingException {
+
+ createDefaultContentXMLFiles();
+ return defaultContentXmlEntries;
+
+ }
+
+ private void createDefaultContentXMLFiles() throws
UnsupportedEncodingException {
+
+ for(String repositoryPath:
collectedSlingInitialContentBundleEntries.stream()
+ .map(SlingInitialContentBundleEntryMetaData::getRepositoryPath)
+ .collect(Collectors.toList())){
+ addDefaultSlingContentXMLAndParents(repositoryPath);
+ }
+ }
+
+ private void addDefaultSlingContentXMLAndParents(String repositoryPath)
throws UnsupportedEncodingException {
+ if(StringUtils.countMatches(repositoryPath, '/') < 2){
+ //we don't add the primary type of level 1 and 2.
+ return;
+ }
+
+ String parentPath = StringUtils.substringBeforeLast(repositoryPath,
"/");
+ //if we already got an entry with the path defined, we skip ahead.
+ if( !alreadyFound(collectedSlingInitialContentBundleEntries,
parentPath) &&
+ !alreadyFound(defaultContentXmlEntries, parentPath)
+ ){
+ final PathEntry pathEntryValue =
getPathEntryFromRepositoryPath(repositoryPath);
+ if(pathEntryValue != null){
+ defaultContentXmlEntries.add(new
SlingInitialContentBundleEntryMetaData(defaultContentXmlFile, pathEntryValue,
parentPath + "/" + DOT_CONTENT_XML));
+ }
+
+ }
+ addDefaultSlingContentXMLAndParents(parentPath);
+
+ }
+
+ private boolean alreadyFound(Set<SlingInitialContentBundleEntryMetaData>
set, String parentPath){
Review Comment:
add @NotNull annotations for the params
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/SlingInitialContentBundleEntryMetaDataCollector.java:
##########
@@ -106,10 +132,54 @@ Set<SlingInitialContentBundleEntryMetaData>
collectFromContextAndWriteTmpFiles()
}
}
}
-
+
return collectedSlingInitialContentBundleEntries;
}
+
+ Set<SlingInitialContentBundleEntryMetaData> getDefaultContentXmlEntries()
throws UnsupportedEncodingException {
+
+ createDefaultContentXMLFiles();
+ return defaultContentXmlEntries;
+
+ }
+
+ private void createDefaultContentXMLFiles() throws
UnsupportedEncodingException {
+
+ for(String repositoryPath:
collectedSlingInitialContentBundleEntries.stream()
+ .map(SlingInitialContentBundleEntryMetaData::getRepositoryPath)
+ .collect(Collectors.toList())){
+ addDefaultSlingContentXMLAndParents(repositoryPath);
+ }
+ }
+
+ private void addDefaultSlingContentXMLAndParents(String repositoryPath)
throws UnsupportedEncodingException {
+ if(StringUtils.countMatches(repositoryPath, '/') < 2){
+ //we don't add the primary type of level 1 and 2.
+ return;
+ }
+
+ String parentPath = StringUtils.substringBeforeLast(repositoryPath,
"/");
Review Comment:
again... what if the path ends with trailing /?
i would strongly recommend to use utilities methods from
`org.apache.jackrabbit.util.Text` instead. getRelativeParent is probably what
you need here.
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/SlingInitialContentBundleEntryMetaDataCollector.java:
##########
@@ -106,10 +132,54 @@ Set<SlingInitialContentBundleEntryMetaData>
collectFromContextAndWriteTmpFiles()
}
}
}
-
+
return collectedSlingInitialContentBundleEntries;
}
+
+ Set<SlingInitialContentBundleEntryMetaData> getDefaultContentXmlEntries()
throws UnsupportedEncodingException {
+
+ createDefaultContentXMLFiles();
+ return defaultContentXmlEntries;
+
+ }
+
+ private void createDefaultContentXMLFiles() throws
UnsupportedEncodingException {
Review Comment:
`UnsupportedEncodingException` is never thrown if you removed the
unnecessary throws clause from `addDefaultSlingContentXMLAndParents`
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/SlingInitialContentBundleEntryMetaDataCollector.java:
##########
@@ -106,10 +132,54 @@ Set<SlingInitialContentBundleEntryMetaData>
collectFromContextAndWriteTmpFiles()
}
}
}
-
+
return collectedSlingInitialContentBundleEntries;
}
+
+ Set<SlingInitialContentBundleEntryMetaData> getDefaultContentXmlEntries()
throws UnsupportedEncodingException {
+
+ createDefaultContentXMLFiles();
+ return defaultContentXmlEntries;
+
+ }
+
+ private void createDefaultContentXMLFiles() throws
UnsupportedEncodingException {
+
+ for(String repositoryPath:
collectedSlingInitialContentBundleEntries.stream()
+ .map(SlingInitialContentBundleEntryMetaData::getRepositoryPath)
+ .collect(Collectors.toList())){
+ addDefaultSlingContentXMLAndParents(repositoryPath);
+ }
+ }
+
+ private void addDefaultSlingContentXMLAndParents(String repositoryPath)
throws UnsupportedEncodingException {
Review Comment:
UnsupportedEncodingException is never thrown (according to my IDE). please
remove it.
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/SlingInitialContentBundleEntryMetaDataCollector.java:
##########
@@ -106,10 +132,54 @@ Set<SlingInitialContentBundleEntryMetaData>
collectFromContextAndWriteTmpFiles()
}
}
}
-
+
return collectedSlingInitialContentBundleEntries;
}
+
+ Set<SlingInitialContentBundleEntryMetaData> getDefaultContentXmlEntries()
throws UnsupportedEncodingException {
Review Comment:
`UnsupportedEncodingException` is never thrown if you remove the unnecessary
clause from `createDefaultContentXMLFiles` and
`addDefaultSlingContentXMLAndParents`
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/SlingInitialContentBundleEntryMetaDataCollector.java:
##########
@@ -190,6 +260,21 @@ private SlingInitialContentBundleEntryMetaData
createSlingInitialContentBundleEn
String repositoryPath = (target != null ? target : "/") +
URLDecoder.decode(entryName.substring(pathEntryValue.getPath().length()),
"UTF-8");
return new SlingInitialContentBundleEntryMetaData(targetFile,
pathEntryValue, repositoryPath);
}
+
+
+ private PathEntry getPathEntryFromRepositoryPath(@NotNull String
repositoryPath) {
+
+ if(StringUtils.countMatches(repositoryPath, '/') < 2){
+ //we don't add the primary type of level 1 and 2.
+ return null;
+ }
+
+ Optional<PathEntry> pathEntryOptional =
context.getPathEntryList().stream().filter(
+ pathEntry ->
checkIfPathStartsWithOrIsEqual(pathEntry.getTarget(), repositoryPath)
Review Comment:
instead of creating `checkIfPathStartsWithOrIsEqual` yourself, i would
strongly recommend to use again Text utility. `isDescendantOrEqual`
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/SlingInitialContentBundleEntryMetaDataCollector.java:
##########
@@ -190,6 +260,21 @@ private SlingInitialContentBundleEntryMetaData
createSlingInitialContentBundleEn
String repositoryPath = (target != null ? target : "/") +
URLDecoder.decode(entryName.substring(pathEntryValue.getPath().length()),
"UTF-8");
return new SlingInitialContentBundleEntryMetaData(targetFile,
pathEntryValue, repositoryPath);
}
+
+
+ private PathEntry getPathEntryFromRepositoryPath(@NotNull String
repositoryPath) {
Review Comment:
add @Nullable annotation for return value.
--
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]
