Skip to content

Warning and error handling in _bytesify_input is too inflexible #166

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
Closed
tiran opened this issue Jan 12, 2018 · 9 comments
Closed

Warning and error handling in _bytesify_input is too inflexible #166

tiran opened this issue Jan 12, 2018 · 9 comments
Milestone
3.0

Comments

@tiran
Copy link
Member

tiran commented Jan 12, 2018

The way how _bytesify_input handles errors and warnings is too inflexible:

python-ldap/Lib/ldap/ldapobject.py

Lines 125 to 158 in ad46c11

def _bytesify_input(self, arg_name, value):
"""Adapt a value following bytes_mode in Python 2.
In Python 3, returns the original value unmodified.
With bytes_mode ON, takes bytes or None and returns bytes or None.
With bytes_mode OFF, takes unicode or None and returns bytes or None.
This function should be applied on all text inputs (distinguished names
and attribute names in modlists) to convert them to the bytes expected
by the C bindings.
"""
if not PY2:
return value
if value is None:
return value
elif self.bytes_mode:
if isinstance(value, bytes):
return value
else:
if self.bytes_mode_hardfail:
raise TypeError("All provided fields *must* be bytes when bytes mode is on; got %r" % (value,))
else:
_raise_byteswarning(
"Received non-bytes value for '{}' with default (disabled) bytes mode; "
"please choose an explicit "
"option for bytes_mode on your LDAP connection".format(arg_name))
return value.encode('utf-8')
else:
if not isinstance(value, text_type):
raise TypeError("All provided fields *must* be text when bytes mode is off; got %r" % (value,))
assert not isinstance(value, bytes)
return value.encode('utf-8')

  • For bytes_mode=None (default), the function emits a bytes warning when the input is text, then encodes it as UTF-8 bytes. Warnings are slow and can cause performance regressions (see Possible performance regression with LDAPBytesWarning #133). The warnings are always emitted, even when warnings are disabled and not shown to the user. The warnings module does not allow early opt-out.
  • For bytes_mode=False (strictly future-compatible) there is no warnings mode at all. When the function is called with bytes as input, it always fails with TypeError. TypeErrors make it really, really hard to port an existing application to future compatible API. There should be a mode that emits just warnings.

Proposal

I suggest that we introduce a bytes_warning flag:

  • bytes_warning=BYTES_WARNINGS_SILENT emits no warnings and doesn't raise TypeError for bytes_mode None, True, and False.
  • bytes_warning=BYTES_WARNINGS_WARN emits a warning when input is text for bytes_mode=None / bytes_mode=True or input is bytes for bytes_mode=False.
  • bytes_warning=BYTES_WARNINGS_ERROR raises a TypeError when input is text for bytes_mode=None / bytes_mode=True or input is bytes for bytes_mode=False.
  • bytes_warning=None (default) depends on bytes_mode:
    • bytes_mode=None -> BYTES_WARNINGS_WARN
    • bytes_mode=True -> BYTES_WARNINGS_ERROR
    • bytes_mode=False -> BYTES_WARNINGS_ERROR

The flags are defined as:

BYTES_WARNINGS_SILENT = 0
BYTES_WARNINGS_WARN = 1
BYTES_WARNINGS_ERROR = 2
@encukou
Copy link
Member

encukou commented Jan 12, 2018

The proposal looks reaonable. I hope to fully get to it next week. (initial nitpick: let's use bytes_warning='warn' instead of numbers.)

I'm also thinking about doing this as 3.1. I feel the betas aren't useful any more; AFAIK there's only you trying them out.

@tiran
Copy link
Member Author

tiran commented Jan 15, 2018

Do you want to address the issue for 3.0 or shall we get 3.0.0 out now and implement this for 3.1?

I'm fine with getting 3.0 out now. But we should make bytes_mode_hardfail attribute a private attribute before 3.0 gets out.

encukou added a commit to encukou/python-ldap that referenced this issue Jan 15, 2018
@encukou
Add bytes_mode='warn' and bytes_mode='silent' options
d984f34 
Fixes: python-ldap#166
@encukou
Copy link
Member

encukou commented Jan 15, 2018 •
edited
Loading

I played around with the idea.
How about making bytes_mode itself hold the, well, mode? That is, add bytes_mode == 'warn' and bytes_mode == 'warn' options?

See the d984f34 for a (test-less) implementation.

@tiran
Copy link
Member Author

tiran commented Jan 15, 2018

How do you plan to encode bytes_mode=False with warning?

@encukou
Copy link
Member

encukou commented Jan 15, 2018

Argh. I misread the most important part of the issue, didn't I.

About deprecating bytes_mode_hardfail -- it's been part of pyldap for a while. It's even gets pickled. Let's remove it in 3.0.
Only renaming it isn't worth the churn. Let's put this in 3.0. Can you prepare a PR?

@tiran
Copy link
Member Author

tiran commented Jan 15, 2018

Well, you skipped the most important part for my use case. :)

What do you want to use instead of bytes_mode_hardfail?

@encukou
Copy link
Member

encukou commented Jan 15, 2018 via email

@tiran
Copy link
Member Author

tiran commented Jan 16, 2018

It's very unlikely that I will have time to develop and test a patch before Feb 6th. I'm busy with DevConf CZ preparations and will have a week of meetings and travel after DevConf.

Proposition

  • Make bytes_mode_hardfail attribute private since we will probably not support in 3.1
  • Release 3.0.0 now
  • Roll out 3.1.0 mid February.

@peterpakos peterpakos mentioned this issue Jan 16, 2018
Closed
@encukou
Copy link
Member

encukou commented Jan 18, 2018

Renaming bytes_mode_hardfail needs some care as well. I'll work on implementing your proposal instead.

@encukou encukou added this to the 3.0 milestone Jan 22, 2018
encukou added a commit to encukou/python-ldap that referenced this issue Jan 22, 2018
@encukou
Add bytes_strictness to allow configuring behavior on bytes/text mism…
53d6bb5 
…atch

Fixes: python-ldap#166
encukou added a commit to encukou/python-ldap that referenced this issue Jan 22, 2018
@encukou
Add bytes_strictness to allow configuring behavior on bytes/text mism…
5ce6e74 
…atch

Fixes: python-ldap#166
@encukou encukou mentioned this issue Jan 22, 2018
Merged
@encukou encukou closed this as completed in 1b17e85 Mar 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.0
Development

No branches or pull requests
2 participants
@encukou @tiran