anchela commented on code in PR #157:
URL:
https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/157#discussion_r1109629933
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/BundleSlingInitialContentJarEntryExtractor.java:
##########
@@ -70,7 +67,8 @@ class BundleSlingInitialContentJarEntryExtractor {
*/
void extractAndAddToAssembler(@NotNull
BundleSlingInitialContentExtractContext context,
@NotNull
SlingInitialContentBundleEntryMetaData slingInitialContentBundleEntryMetaData,
- @NotNull
Set<SlingInitialContentBundleEntryMetaData>
collectedSlingInitialContentBundleEntries) throws IOException,
ConverterException {
+ @NotNull
Set<SlingInitialContentBundleEntryMetaData>
collectedSlingInitialContentBundleEntries,
+ @NotNull boolean isDefaultContentXmlFile)
throws IOException, ConverterException {
Review Comment:
this new boolean param is missing in the javadoc comment -> add it and
explain what this is used for..... why you have to add it now :)
my first reaction was: eh.... what on earth is a non-default-content-xml
file and what makes it a default-content-xml-file.
help me understand the code, pliiiiiiiis :)
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/BundleSlingInitialContentJarEntryExtractor.java:
##########
@@ -117,17 +115,21 @@ void extractAndAddToAssembler(@NotNull
BundleSlingInitialContentExtractContext c
}
- try (Archive virtualArchive =
SingleFileArchive.fromPathOrInputStream(tmpDocViewInputFile,
bundleFileInputStream,
- () ->
Files.createTempFile(context.getConverter().getTempDirectory().toPath(),
"initial-content", Text.getName(file.getName())), contentPackageEntryPath)) {
- // in which content package should this end up?
-
- if (tmpDocViewInputFile != null) {
- packageAssembler.addEntry(contentPackageEntryPath,
tmpDocViewInputFile.toFile());
- } else {
- packageAssembler.addEntry(contentPackageEntryPath,
bundleFileInputStream);
+ //if we are writing in a .content.xml file as default,
+ // but the .content.xml is already present (written by
specifications, then we don't do anything.)
Review Comment:
i think the ) needs to go in a different place to make it clear, no?
`// but the .content.xml is already present (written by specifications),
then we don't do anything.`
##########
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()
Review Comment:
nitpicking: missing space between for and (
##########
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){
Review Comment:
nitpicking: missing space between if and (
##########
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) &&
Review Comment:
space missing. please reformat your code to make it consistent with the rest
of the module
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/BundleSlingInitialContentJarEntryExtractor.java:
##########
@@ -84,7 +82,7 @@ void extractAndAddToAssembler(@NotNull
BundleSlingInitialContentExtractContext c
VaultPackageAssembler packageAssembler =
assemblerProvider.initPackageAssemblerForPath(context, repositoryPath,
pathEntryValue);
final ContentReader contentReader =
contentReaderProvider.getContentReaderForEntry(file, pathEntryValue);
- if (contentReader != null) {
+ if (contentReader != null && !isDefaultContentXmlFile) {
Review Comment:
if `isDefaultContentXmlFile` is false you don't need to obtain the
`contentReader` to start with, no?
i am not sure i understand why this is the case, but it would make the code
more readable if you would re-arrange it and only retrieve the contentreader if
needed.
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/BundleSlingInitialContentJarEntryExtractor.java:
##########
@@ -117,17 +115,21 @@ void extractAndAddToAssembler(@NotNull
BundleSlingInitialContentExtractContext c
}
- try (Archive virtualArchive =
SingleFileArchive.fromPathOrInputStream(tmpDocViewInputFile,
bundleFileInputStream,
- () ->
Files.createTempFile(context.getConverter().getTempDirectory().toPath(),
"initial-content", Text.getName(file.getName())), contentPackageEntryPath)) {
- // in which content package should this end up?
-
- if (tmpDocViewInputFile != null) {
- packageAssembler.addEntry(contentPackageEntryPath,
tmpDocViewInputFile.toFile());
- } else {
- packageAssembler.addEntry(contentPackageEntryPath,
bundleFileInputStream);
+ //if we are writing in a .content.xml file as default,
+ // but the .content.xml is already present (written by
specifications, then we don't do anything.)
+ if(!(isDefaultContentXmlFile &&
packageAssembler.alreadyPresent(contentPackageEntryPath))){
Review Comment:
nitpicking: you have a space missing after the if.
the serious part:
i am trying to map your comment above to this if-clause.... you only want to
skip this step if `isDefaultContentXmlFile` is `true` and the entry is already
present, right?
if `isDefaultContentXmlFile` is false you don't care about the collision and
if the entry does not exist you can safely ignore the
`isDefaultContentXmlFile`... is that what you are saying?
why? and how did it work before? because the check for alreadyPresent was
not present before, right?
please elaborate in the comment for the next dev looking at this being able
to follow you
##########
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){
Review Comment:
again 2 times
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/SlingInitialContentBundleEntryMetaDataCollector.java:
##########
@@ -190,6 +249,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){
Review Comment:
and add a space after if and before { (or let the IDE fix the formatting)
##########
src/main/java/org/apache/sling/feature/cpconverter/handlers/slinginitialcontent/SlingInitialContentBundleEntryMetaData.java:
##########
@@ -74,4 +74,11 @@ public String toString() {
"repositoryPath='" + repositoryPath + '\'' +
'}';
}
+
+ @Override
+ public int compareTo(@NotNull SlingInitialContentBundleEntryMetaData o) {
+ return this.repositoryPath.compareTo(o.repositoryPath);
Review Comment:
i am not convinced that string comparison is sensible for paths. what is the
goal 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 {
+
+ 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)
+ ){
Review Comment:
again
##########
src/main/java/org/apache/sling/feature/cpconverter/vltpkg/VaultPackageAssembler.java:
##########
@@ -233,6 +233,11 @@ public void addEntry(@NotNull String path, @NotNull
InputStream input) throws IO
}
}
+ public @NotNull boolean alreadyPresent(@NotNull String path){
Review Comment:
nitpicking: missing space before {
--
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]
