You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by GitBox <gi...@apache.org> on 2020/06/26 07:54:10 UTC
[GitHub] [tomcat] jfclere opened a new pull request #309: Allow recursive substitution of properties.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] jfclere commented on a change in pull request #309: Allow recursive substitution of properties.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] FSchumacher commented on a change in pull request #309: Allow recursive substitution of properties.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on pull request #309: Allow recursive substitution of properties.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] jfclere commented on pull request #309: Allow recursive substitution of properties.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] martin-g commented on pull request #309: Allow recursive substitution of properties.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] FSchumacher commented on pull request #309: Allow recursive substitution of properties.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] jfclere commented on a change in pull request #309: Allow recursive substitution of properties.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] FSchumacher commented on pull request #309: Allow recursive substitution of properties.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] jfclere merged pull request #309: Allow recursive substitution of properties.
Posted by GitBox <gi...@apache.org>.
jfclere merged pull request #309:
URL: https://github.com/apache/tomcat/pull/309
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] ChristopherSchultz commented on a change in pull request #309: Allow recursive substitution of properties.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] FSchumacher commented on a change in pull request #309: Allow recursive substitution of properties.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] jfclere commented on pull request #309: Allow recursive substitution of properties.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] jfclere commented on pull request #309: Allow recursive substitution of properties.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org