Skip to content

[cleanup][broker] Pass subName to authz provider on unsubscribe - #19176

Merged
michaeljmarshall merged 1 commit into
apache:masterfrom
michaeljmarshall:fix-missing-subscription-name
Jan 11, 2023
Merged

[cleanup][broker] Pass subName to authz provider on unsubscribe#19176
michaeljmarshall merged 1 commit into
apache:masterfrom
michaeljmarshall:fix-missing-subscription-name

Conversation

@michaeljmarshall

Copy link
Copy Markdown
Member

Motivation

In #15347, there is a regression where the subName needed to be passed to the authorization provider. That PR is not released, so this PR is just cleanup

Modifications

  • Pass subName for validateTopicOperationAsync in PersistentTopicsBase#internalDeleteSubscriptionForNonPartitionedTopicAsync

Verifying this change

We should add testing to this part of the code base to prevent future regressions.

Does this pull request potentially affect one of the following parts:

This is not a breaking change.

Documentation

  • doc-not-needed

@michaeljmarshall michaeljmarshall added type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use doc-not-needed Your PR changes do not impact docs area/authz labels Jan 10, 2023
@michaeljmarshall michaeljmarshall added this to the 2.12.0 milestone Jan 10, 2023
@michaeljmarshall michaeljmarshall self-assigned this Jan 10, 2023
@codecov-commenter

codecov-commenter commented Jan 10, 2023

Copy link
Copy Markdown

Codecov Report

Merging #19176 (41d2c6f) into master (4028ad3) will increase coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19176      +/-   ##
============================================
+ Coverage     47.45%   47.57%   +0.11%     
+ Complexity    10760     9556    -1204     
============================================
  Files           713      633      -80     
  Lines         69672    59898    -9774     
  Branches       7482     6239    -1243     
============================================
- Hits          33063    28496    -4567     
+ Misses        32895    28286    -4609     
+ Partials       3714     3116     -598     
Flag Coverage Δ
unittests 47.57% <ø> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...pulsar/broker/admin/impl/PersistentTopicsBase.java 58.89% <ø> (-0.28%) ⬇️
.../apache/pulsar/broker/admin/impl/PackagesBase.java 54.12% <0.00%> (-13.77%) ⬇️
...r/service/AbstractDispatcherMultipleConsumers.java 58.06% <0.00%> (-12.91%) ⬇️
...g/apache/pulsar/client/impl/ConnectionHandler.java 50.00% <0.00%> (-5.32%) ⬇️
.../pulsar/broker/service/BrokerServiceException.java 47.27% <0.00%> (-4.55%) ⬇️
...istentStreamingDispatcherSingleActiveConsumer.java 50.51% <0.00%> (-3.10%) ⬇️
...oker/service/nonpersistent/NonPersistentTopic.java 38.93% <0.00%> (-3.09%) ⬇️
.../org/apache/pulsar/broker/admin/AdminResource.java 63.69% <0.00%> (-2.97%) ⬇️
...va/org/apache/pulsar/broker/service/ServerCnx.java 47.87% <0.00%> (-2.27%) ⬇️
.../org/apache/pulsar/broker/admin/v2/Namespaces.java 57.89% <0.00%> (-2.09%) ⬇️
... and 119 more

@mattisonchao mattisonchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add a test for this change. :)

@michaeljmarshall

Copy link
Copy Markdown
Member Author

@mattisonchao - I am working on a more general test to cover more cases. Before that, I am doing some cleanup of the authorization code base. I created an issue to improve the tests in a generic way: #19183.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/authz doc-not-needed Your PR changes do not impact docs ready-to-test type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants