[pulsar] branch master updated: [improve][broker] Store nonBlank clientVersions that have spaces (#19616)

[mmarshall]

This is an automated email from the ASF dual-hosted git repository.

mmarshall pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pulsar.git


The following commit(s) were added to refs/heads/master by this push:
     new 9e9ee02fe1d [improve][broker] Store nonBlank clientVersions that have 
spaces (#19616)
9e9ee02fe1d is described below

commit 9e9ee02fe1d0e9434b5510c555a5906c9b427bdd
Author: Michael Marshall <mmarsh...@apache.org>
AuthorDate: Fri Feb 24 11:30:47 2023 -0600

    [improve][broker] Store nonBlank clientVersions that have spaces (#19616)
    
    Relates to: https://github.com/apache/pulsar/pull/19540
    
    ### Motivation
    
    We currently filter out the `clientVersion` when it has a `" "` in it. As a 
consequence, we filter out the go client's version because of how it is made:
    
    
https://github.com/apache/pulsar-client-go/blob/dedbdc45c63b06e6a12356785418cd906d6bab3c/pulsar/internal/version.go#L43
    
    Given that we do not have any documented restrictions on the 
`clientVersion`, I think we should not drop clients that have a space in their 
name.
    
    I propose that we remove the filter logic to store all "valid" versions 
supplied by the client.
    
    ### Modifications
    
    * Update `ServerCnx` to store `clientVersion` when it has a space in its 
name.
    
    ### Verifying this change
    
    A new test is added.
    
    ### Does this pull request potentially affect one of the following parts:
    
    This is a backwards compatible change.
    
    ### Documentation
    
    - [x] `doc-not-needed`
    
    ### Matching PR in forked repository
    
    PR in forked repository: skipping for this minor change that shouldn't make 
any tests fail
---
 .../apache/pulsar/broker/service/ServerCnx.java    |  2 +-
 .../pulsar/broker/service/ServerCnxTest.java       | 29 ++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git 
a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java 
b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
index 242947f6a0f..2d6bed3eb7e 100644
--- 
a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
+++ 
b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
@@ -690,7 +690,7 @@ public class ServerCnx extends PulsarHandler implements 
TransportCnx {
             log.debug("[{}] connect state change to : [{}]", remoteAddress, 
State.Connected.name());
         }
         setRemoteEndpointProtocolVersion(clientProtoVersion);
-        if (isNotBlank(clientVersion) && !clientVersion.contains(" ") /* 
ignore default version: pulsar client */) {
+        if (isNotBlank(clientVersion)) {
             this.clientVersion = clientVersion.intern();
         }
         if (brokerInterceptor != null) {
diff --git 
a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java
 
b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java
index a29d6dac720..4580f028de2 100644
--- 
a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java
+++ 
b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java
@@ -143,6 +143,7 @@ import org.mockito.Mockito;
 import org.mockito.stubbing.Answer;
 import org.testng.annotations.AfterMethod;
 import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
 @SuppressWarnings("unchecked")
@@ -293,6 +294,34 @@ public class ServerCnxTest {
         channel.finish();
     }
 
+    @DataProvider(name = "clientVersions")
+    public Object[][] clientVersions() {
+        return new Object[][]{
+                {"Pulsar Client", true},
+                {"Pulsar Go 0.2.1", true},
+                {"Pulsar-Client-Java-v1.15.2", true},
+                {"pulsar-java-3.0.0", true},
+                {"", false},
+                {" ", false}
+        };
+    }
+
+    @Test(dataProvider = "clientVersions")
+    public void testStoreClientVersionWhenNotBlank(String clientVersion, 
boolean expectSetToClientVersion) throws Exception {
+        resetChannel();
+        assertTrue(channel.isActive());
+        assertEquals(serverCnx.getState(), State.Start);
+
+        ByteBuf clientCommand = Commands.newConnect("", "", clientVersion);
+        channel.writeInbound(clientCommand);
+
+        assertEquals(serverCnx.getState(), State.Connected);
+        assertTrue(getResponse() instanceof CommandConnected);
+
+        assertEquals(serverCnx.getClientVersion(), expectSetToClientVersion ? 
clientVersion : null);
+        channel.finish();
+    }
+
     @Test(timeOut = 30000)
     public void testKeepAlive() throws Exception {
         resetChannel();