[GitHub] [pulsar] eolivelli commented on a change in pull request #8454: [pulsar-broker] Security-Recommendation: Remove verbose error message with system info for admin-api

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

eolivelli commented on a change in pull request #8454:
URL: https://github.com/apache/pulsar/pull/8454#discussion_r518574819



##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest2.java
##########
@@ -1338,6 +1338,16 @@ public void testMaxNamespacesPerTenant() throws Exception {
 
     }
 
+    @Test
+    public void testInvalidBundleErrorResponse() throws Exception {
+        try {
+            admin.namespaces().deleteNamespaceBundle("prop-xyz/ns1", "invalid-bundle");
+            fail("should have failed due to invalid bundle");
+        } catch (PreconditionFailedException e) {
+            assertTrue(e.getMessage().startsWith("Invalid bundle range"));

Review comment:
       thank you for adding the test @rdhabalia 
   
   I asked for a test, because IIUC the issue here is that we want a new message for security reasons, 
   messages in exceptions sometimes change due to refactors and it is probable that in the future someone will change the error message if there is no test case.
   
   the implementation is that we are now referring to the bundleRange in the message
   IMHO the test should test that actually the message contains that value.
   
   Also I would add here a comment that states that it is better to not change the message for security reasons (with a minimal explanation, not just "for security reasons")
   
   




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