[GitHub] [tomcat] jfclere opened a new pull request #309: Allow recursive substitution of properties.

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] jfclere opened a new pull request #309: Allow recursive substitution of properties.

GitBox

jfclere opened a new pull request #309:
URL: https://github.com/apache/tomcat/pull/309


   Related to https://issues.redhat.com/browse/JWS-973


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] michael-o commented on pull request #309: Allow recursive substitution of properties.

GitBox

michael-o commented on pull request #309:
URL: https://github.com/apache/tomcat/pull/309#issuecomment-650097670


   I don't have access to the ticket even with my Red Hat Enterprise Account.


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] FSchumacher commented on pull request #309: Allow recursive substitution of properties.

GitBox
In reply to this post by GitBox

FSchumacher commented on pull request #309:
URL: https://github.com/apache/tomcat/pull/309#issuecomment-650099473


   That could lead to a StackOverflowError.
   ```
   @Test
   public void testReplacePropertiesRecursively() {
       Properties properties = new Properties();
       properties.setProperty("replaceMe", "something ${replaceMe}");
       IntrospectionUtils.replaceProperties("${replaceMe}", properties, null, null);
   }
   ```
   Perhaps we should add a max depth parameter here?


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] jfclere commented on pull request #309: Allow recursive substitution of properties.

GitBox
In reply to this post by GitBox

jfclere commented on pull request #309:
URL: https://github.com/apache/tomcat/pull/309#issuecomment-650128576


   I have opened the JIRA and yes the recursion needs to be controlled.


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] FSchumacher commented on pull request #309: Allow recursive substitution of properties.

GitBox
In reply to this post by GitBox

FSchumacher commented on pull request #309:
URL: https://github.com/apache/tomcat/pull/309#issuecomment-650230095


   I tried to push to this PR branch directly, but I think I messed it up :(
   The current change is not thread safe. I would go with a method parameter and a wrapper to count the iterations.
   ```
   public String method(String param) { return "something"; }
   ```
   to
   ```
   public String method(String param) { return method(param, 0); }
   private String method(String param, int depth) {
     if (depth > 5) { return param; }
     return method(param, depth+1);
   }
   ```


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] jfclere commented on pull request #309: Allow recursive substitution of properties.

GitBox
In reply to this post by GitBox

jfclere commented on pull request #309:
URL: https://github.com/apache/tomcat/pull/309#issuecomment-650262993


   @FSchumacher something like that?


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] ChristopherSchultz commented on a change in pull request #309: Allow recursive substitution of properties.

GitBox
In reply to this post by GitBox

ChristopherSchultz commented on a change in pull request #309:
URL: https://github.com/apache/tomcat/pull/309#discussion_r446392364



##########
File path: java/org/apache/tomcat/util/IntrospectionUtils.java
##########
@@ -332,7 +341,15 @@ public static String replaceProperties(String value,
         }
         if (prev < value.length())
             sb.append(value.substring(prev));
-        return sb.toString();
+        String newval = sb.toString();
+        if (newval.indexOf('$') < 0) {
+            return newval;
+        }
+        if (newval.equals(value))
+            return value;
+        if (log.isDebugEnabled())
+            log.debug("IntrospectionUtils.replaceProperties iter on: " + newval);
+        return replaceProperties(newval, staticProp, dynamicProp, classLoader, iterationCount+1);

Review comment:
       Does this need to be recursive? Can we not simply have a loop of replacements?

##########
File path: java/org/apache/tomcat/util/IntrospectionUtils.java
##########
@@ -285,10 +285,19 @@ public static Object getProperty(Object o, String name) {
     public static String replaceProperties(String value,
             Hashtable<Object,Object> staticProp, PropertySource dynamicProp[],
             ClassLoader classLoader) {
+            return replaceProperties(value, staticProp, dynamicProp, classLoader, 0);
+    }
 
+    private static String replaceProperties(String value,
+            Hashtable<Object,Object> staticProp, PropertySource dynamicProp[],
+            ClassLoader classLoader, int iterationCount) {
         if (value.indexOf('$') < 0) {

Review comment:
       We should be looking for `${` and not `$`. Class names often contain `$` because of nested classes, and class names are often used in system properties. Plus, if we are looking for `${` we should look for `${`.
   
   I realize that this is existing code, but we may as well improve it while we're in here.

##########
File path: java/org/apache/tomcat/util/IntrospectionUtils.java
##########
@@ -332,7 +341,15 @@ public static String replaceProperties(String value,
         }
         if (prev < value.length())
             sb.append(value.substring(prev));
-        return sb.toString();
+        String newval = sb.toString();
+        if (newval.indexOf('$') < 0) {

Review comment:
       Same, here: look for `${`, not `$`.




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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] jfclere commented on a change in pull request #309: Allow recursive substitution of properties.

GitBox
In reply to this post by GitBox

jfclere commented on a change in pull request #309:
URL: https://github.com/apache/tomcat/pull/309#discussion_r446793598



##########
File path: java/org/apache/tomcat/util/IntrospectionUtils.java
##########
@@ -332,7 +341,15 @@ public static String replaceProperties(String value,
         }
         if (prev < value.length())
             sb.append(value.substring(prev));
-        return sb.toString();
+        String newval = sb.toString();
+        if (newval.indexOf('$') < 0) {
+            return newval;
+        }
+        if (newval.equals(value))
+            return value;
+        if (log.isDebugEnabled())
+            log.debug("IntrospectionUtils.replaceProperties iter on: " + newval);
+        return replaceProperties(newval, staticProp, dynamicProp, classLoader, iterationCount+1);

Review comment:
       A loop would work to but that is more changes.




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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] jfclere commented on a change in pull request #309: Allow recursive substitution of properties.

GitBox
In reply to this post by GitBox

jfclere commented on a change in pull request #309:
URL: https://github.com/apache/tomcat/pull/309#discussion_r446794162



##########
File path: java/org/apache/tomcat/util/IntrospectionUtils.java
##########
@@ -332,7 +341,15 @@ public static String replaceProperties(String value,
         }
         if (prev < value.length())
             sb.append(value.substring(prev));
-        return sb.toString();
+        String newval = sb.toString();
+        if (newval.indexOf('$') < 0) {

Review comment:
       Do we want to process "bla ${variable} bla" too?
   If $ is not enough only ${...} makes sense no?




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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] FSchumacher commented on a change in pull request #309: Allow recursive substitution of properties.

GitBox
In reply to this post by GitBox

FSchumacher commented on a change in pull request #309:
URL: https://github.com/apache/tomcat/pull/309#discussion_r446873594



##########
File path: java/org/apache/tomcat/util/IntrospectionUtils.java
##########
@@ -332,7 +341,15 @@ public static String replaceProperties(String value,
         }
         if (prev < value.length())
             sb.append(value.substring(prev));
-        return sb.toString();
+        String newval = sb.toString();
+        if (newval.indexOf('$') < 0) {

Review comment:
       I think the first variant is the way to go. We even have tests, that check, if `abc${normal}xyz` gets replaced.




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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] FSchumacher commented on a change in pull request #309: Allow recursive substitution of properties.

GitBox
In reply to this post by GitBox

FSchumacher commented on a change in pull request #309:
URL: https://github.com/apache/tomcat/pull/309#discussion_r446874481



##########
File path: java/org/apache/tomcat/util/IntrospectionUtils.java
##########
@@ -332,7 +341,15 @@ public static String replaceProperties(String value,
         }
         if (prev < value.length())
             sb.append(value.substring(prev));
-        return sb.toString();
+        String newval = sb.toString();
+        if (newval.indexOf('$') < 0) {
+            return newval;
+        }
+        if (newval.equals(value))
+            return value;
+        if (log.isDebugEnabled())
+            log.debug("IntrospectionUtils.replaceProperties iter on: " + newval);
+        return replaceProperties(newval, staticProp, dynamicProp, classLoader, iterationCount+1);

Review comment:
       I would stay with the recursion here, as it is doesn't seem to be time critical to replace those placeholders and I find it easier to read with the recursion.




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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] martin-g commented on pull request #309: Allow recursive substitution of properties.

GitBox
In reply to this post by GitBox

martin-g commented on pull request #309:
URL: https://github.com/apache/tomcat/pull/309#issuecomment-651073064


   New test case(s) in https://github.com/apache/tomcat/blob/master/test/org/apache/tomcat/util/TestIntrospectionUtils.java would be good to be added!


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] jfclere commented on pull request #309: Allow recursive substitution of properties.

GitBox
In reply to this post by GitBox

jfclere commented on pull request #309:
URL: https://github.com/apache/tomcat/pull/309#issuecomment-654734467


   I will work again next week on the PR, thank for you patience.


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

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]