Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] Fix NamedEntity::checkName regression #127

Merged
merged 1 commit into from
Nov 28, 2022

Conversation

erobot
Copy link
Contributor

@erobot erobot commented Nov 25, 2022

Motivation

Namspace name with underscore is valid before but now invalid. This regression caused by #52.

Broker validates NamedEntity using regex "^[-=:.\\w]*$", which actually includes the underscore. Although the comment saying alphanumeric (a-za-z_0-9) and these special characters -=: is a bit confusing. But we should not break the actual validation rules used a long time ago.

https://github.com/apache/pulsar/blob/355ba6002fc33b2716f59ca5d3a4fac667ecf3e1/pulsar-common/src/main/java/org/apache/pulsar/common/naming/NamedEntity.java#L30-L33

Modifications

Make underscore valid in NamedEntity.

Verifying this change

  • Make sure that the change passes the CI checks.

This change can covered by existing tests with some modifications, such as NamespaceNameTest.testNamespaceNameLegalCharacters and TopicNameTest.testLegalNonAlphaCharacters.

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    fix regression only

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@BewareMyPower BewareMyPower added this to the 3.1.0 milestone Nov 26, 2022
@BewareMyPower
Copy link
Contributor

_ should not be treated as a "special character". Even in C++ regex, "\w" also includes the understore. Since the 3.1.0 is during the release process, I hope we can include this fix. /cc @RobertIndie @merlimat

@BewareMyPower BewareMyPower merged commit 96d8705 into apache:main Nov 28, 2022
RobertIndie pushed a commit to RobertIndie/pulsar-client-cpp that referenced this pull request Nov 28, 2022
RobertIndie pushed a commit to streamnative/pulsar-client-cpp that referenced this pull request Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants