Opened 3 years ago

Last modified 3 years ago

#8368 task new

Deprecate twisted.cred.credentials.UsernameHashedPassword

Reported by: mesozoic Owned by: Craig Rodrigues
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: deprecated-usernamehashedpassword-8368
branch-diff, diff-cov, branch-cov, buildbot
Author:

Description

See #3648 for historical context on why we're deprecating this class.

Change History (15)

comment:1 Changed 3 years ago by mesozoic

Branch: deprecated-usernamehashedpassword-8368
Keywords: review added
Owner: mesozoic deleted

Responding to feedback from ticket:3648#comment:22 so that I can finally get this off my plate and pick up something interesting.

15.2.0 was already released ... the branch should be updated to 15.3.0

Updated to 16.3.0 :)

From what I can see, the latest proposed changes does not fix the bug but just deprecate the code. I think that you should either update the ticket description/title or create a new ticket dedicated to deprecation... and close this ticket as wont fix.

Here we are!

The deprecation code needs a dedicated new test. Check current test suite for test named 'test_deprecation*`

Not sure I understand the value here. We use callDeprecated in two separate locations; seems like good enough coverage for the fact that it's deprecated. At the point where those two tests get removed, the class is probably getting removed.

Maybe the news file should be updated to "twisted.cred.credentials.UsernameHashedPassword is now deprecated."

You got it.

https://github.com/twisted/twisted/pull/84

comment:2 Changed 3 years ago by mesozoic

Summary: Deprecated twisted.cred.credentials.UsernameHashedPasswordDeprecate twisted.cred.credentials.UsernameHashedPassword

comment:3 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to mesozoic

Hi,

Thanks for the branch.


1.Please see our deprecation policy for information about deprecating a class

http://twistedmatrix.com/documents/current/core/development/policy/compatibility-policy.html#classes

The standard method for deprecating a class is to raise a warning at import time and not each time a class is instantiated.


  1. Also, check http://twistedmatrix.com/documents/current/core/development/policy/compatibility-policy.html#testing-deprecation-code for information about testing the deprecations.

using callDeprecated is existing tests is fine, but we need a new test using flushWarnings to make sure the warning is raised at the right level.

callDeprecated will work even if the warning is raised from a different level


I am aware that all the flushWarnings test dance is tedious, but this is the current standard.

If you want to this changed, please raise this issue over the Twisted mailing list.

I can tell that the dedicated flushWarning tests were very useful in the past as callDeprecated is sloppy and the deprecation helpers were not well tested or people were manually using warnings.warn() with a wrong stack level.


Please check my comments and resubmit.

Thanks!

comment:4 Changed 3 years ago by mesozoic

Keywords: review added
Owner: mesozoic deleted

Cool, that all makes sense. Thanks for clarifying. I've updated the pull request and kicked off another buildbot.

comment:5 Changed 3 years ago by Adi Roiban

Thanks.

Changes look good.

I think that we need to improve the documentation in the 'testing deprecation code' to make it clear that no warning should be left behind.

The updates for callDeprecated were ok as the test suite should not leave any deprecation code unhandled.

I am not sure why the builders are not reporting these warnings as I see that credentials.UsernameHashedPassword is used in other tests and the warnings are not handled.

comment:6 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to mesozoic

The last part of the Compatibility/deprecation guide contain info about checking for deprecation warnings

I did

$ python -Werror::DeprecationWarning ./bin/trial twisted/cred/test/test_cred.py

and it failed.

Please look at updating your branch so that it produce no new warnings.

In the process feel free to update the deprecation documentation based on your current experience.

comment:7 Changed 3 years ago by mesozoic

Keywords: review added
Owner: mesozoic deleted

OK, I've looked at this, and I think the best thing to do is nuke the test cases that reference this class. callDeprecated doesn't work with a class that's a deprecated module attribute; the warning gets raised when our test code references the class, not when calling it. I suspect that may be why the new warnings weren't caught.

I've updated the pull request with more deleted code and kicked off another Buildbot run.

comment:8 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to mesozoic

Can you please also update the docs/core/development/policy/compatibility-policy.rst file to provide some details about how deprecated classes should be tests?

I would suggest to also send an email over the ML to let other developers know about this method.


In general I am not very happy with removing tests. We might want to deprecate something ... but then during the 1 year deprecation time, we might find security bugs for the deprecated code and we might want to fix them.

Also, during that 1 year of deprecation time, the code is still supported and we should make sure that we don't introduce regressions.


I would say that we should find and document a better way.

What do you say if:

  • all normal/existing imports are done using something like callDeprecated
  • the deprecation test will use reload() py2 / imporlib.reload() py3 ... t.python.compat.reload() to trigger the warning

Please check my comments and resubmit for review with updated documentation and examples :)

Thanks!

comment:9 Changed 3 years ago by Adi Roiban

As long as you are importing just the module, the warning is not raised.

I have created #8478 to update the documentation.


Please revert the removed tests.

You can use an import statement like

# This is imported in this way to work arround the UsernameHashedPassword
# deprecation warning.
from twisted.cred import credentials
from twisted.cred.credentials import UsernamePassword, IUsernamePassword
from twisted.cred.credentials import IUsernameHashedPassword

Why not also deprecate IUsernameHashedPassword ?

comment:10 Changed 3 years ago by mesozoic

OK, I've added a new getDeprecatedModuleAttribute helper method for TestCase, which will retrieve a module attribute that's been marked for deprecation without triggering the deprecation warning. This allows us to refer to this class in a couple tests without having to do anything funky to suppress import-time warnings.

I chose not to deprecate IUsernameHashedPassword because it is used in a number of other places _besides_ UsernameHashedPassword (e.g. DigestedCredentials) and I thought it best to keep the scope of this branch small. Since interfaces get used at the top of a module, I'm not sure how we'd suppress the DeprecationWarnings generated by accessing them.

I'll submit for review as soon as the tests pass.

comment:11 Changed 3 years ago by mesozoic

Keywords: review added
Owner: mesozoic deleted

comment:12 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to mesozoic

Thanks for the changes. They look good.

Major comments.

  1. In getDeprecatedModuleAttribute the branch for if len(warningsShown) == 0: is not covered
  1. getDeprecatedModuleAttribute is a public API and we should have a twisted/topfiles/8368.feature advertising it. ... trial is in the public API realm
  1. Where are the test for getDeprecatedModuleAttribute ? Please consider adding dedicated tests. I think that we are recommending using flushWarnings for the actual deprecation tests since the deprecation helpers have little tests and no way to tell if the are correct or not.
  1. We should also update this docstring https://github.com/twisted/twisted/blob/trunk/twisted/python/deprecate.py#L20

Minor comments:

  1. test_deprecationUsingFlushWarnings docstring should be updated to reflect that it is no longer the recommended method.
  1. For getDeprecatedModuleAttribute instead of message maybe we can just have replacement and only the pass replacenet name... as the other string should be standard.
  1. I would argue that getDeprecatedModuleAttribute can be merged in #8478 and once that branch is merged in trunk, we should came back at this ticket.
  1. For IUsernameHashedPassword deprecation, I think that your comment from the Trac ticket would be a good in code comment to describe why are we stuck with that interface.
  1. I would argue that we should remove the extensive docstring documentation from https://github.com/twisted/twisted/blob/trunk/twisted/python/deprecate.py and make sure all missing information is in the .rst docs

Please check my comments and resubmit.

Thanks!

comment:13 Changed 3 years ago by Jean-Paul Calderone

Keywords: review added

comment:14 Changed 3 years ago by Jean-Paul Calderone

Owner: mesozoic deleted

comment:15 Changed 3 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Craig Rodrigues
Note: See TracTickets for help on using tickets.