You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2021/03/01 19:23:41 UTC
[GitHub] [logging-log4j2] vy opened a new pull request #471: LOG4J2-2948 Replace HashSet with IdentityHashMap in ParameterFormatter to detect cycles.
vy opened a new pull request #471:
URL: https://github.com/apache/logging-log4j2/pull/471
----------------------------------------------------------------
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
[GitHub] [logging-log4j2] vy commented on a change in pull request #471: LOG4J2-2948 Replace HashSet with IdentityHashMap in ParameterFormatter to detect cycles.
Posted by GitBox <gi...@apache.org>.
vy commented on a change in pull request #471:
URL: https://github.com/apache/logging-log4j2/pull/471#discussion_r585384100
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterFormatter.java
##########
@@ -538,24 +536,28 @@ private static void appendArray(final Object o, final StringBuilder str, Set<Str
} else {
str.append(", ");
}
- recursiveDeepToString(current, str, new HashSet<>(dejaVu));
+ recursiveDeepToString(current, str, effectiveDejaVu);
Review comment:
Good catch! Putting the cloning back with a unit test.
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterFormatter.java
##########
@@ -448,7 +450,7 @@ static String deepToString(final Object o) {
* @param str the StringBuilder that o will be appended to
* @param dejaVu a list of container identities that were already used.
Review comment:
:+1: Will do.
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterFormatter.java
##########
@@ -448,7 +450,7 @@ static String deepToString(final Object o) {
* @param str the StringBuilder that o will be appended to
* @param dejaVu a list of container identities that were already used.
*/
- static void recursiveDeepToString(final Object o, final StringBuilder str, final Set<String> dejaVu) {
+ static void recursiveDeepToString(final Object o, final StringBuilder str, final Set<Object> dejaVu) {
Review comment:
Makes sense, implemented.
----------------------------------------------------------------
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
[GitHub] [logging-log4j2] vy commented on pull request #471: LOG4J2-2948 Replace HashSet with IdentityHashMap in ParameterFormatter to detect cycles.
Posted by GitBox <gi...@apache.org>.
vy commented on pull request #471:
URL: https://github.com/apache/logging-log4j2/pull/471#issuecomment-788765792
@carterkozak, would you mind skimming through the changes one last time, please? If you approve, I am gonna merge this into `release-2.x` and will get busy with porting it to `master`.
----------------------------------------------------------------
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
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #471: LOG4J2-2948 Replace HashSet with IdentityHashMap in ParameterFormatter to detect cycles.
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #471:
URL: https://github.com/apache/logging-log4j2/pull/471#discussion_r585008161
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterFormatter.java
##########
@@ -538,24 +536,28 @@ private static void appendArray(final Object o, final StringBuilder str, Set<Str
} else {
str.append(", ");
}
- recursiveDeepToString(current, str, new HashSet<>(dejaVu));
+ recursiveDeepToString(current, str, effectiveDejaVu);
Review comment:
By removing the `new HashSet` as we traverse the collection, I think we end up producing potentially confusing data. I think we may want to keep the collection creation despite the additional allocation cost.
We should add tests for this case ;-)
Consider `[Foo{A}, Foo{A}]`. It duplicates data, however it's not recursive. We only want to catch cycles here, not duplication, e.g. `A=[Foo{B}, A]` if that makes sense. What do you think?
edit: clearer example, hopefully
----------------------------------------------------------------
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
[GitHub] [logging-log4j2] vy merged pull request #471: LOG4J2-2948 Replace HashSet with IdentityHashMap in ParameterFormatter to detect cycles.
Posted by GitBox <gi...@apache.org>.
vy merged pull request #471:
URL: https://github.com/apache/logging-log4j2/pull/471
----------------------------------------------------------------
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
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #471: LOG4J2-2948 Replace HashSet with IdentityHashMap in ParameterFormatter to detect cycles.
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #471:
URL: https://github.com/apache/logging-log4j2/pull/471#discussion_r585530427
##########
File path: log4j-api/src/test/java/org/apache/logging/log4j/message/ParameterFormatterTest.java
##########
@@ -180,13 +182,29 @@ public void testDeepToString() throws Exception {
}
@Test
- public void testIdentityToString() throws Exception {
+ public void testDeepToStringUsingNonRecursiveButConsequentObjects() {
final List<Object> list = new ArrayList<>();
+ final Object item = Collections.singletonList(0);
list.add(1);
+ list.add(item);
+ list.add(2);
+ list.add(item);
+ list.add(3);
+ final String actual = ParameterFormatter.deepToString(list);
+ final String expected = "[1, [0], 2, [0], 3]";
+ assertEquals(expected, actual);
+ }
Review comment:
Thanks!
----------------------------------------------------------------
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
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #471: LOG4J2-2948 Replace HashSet with IdentityHashMap in ParameterFormatter to detect cycles.
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #471:
URL: https://github.com/apache/logging-log4j2/pull/471#discussion_r585008161
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterFormatter.java
##########
@@ -538,24 +536,28 @@ private static void appendArray(final Object o, final StringBuilder str, Set<Str
} else {
str.append(", ");
}
- recursiveDeepToString(current, str, new HashSet<>(dejaVu));
+ recursiveDeepToString(current, str, effectiveDejaVu);
Review comment:
By removing the `new HashSet` as we traverse the collection, I think we end up producing potentially confusing data. I think we may want to keep the collection creation despite the additional allocation cost.
We should add tests for this case ;-)
Consider `[Foo{A}, Foo{A}]`. It duplicates data, however it's not recursive. We only want to catch cycles here, not duplication, e.g. `A=[Foo{delegate=A}]` if that makes sense. What do you think?
----------------------------------------------------------------
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
[GitHub] [logging-log4j2] Marcono1234 commented on a change in pull request #471: LOG4J2-2948 Replace HashSet with IdentityHashMap in ParameterFormatter to detect cycles.
Posted by GitBox <gi...@apache.org>.
Marcono1234 commented on a change in pull request #471:
URL: https://github.com/apache/logging-log4j2/pull/471#discussion_r585119540
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterFormatter.java
##########
@@ -448,7 +450,7 @@ static String deepToString(final Object o) {
* @param str the StringBuilder that o will be appended to
* @param dejaVu a list of container identities that were already used.
*/
- static void recursiveDeepToString(final Object o, final StringBuilder str, final Set<String> dejaVu) {
+ static void recursiveDeepToString(final Object o, final StringBuilder str, final Set<Object> dejaVu) {
Review comment:
Would it make sense to make this method private and instead add a package-private overload `recursiveDeepToString(Object, StringBuilder)`?
Most of the calls to this method have `null` as dejaVu, and especially the calls from `MapMessage` should not have to care about how recursion tracking is implemented.
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterFormatter.java
##########
@@ -448,7 +450,7 @@ static String deepToString(final Object o) {
* @param str the StringBuilder that o will be appended to
* @param dejaVu a list of container identities that were already used.
Review comment:
```suggestion
* @param dejaVu a set of container objects directly or transitively containing {@code o}
```
----------------------------------------------------------------
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
[GitHub] [logging-log4j2] vy commented on pull request #471: LOG4J2-2948 Replace HashSet with IdentityHashMap in ParameterFormatter to detect cycles.
Posted by GitBox <gi...@apache.org>.
vy commented on pull request #471:
URL: https://github.com/apache/logging-log4j2/pull/471#issuecomment-788212187
@rgoers, would you mind reviewing the changes, please? Note that I don't create a new `HashSet` for each internal call, which has used to be the case. According to tests, this should be fine.
----------------------------------------------------------------
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