Opened 2 years ago

Closed 18 months ago

#5852 task closed fixed (fixed)

Deprecating tlsMode

Reported by: argonemyth Owned by: rwall
Priority: low Milestone:
Component: mail Keywords: smtp ESMTPClient tlsMode
Cc: bitsink@…, moijes12@…, richard@… Branch: branches/deprecate-tlsmode-5852
(diff, github, buildbot, log)
Author: moijes12, rwall Launchpad Bug:

Description

The class attribute tlsMode in twisted.mail.smtp.ESMTPClient is no longer an functional attribute - the usage of secure transport doesn't depend on the value anymore. It's time to remove it from the class.

Attachments (9)

5852.patch (1.6 KB) - added by moijes12 2 years ago.
Are you looking for something like this ?
5852.2.patch (2.5 KB) - added by moijes12 2 years ago.
5852.3.patch (2.3 KB) - added by moijes12 2 years ago.
5852.4.patch (3.2 KB) - added by moijes12 2 years ago.
5852.5.patch (3.4 KB) - added by moijes12 2 years ago.
5852.6.patch (3.4 KB) - added by moijes12 2 years ago.
5852.7.patch (3.4 KB) - added by lvh 20 months ago.
Same as last patch, but 12.3 -> 13.0
5852.8.patch (2.7 KB) - added by rwall 19 months ago.
5852.9.patch (4.3 KB) - added by rwall 18 months ago.
Now with a getattr method as suggested by tomprince

Download all attachments as: .zip

Change History (32)

Changed 2 years ago by moijes12

Are you looking for something like this ?

comment:1 Changed 2 years ago by moijes12

  • Keywords review added
  • Owner argonemyth deleted

comment:2 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to moijes12

Thanks for working on this.

This patch doesn't accomplish the desired result, which is to emit a deprecation when the tlsMode attribute is read or written. The goal is to warn developers with code that manipulates this attribute so they know the attribute no longer means anything. Introducing a new API that emits a deprecation warning doesn't accomplish the goal, since existing code won't use that API.

A property can be used to achieve the desired effect for when tlsMode is read. Unfortunately, since it's on a classic class, write properties don't work, so a __setattr__ hook may be necessary.

Changed 2 years ago by moijes12

comment:3 Changed 2 years ago by moijes12

  • Keywords review added
  • Owner moijes12 deleted

comment:4 Changed 2 years ago by namn

  • Cc bitsink@… added

Line 1246 in smtp.py should be reflowed to 79 columns.

Line 612 in test_smtp.py looks redundant. I think line 1529 in that same file is also not needed. Line 1542 in the same file should be reflowed.

I would replace is with has been in 5852.removal.

Changed 2 years ago by moijes12

comment:5 Changed 2 years ago by moijes12

  • Cc moijes12@… added

comment:6 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to moijes12

Thanks for your continuing work on this issue.

  1. warnings.catch_warnings is not how we unit test deprecations. Please stick to the convention established in other deprecation unit tests in Twisted.
  2. The implementation of __setattr__ breaks all attribute setting, completely.
  3. The news fragment can just say that the attribute is deprecated, the version information is not needed (the news fragment will appear in the news file under a section for a particular version of Twisted).

Changed 2 years ago by moijes12

comment:7 Changed 2 years ago by moijes12

  • Keywords review added
  • Owner moijes12 deleted

comment:8 Changed 2 years ago by moijes12

twisted.mail.smtp.ESMTPClient being a classic class, I couldn't use descriptors. So I had to use setattr hook. What I did notice was that the test messages were getting suppressed because of previous function definitions and therefore I had to reset the C{.suppress} . Atleast that is what my understanding of it is.

comment:9 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to moijes12

Thanks. A few issues remain:

  1. The new test asserts the wrong thing. Creating an ESMTPClient instance should not cause a deprecation warning, twisted.mail.smtp.ESMTPClient is deprecated since Twisted 12.2. ESMTPClient is not, in fact deprecated. Only the tlsMode attribute is deprecated. This means:
    1. The deprecation warning message needs to be changed
    2. The warning needs to be emitted only if the tlsMode attribute is set. Perhaps it is set as a side effect of instantiating ESMTPClient now, but if so that is a problem that needs to be addressed. Using non-deprecated Twisted APIs must not trigger deprecation warnings.
  2. 12.2 has been released. The next release will probably be 12.3, so that's the version tlsMode needs to be deprecated in.
  3. Pass an argument to the flushWarnings call. Otherwise you flush every single warning that has happened, and you avoid testing that the stacklevel argument passed to warnings.warn was given the correct value. The argument is a list of functions which could have triggered the warning. If the stacklevel argument has the wrong value, then the wrong function will be marked as triggering the warning and flushWarnings will tell you about this by not returning the warning, so your test can fail.

Thanks again.

Changed 2 years ago by moijes12

comment:10 Changed 2 years ago by moijes12

  • Keywords review added
  • Owner moijes12 deleted

tlsMode being deprecated, I've removed the its initialisation from the init method of ESMTPClient. The messages have been updated.

Changed 2 years ago by moijes12

comment:11 Changed 20 months ago by exarkun

This needs to happen after #3989.

comment:12 Changed 20 months ago by lvh

  • Keywords review removed
  • Owner set to lvh

The bug exarkun mentioned is now closed. The patch looks okay to me, apart from the obvious fact that 12.3 is released now, so it should be deprecated in 13.0. I'll fix that and resubmit for review.

Changed 20 months ago by lvh

Same as last patch, but 12.3 -> 13.0

comment:13 Changed 20 months ago by lvh

  • Keywords review added
  • Owner lvh deleted

New patch added, please review.

comment:14 Changed 19 months ago by rwall

  • Cc richard@… added
  • Keywords review removed
  • Owner set to rwall
  • Status changed from new to assigned

Code Review

  1. Patch didn't apply cleanly to trunk for me - probably since #3989.
    $ patch -p0 < ../5852.7.patch
    patching file twisted/mail/smtp.py
    Hunk #4 FAILED at 1247.
    1 out of 4 hunks FAILED -- saving rejects to file twisted/mail/smtp.py.rej
    patching file twisted/mail/test/test_smtp.py
    Hunk #1 FAILED at 6.
    Hunk #2 succeeded at 611 (offset 2 lines).
    Hunk #3 FAILED at 1518.
    2 out of 3 hunks FAILED -- saving rejects to file twisted/mail/test/test_smtp.py.rej
    patching file twisted/mail/topfiles/5852.removal
    
  2. There's some trailing Whitespace in the patch.
  3. There are still lines > 80 chars
  4. Remove the word "Test " from test method docstring - instead just "ESMTPClient.tlsMode is deprecated"
  5. This ticket title is quite misleading

I'll clean up the patch and resubmit.

Changed 19 months ago by rwall

comment:15 Changed 19 months ago by rwall

  • Keywords review added
  • Owner rwall deleted
  • Status changed from assigned to new

Ready for review in attachment:5852.8.patch

  1. Patch should now merge cleanly to trunk
  2. Re-flowed long lines
  3. Moved deprecation test to alongside existing ESMTPClient TLS tests.
  4. Removed unused warning module in test_smtp.py
  5. Removed the "suppress" attribute from the test method - it looked unnecessary.
  6. Added epytext ref to the test docstring.

-RichardW.

comment:16 follow-up: Changed 19 months ago by tom.prince

  • Keywords review removed
  • Owner set to rwall

This only emits a deprecation warning when writing to tlsMode, not when reading. Both should be handled. Otherwise, this looks good.

Changed 18 months ago by rwall

Now with a getattr method as suggested by tomprince

comment:17 in reply to: ↑ 16 Changed 18 months ago by rwall

  • Keywords review added
  • Owner rwall deleted

Replying to tom.prince:

This only emits a deprecation warning when writing to tlsMode, not when reading. Both should be handled. Otherwise, this looks good.

Ok.

  • I renamed the actual tlsMode value to _tlsMode
  • I added a getattr method and modified setattr to access the private var
  • ...and some new tests.

I didn't try and do anything about accessing or setting it as a class variable

See attachment:5852.9.patch

-RichardW.

comment:18 Changed 18 months ago by thijs

  • Owner set to thijs
  • Status changed from new to assigned

Putting this in a branch and see what the buildfarm thinks of it.

comment:19 Changed 18 months ago by thijs

  • Author set to thijs
  • Branch set to branches/deprecate-tlsmode-5852

(In [37187]) Branching to 'deprecate-tlsmode-5852'

comment:20 Changed 18 months ago by thijs

(In [37188]) Apply 5852.9.patch, refs #5852

comment:21 Changed 18 months ago by thijs

  • Author changed from thijs to moijes12, rwall
  • Owner thijs deleted
  • Status changed from assigned to new

Forced a build.

comment:22 Changed 18 months ago by tom.prince

  • Keywords review removed
  • Owner set to rwall

One minor point:

  • The AttributeError should use self.__class__.__name__ instead of hardcoding it.

Otherwise, this looks good. Thanks for your work on this.
Please commit after fixing that.

comment:23 Changed 18 months ago by rwall

  • Resolution set to fixed
  • Status changed from new to closed

(In [37202]) Merge deprecate-tlsmode-5852: tlsMode attribute of twisted.mail.smtp.ESMTPClient is deprecated.

Author: moijes12, rwall
Reviewer: exarkun, namn, lvh, rwall, tom.prince, thijs
Fixes: #5852

The tlsMode attribute of twisted.mail.smtp.ESMTPClient is deprecated.

Note: See TracTickets for help on using tickets.