Skip to content

When raising LDAPBytesWarning, walk the stack to determine stacklevel #128

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 15, 2017
Merged

When raising LDAPBytesWarning, walk the stack to determine stacklevel #128

merged 2 commits into from
Dec 15, 2017

Conversation

encukou
Copy link
Member

@encukou encukou commented Dec 13, 2017 •
edited
Loading

This is a simplification of #120 (and #109).

Turns out extension functions have globals(), and putting the marker in the _ldap module as well means we can only walk the stack – no other changes necessary.
BTW I do like removing _bytesify_inputs! Let's do that in 3.1.

I did have some trouble in py2 tests, see the _normalize inner function.

@encukou
Copy link
Member Author

encukou commented Dec 13, 2017

@tiran, does this look OK?

tiran
tiran requested changes Dec 13, 2017
View reviewed changes
Lib/ldap/ldapobject.py Outdated
# and in the _ldap extension, based on a marker in globals().
stacklevel = 0
try:
getframe = frame = sys._getframe
Copy link
Member

Choose a reason for hiding this comment

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

Why assign to frame?

Lib/ldap/ldapobject.py Outdated
# getting a useful stacklevel is tricky.
# We walk stack frames, ignoring all functions in this file
# and in the _ldap extension, based on a marker in globals().
stacklevel = 0
Copy link
Member

Choose a reason for hiding this comment

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

You can start with an offset of 2, maybe 3.

@tiran tiran mentioned this pull request Dec 13, 2017
Closed
@codecov
Copy link

codecov bot commented Dec 13, 2017 •
edited
Loading

Codecov Report

Merging #128 into master will decrease coverage by <.01%.
The diff coverage is 76.47%.

@@            Coverage Diff             @@
##           master     #128      +/-   ##
==========================================
- Coverage   68.92%   68.92%   -0.01%     
==========================================
  Files          49       49              
  Lines        4686     4701      +15     
  Branches      788      790       +2     
==========================================
+ Hits         3230     3240      +10     
- Misses       1108     1111       +3     
- Partials      348      350       +2
Impacted Files Coverage Δ
Modules/ldapmodule.c 64% <0%> (-5.57%) ⬇️
Lib/ldap/functions.py 76.08% <100%> (+0.53%) ⬆️
Lib/ldap/ldapobject.py 67.42% <85.71%> (+0.61%) ⬆️
Lib/slapdtest/_slapdtest.py 82.53% <0%> (-0.8%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf24a54...db98910. Read the comment docs.

@encukou
Copy link
Member Author

encukou commented Dec 13, 2017

Thanks, will fix.
The same thing needs to be done for the warning in __init__; ReconnectLDAPObject calls its superclass there.
I'm testing an update.

@encukou
Copy link
Member Author

encukou commented Dec 13, 2017

this is slowly leaving "simplification" territory...

tiran
tiran requested changes Dec 13, 2017
View reviewed changes
Lib/ldap/ldapobject.py Outdated
else:
frame = sys._getframe(stacklevel)
# walk up the stacks until we leave the file
while frame and frame.f_globals.get('_LDAP_WARN_SKIP_FRAME'):
Copy link
Member

Choose a reason for hiding this comment

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

frame is not defined in case sys._getframe does not exist. Can we rather wrap the whole block into a hasattr(sys, '_getframe') check?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, thanks for the catch!

tiran and others added 2 commits December 13, 2017 16:16
@encukou encukou closed this Dec 13, 2017
@encukou encukou reopened this Dec 13, 2017
@encukou
Copy link
Member Author

encukou commented Dec 13, 2017

I'll be away until Friday, and I'd like to release beta 3 on Friday or Monday.
@tiran, feel free to take over if there's anything more to be done here.

@tiran
Copy link
Member

tiran commented Dec 15, 2017 •
edited
Loading

The latest version of the PR addresses the stacklevel issue. I'm not quite happy with warnings in general. There are two potential issues:

  1. It's still hard to pin point bytes issues. The warning message does not include a hint, which argument is wrong. We may want to consider to include the name of the argument with invalid type (e.g. dn) in the warning message. python-ldap must not include the actual value, because it may contain confidential information like credentials.
  2. I suspect (without profiling) that warnings are a performance regression for add* and search*. All add op take a modlist, all search ops take an optional attrlist. The lists can be pretty long and we do the full warning dance for every entry. We should only warn once per operation.

Both issues can be addressed in new PRs.

@encukou encukou merged commit 4bc1f58 into python-ldap:master Dec 15, 2017
@encukou encukou mentioned this pull request Dec 15, 2017
Closed
This was referenced Dec 15, 2017
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiran tiran tiran requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@encukou @tiran