shibd commented on code in PR #25304:
URL: https://github.com/apache/pulsar/pull/25304#discussion_r2923388518
##########
pulsar-websocket/src/main/java/org/apache/pulsar/websocket/AbstractWebSocketHandler.java:
##########
@@ -271,28 +263,15 @@ protected void extractTopicName(HttpServletRequest
request) {
checkArgument(parts.size() >= 8, "Invalid topic name format");
checkArgument(parts.get(1).equals("ws"));
+ checkArgument(parts.get(2).equals("v2"));
- final boolean isV2Format = parts.get(2).equals("v2");
- final int domainIndex = isV2Format ? 4 : 3;
- checkArgument(parts.get(domainIndex).equals("persistent")
- || parts.get(domainIndex).equals("non-persistent"));
+ checkArgument(parts.get(4).equals("persistent")
+ || parts.get(4).equals("non-persistent"));
- final String domain = parts.get(domainIndex);
- final NamespaceName namespace = isV2Format ?
NamespaceName.get(parts.get(5), parts.get(6)) :
- NamespaceName.get(parts.get(4), parts.get(5), parts.get(6));
+ final String domain = parts.get(4);
+ final NamespaceName namespace = NamespaceName.get(parts.get(5),
parts.get(6));
- // The topic name which contains slashes is also split, so it needs to
be jointed
- int startPosition = 7;
- boolean isConsumer = "consumer".equals(parts.get(2)) ||
"consumer".equals(parts.get(3));
- int endPosition = isConsumer ? parts.size() - 1 : parts.size();
- StringBuilder topicName = new StringBuilder(parts.get(startPosition));
- while (++startPosition < endPosition) {
- if (StringUtils.isEmpty(parts.get(startPosition))) {
- continue;
- }
- topicName.append("/").append(parts.get(startPosition));
- }
- final String name = Codec.decode(topicName.toString());
+ final String name = Codec.decode(parts.get(7));
Review Comment:
This now only decodes `parts.get(7)`, so a valid V2 WebSocket topic like
`/ws/v2/consumer/persistent/tenant/ns/some/topic/with/slashes/sub` gets
truncated to `some`. Even if we decide to reject `/` in local names, this
should fail explicitly rather than silently parsing the wrong topic.
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -133,40 +126,25 @@ private TopicName(String completeTopicName) {
}
}
- // The fully qualified topic name can be in two different forms:
- // new: persistent://tenant/namespace/topic
- // legacy: persistent://tenant/cluster/namespace/topic
-
+ // Expected format: persistent://tenant/namespace/topic
List<String> parts =
Splitter.on("://").limit(2).splitToList(completeTopicName);
this.domain = TopicDomain.getEnum(parts.get(0));
String rest = parts.get(1);
- // The rest of the name can be in different forms:
- // new: tenant/namespace/<localName>
- // legacy: tenant/cluster/namespace/<localName>
- // Examples of localName:
- // 1. some, name, xyz
- // 2. xyz-123, feeder-2
-
-
+ // Expected format: tenant/namespace/<localName>
parts = Splitter.on("/").limit(4).splitToList(rest);
- if (parts.size() == 3) {
- // New topic name without cluster name
+ if (parts.size() == 4) {
Review Comment:
This rejects any 4-segment V2 topic path, but Pulsar still allows `/` inside
the local topic name. For example, `persistent://tenant/ns/a/b` is a valid V2
topic today and should be parsed as tenant=`tenant`, namespace=`ns`,
localName=`a/b`, not rejected as a V1 name. Could we keep rejecting the real V1
form while still allowing slashes in the local name?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
