[fix] [broker] No longer allow creating subscription that contains slash #23594
Conversation
|
LGTM. Do we need to support decoding URL-encoded subscription names in broker side? Thus users can use existing URL-encoded subscription names in HTTP requests. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Can you put it into a method to facilitate the extension of more subscription name restrictions in the future |
The subscription name should be URL-encoded. For example, to create a subscription I believe I appreciate the approach of restricting naming conventions for subscription and topic names. In my organization, subscription names are limited to the following characters: |
|
Related PR: #23620 |
There was a problem hiding this comment.
It looks like PR #23620 could be an effective way to prevent the auto creation of topics with slashes.
I think that we should consider implementing #23614 to validate subscription names instead of preventing the creation of subscription names with slashes.
As pointed out by @Shawyeok in #23594 (comment), if a slash is included in the subscription name, it should be url encoded. That's why this PR doesn't make sense to me and I'd prefer #23620 as the solution.
But should we prevent the creation if the result of the validated subscription name is Sent a discussion to do that |
@poorbarcode I think it makes sense to reference #23614 and #23620 in the discussion since those describe useful solutions. They are addressing 2 separate issues. |
6cf1439 to
f74a42e
Compare
|
Used the util method https://github.com/apache/pulsar/blob/master/pulsar-common/src/main/java/org/apache/pulsar/common/naming/NamedEntity.java to check new subscription name, please take a look |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #23594 +/- ##
============================================
+ Coverage 73.57% 74.24% +0.66%
- Complexity 32624 32685 +61
============================================
Files 1877 1868 -9
Lines 139502 145335 +5833
Branches 15299 16626 +1327
============================================
+ Hits 102638 107900 +5262
+ Misses 28908 28874 -34
- Partials 7956 8561 +605
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ash (apache#23594) (cherry picked from commit 7d54f40) (cherry picked from commit a27a375)
…ash (apache#23594) (cherry picked from commit 7d54f40) (cherry picked from commit c28b59a)
…ash (apache#23594) (cherry picked from commit 7d54f40) (cherry picked from commit c28b59a)
…ash (apache#23594) (cherry picked from commit 7d54f40) (cherry picked from commit a27a375)
…ash (apache#23594) (cherry picked from commit 7d54f40) (cherry picked from commit a27a375)
…ash (apache#23594) (cherry picked from commit 7d54f40)
…ash (apache#23594) (cherry picked from commit 7d54f40) (cherry picked from commit c28b59a) (cherry picked from commit af2de4f)
…ash (apache#23594) (cherry picked from commit 7d54f40)
…ash (apache#23594) (cherry picked from commit 7d54f40) (cherry picked from commit c28b59a) (cherry picked from commit af2de4f)
…ash (apache#23594) (cherry picked from commit 7d54f40)
Motivation
Topic name rule
{tenant}/{cluster}/{namespace}/{topic}/{subscription}{tenant}/{namespace}/{topic}/{subscription}There are many HTTP APIs defined as
HTTP Method {topic name}/{subscription name}If a subscription contains
/, the broker will assume it is a topic created with the old rule, then users will get acluster does not existerror or atopic not found errorModifications
Use the util method https://github.com/apache/pulsar/blob/master/pulsar-common/src/main/java/org/apache/pulsar/common/naming/NamedEntity.java to check new subscription name
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: x