[GitHub] [druid] paul-rogers commented on a diff in pull request #13045: fix bug in /status/properties filtering

[GitBox <gi...@apache.org>]

paul-rogers commented on code in PR #13045:
URL: https://github.com/apache/druid/pull/13045#discussion_r965319656


##########
server/src/main/java/org/apache/druid/server/StatusResource.java:
##########
@@ -92,15 +96,15 @@ private Map<String, String> filterHiddenProperties(
       Map<String, String> allProperties
   )
   {
-    return Maps.filterEntries(
-        allProperties,
-        (entry) -> hiddenProperties
-            .stream()
-            .anyMatch(
-                hiddenPropertyElement ->
-                    !StringUtils.toLowerCase(entry.getKey()).contains(StringUtils.toLowerCase(hiddenPropertyElement))
-            )
+    Map<String, String> propertyCopy = new HashMap<>(allProperties);

Review Comment:
   When this code is nested inside a resource, it is hard to test. Should we pull this out as a static method so it can be tested easily with various sets of properties and hidden property lists?



##########
server/src/main/java/org/apache/druid/server/StatusResource.java:
##########
@@ -92,15 +96,15 @@ private Map<String, String> filterHiddenProperties(
       Map<String, String> allProperties
   )
   {
-    return Maps.filterEntries(
-        allProperties,
-        (entry) -> hiddenProperties
-            .stream()
-            .anyMatch(
-                hiddenPropertyElement ->
-                    !StringUtils.toLowerCase(entry.getKey()).contains(StringUtils.toLowerCase(hiddenPropertyElement))
-            )
+    Map<String, String> propertyCopy = new HashMap<>(allProperties);
+    allProperties.keySet().forEach(

Review Comment:
   The set of hidden properties is likely to be much smaller than the set of all properties. Should we iterate over the hidden properties so we have a much lower total cost? One could argue that this operation is used infrequently and efficiency doesn't matter. But, since we're changing the code anyway, we might as well make it simple.



##########
server/src/main/java/org/apache/druid/server/StatusResource.java:
##########
@@ -92,15 +96,15 @@ private Map<String, String> filterHiddenProperties(
       Map<String, String> allProperties
   )
   {
-    return Maps.filterEntries(
-        allProperties,
-        (entry) -> hiddenProperties
-            .stream()
-            .anyMatch(
-                hiddenPropertyElement ->
-                    !StringUtils.toLowerCase(entry.getKey()).contains(StringUtils.toLowerCase(hiddenPropertyElement))
-            )
+    Map<String, String> propertyCopy = new HashMap<>(allProperties);
+    allProperties.keySet().forEach(
+        (key) -> {
+          if (hiddenProperties.stream().anyMatch((hiddenProperty) -> StringUtils.toLowerCase(key).contains(StringUtils.toLowerCase(hiddenProperty)))) {
+            propertyCopy.remove(key);

Review Comment:
   Should we convert the value to, say, "*****" so that at least the user knows that the property was set to something?



-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org