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:
[email protected]
