You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2021/07/02 13:00:55 UTC
[GitHub] [sling-org-apache-sling-feature-cpconverter] anchela commented on a change in pull request #88: SLING-10469 - detecting dir folder and substructure belonging to nt f…
anchela commented on a change in pull request #88:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/88#discussion_r662989368
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/handlers/AbstractConfigurationEntryHandler.java
##########
@@ -48,47 +48,52 @@ public final void handle(@NotNull String path, @NotNull Archive archive, @NotNul
String runMode;
// we are pretty sure it matches, here
if (matcher.matches()) {
- String pid = matcher.group("pid");
-
- int idx = pid.lastIndexOf('/');
- if (idx != -1) {
- pid = pid.substring(idx + 1);
- }
- String factoryPid = null;
- String id;
- int n = pid.indexOf('~');
- if (n == -1) {
- n = pid.indexOf('-');
- }
- if (n > 0) {
- factoryPid = pid.substring(0, n);
- id = factoryPid.concat("~").concat(pid.substring(n + 1));
+ if (matcher.group("dir") != null) {
+ // preventing invalid results as the corresponding configuration will be stripped from the resulting package causing the
Review comment:
incomplete comment.... causing the [?]
i think the rest of the sentence is missing.
maybe also adding link to JIRA for clarity?
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/handlers/AbstractConfigurationEntryHandler.java
##########
@@ -33,7 +33,7 @@
private boolean enforceConfigurationBelowConfigFolder;
public AbstractConfigurationEntryHandler(@NotNull String extension) {
- super("/jcr_root/(?:apps|libs)/.+/(?<foldername>config|install)(\\.(?<runmode>[^/]+))?/(?<pid>.*)\\." + extension);
+ super("/jcr_root/(?:apps|libs)/.+/(?<foldername>config|install)(\\.(?<runmode>[^/]+))?(.*)/(?<pid>[^\\/]*)\\." + extension + ("(?<dir>.dir(/\\.content\\.xml)?)?$"));
Review comment:
a comment would be nice to explain the regexp
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/handlers/AbstractConfigurationEntryHandler.java
##########
@@ -48,47 +48,52 @@ public final void handle(@NotNull String path, @NotNull Archive archive, @NotNul
String runMode;
// we are pretty sure it matches, here
if (matcher.matches()) {
- String pid = matcher.group("pid");
-
- int idx = pid.lastIndexOf('/');
- if (idx != -1) {
- pid = pid.substring(idx + 1);
- }
- String factoryPid = null;
- String id;
- int n = pid.indexOf('~');
- if (n == -1) {
- n = pid.indexOf('-');
- }
- if (n > 0) {
- factoryPid = pid.substring(0, n);
- id = factoryPid.concat("~").concat(pid.substring(n + 1));
+ if (matcher.group("dir") != null) {
+ // preventing invalid results as the corresponding configuration will be stripped from the resulting package causing the
+ logger.info("{} is only a dir folder next to config - removing.", path);
} else {
- id = pid;
- }
+ String pid = matcher.group("pid");
- logger.info("Processing configuration '{}'.", id);
+ int idx = pid.lastIndexOf('/');
Review comment:
shouldn't there be a check for pid being null?
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/handlers/AbstractConfigurationEntryHandler.java
##########
@@ -48,47 +48,52 @@ public final void handle(@NotNull String path, @NotNull Archive archive, @NotNul
String runMode;
// we are pretty sure it matches, here
Review comment:
pretty sure? what that does mean???
i would suggest to verify
##########
File path: src/main/java/org/apache/sling/feature/cpconverter/handlers/AbstractConfigurationEntryHandler.java
##########
@@ -48,47 +48,52 @@ public final void handle(@NotNull String path, @NotNull Archive archive, @NotNul
String runMode;
// we are pretty sure it matches, here
if (matcher.matches()) {
- String pid = matcher.group("pid");
-
- int idx = pid.lastIndexOf('/');
- if (idx != -1) {
- pid = pid.substring(idx + 1);
- }
- String factoryPid = null;
- String id;
- int n = pid.indexOf('~');
- if (n == -1) {
- n = pid.indexOf('-');
- }
- if (n > 0) {
- factoryPid = pid.substring(0, n);
- id = factoryPid.concat("~").concat(pid.substring(n + 1));
+ if (matcher.group("dir") != null) {
+ // preventing invalid results as the corresponding configuration will be stripped from the resulting package causing the
+ logger.info("{} is only a dir folder next to config - removing.", path);
} else {
- id = pid;
- }
+ String pid = matcher.group("pid");
- logger.info("Processing configuration '{}'.", id);
+ int idx = pid.lastIndexOf('/');
+ if (idx != -1) {
+ pid = pid.substring(idx + 1);
+ }
+ String factoryPid = null;
+ String id;
+ int n = pid.indexOf('~');
+ if (n == -1) {
+ n = pid.indexOf('-');
+ }
+ if (n > 0) {
+ factoryPid = pid.substring(0, n);
+ id = factoryPid.concat("~").concat(pid.substring(n + 1));
+ } else {
+ id = pid;
+ }
+
+ logger.info("Processing configuration '{}'.", id);
+
+ Dictionary<String, Object> configurationProperties;
+ try (InputStream input = Objects.requireNonNull(archive.openInputStream(entry))) {
+ configurationProperties = parseConfiguration(id, input);
+ }
+
+ if (configurationProperties == null) {
+ logger.info("{} entry does not contain a valid OSGi configuration, treating it as a regular resource", path);
+ Objects.requireNonNull(converter.getMainPackageAssembler()).addEntry(path, archive, entry);
Review comment:
note.... with changes made for SLING-10579 `converter.getMainPackageAssembler()` is annotated as return a @NoNull value. so, the check should be removed as it has been moved to the method itself to prevent having to check all over the place (and forgetting it in half of the cases)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org