Python3 support

[Krejdom]

Hi, could you please support Python3? Many distributions are shifting to Python3 very soon and Python 2.7 will be supported only until 2020.

[psi29a]

I have a python3 branch for about a year. Every month or so I check to see if the required twisted component has been ported yet or not.

It is on the list. :)

[s0undt3ch]

@psi29a: I noticed your py3 branch hasn't had an update for about a year now. With twsited 16.4.0 being released stating 35+ modules ported to py3, by any chance, the one you've been looking for is in that list?

[psi29a]

I've been looking forward to that release a lot. When I have time, I'll give it a glance. I have a lot on my plate at the moment. If you would like to take my branch for a spin and test with 16.4, let us know! :)

[s0undt3ch]

I'll try it

[adiroiban]

Is there a list of Twisted code which needs to be ported to py3?

We can use that to prioritize the Twisted port... and if there is a list in this ticket maybe people wanting to see ldaptor on py3 can help first with those ports.

Thanks :)

[psi29a]

It's been awhile, I should give this a go again to see where we stand. I had a branch I was working on, but was blocked by 'srvconnect'.

My branch is here, 3 commits ahead of master.
https://github.com/psi29a/ldaptor/tree/python3

That was from Sep 14, 2015
I'm hoping that srvconnect has been updated by now?

[adiroiban]

The Twisted modules not ported to py3 are listed here https://github.com/twisted/twisted/blob/twisted-17.9.0/src/twisted/python/_setup.py#L366

I don't see twisted.names there so I guess that the whole DNS part was ported to py3

[psi29a]

super...you can take my branch and move forward if you want. I'll give it an eyeball over the weekend.

[tardyp]

FWIW, we are willing to switch buildbot to ldaptor from ldap3 if this lib is compatible with py3.

[adiroiban]

@psi29a thanks. Let me see what I can do... is only 3 comments but py3 porting stuff are mixed with style changes so it hard to tell which are the critical parts which needs extra care during review.

I will try to break it into 2 PRs. One for the cleanup and then another one for the actual py3 stuff.

I expect that the cleanup PR will be accepted without any controversy :)

[s0undt3ch]

If this gets py3 support I'll also move an internal project to it(been using a java test LDAP server for integration tests, ugh)...

[adiroiban]

@psi29a I am trying to work on your branch... but is hard.
I moved the non related py3 changes into a separate PR, but I am still confused by some changes.

For example, I don't understand why list is forced in some places, where an iterator would also be fine:

master...psi29a:python3#diff-27e63de333e9229e42907d35ed5b030aR138

[adiroiban]

I sent some PR which are targeting the py3 port.

To make it easier to review I plan to break them into multiple ones.

Next I plan to work on updating the code so that python2 -3 trial ldaptor will no show any errors

If you have time, please review the current PRs

[psi29a]

Sure, thanks for putting forth the effort!

Update: merged the cleanup branch. for future reference, we should be touching the Changelog as well when making PRs, so that it makes it easier to make releases... it's already in the changelog! :)

[adiroiban]

@s0undt3ch since you are already using a java ldap server, I guess that this is done in a separate process.
Why not run ldaptor now in the same way, ie a separate python2 process.

Note that I am helping with porting the tests to python3... and check no regressions are done in python2.
I don't have any python3 production code, so even if all the tests are green on python3 the port might be broken.

@psi29a does coveralls provide diff reports. I searched the internet but I could not find how to enable it.
Also, at https://coveralls.io/github/twisted/ldaptor I don't have any configuration options.
Can you or someone who has access enable the coverage report for git commit status?

Maybe we should switch from coveralls to codecov.io and get diff reports and enforce a 100% coverage for the diff.

[psi29a]

I'll look into it, thanks. :)

[adiroiban]

Coverage reported for added to the PR diff.

I am doing my best to not introduce regressions ... so when tests are missing I will do my try to write them... resulting in bigger diffs... but I hope for the best

#86 needs review

I have to other branches on the pipeline... but they depend on the changes from #86

[cwaldbieser]

Just wondering-- do we have some kind of strategy for supporting Python 3.x? Is there a recommended approach we want contributors to take? Is there existing guidance?

[psi29a]

Latest Twisted release has even more things ported to Py3.

I think the strategy is making sure code runs on both Py2 and Py3 but I believe the blocker has always been Twisted itself.

[adiroiban]

I think that the first thing to do is to make sure that the existing test pass on py3.... that is a test driven porting.

Not sure if latest Twisted can help. AFAIK, all modules used by ldaptor were already ported in January 2018.

Even with #96 merged, there is still a significant number of failing test.

Ran 523 tests in 1.713s
FAILED (failures=64, errors=137, successes=400)

But, as far as I remember, most part of the remaining failures are due to 2 or 3 methods which needs to be ported

[adiroiban]

I think that that the main decision should be made regarding the internal handling of data.
Should ldaptor API enforce bytes, or should it accept Unicode/text.

When running ldaptor under python2 there might be some places where Unicode/text will work due to the relax comparison in Python2.7

For example:

Python 2.7.12 (default, Dec  4 2017, 14:50:18) 
>>> u'text' == b'text'
True

Python 3.5.2 (default, Nov 23 2017, 16:37:01) 
>>> u'text' == b'text'
False
 

---

There are a couple of things that need to be agreed in term of strategy when the default behaviour of py2 and py3 differ.

For example:

Python 2.7.12 (default, Dec  4 2017, 14:50:18) 
>>> object() > object()
False

Python 3.5.2 (default, Nov 23 2017, 16:37:01) 
>>> object() > object()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unorderable types: object() > object()

---

Since there are not many people with time to develop ldaptor and dedicate time to work at backward compatibility code, I suggest to look to the future and implement Python3 behaviour, even in Python2.

This might break the Python code using ldaptor, but python2 will be gone anyway so the code should be updated anyway at some point.

But I am ok to go with any other option.

[psi29a]

@wiml and merged! :)

Thanks for bringing us one step further.

[hawkowl]

Hi! Anything I can do to lend some of my porting expertise?

[adiroiban]

@hawkowl see status of latest tests https://travis-ci.org/twisted/ldaptor/jobs/443707583

@GrayAn made a great step by implemeting toWire.

Some ldiff related tests are failing... but because of Unicode vs bytes values... so they only need explicit bytes markup in the code.

ex

First differing element 0:
Modif[360 chars]'dc', value='com'),)))), modifications=[Add(b'foo', [b'bar'])])
Modif[360 chars]'dc', value='com'),)))), modifications=[Add('foo', ['bar'])])

and more code need to be migrated to toWire

- server.dataReceived(str(pureldap.LDAPMessage(pureldap.LDAPBindRequest(dn='cn=jack,dc=example,dc=com', auth='s3krit'), id=4)))
+ server.dataReceived(pureldap.LDAPMessage(pureldap.LDAPBindRequest(dn='cn=jack,dc=example,dc=com', auth='s3krit'), id=4).toWire())

So, I think that there is nothing complicated left on the porting... just hard work :(

[cwaldbieser]

If we want PRs for Python3 compatibility, should be have something in CONTRIBUTING that gives some pointers on how to get started or how to test? E.g. do I need to make changes to the tox config or command I run to test for Python3? What are the major issues and what is the general strategy I can use?

[adiroiban]

@cwaldbieser We can put some notes to CONTRIBUTING.... but I guess that each person can have a different strategy.

My plan was to migrate all the existing tests first and only then look at the actual ldaptor functionality on Py3.
While porting the tests, I have created a tox env with all the tests which were already ported, to avoid regressions.
While porting py3 code, I wrote the small build code to report coverage as it was a great help for me to detect unused code or code without tests.

If I found a major issue, I just wrote here or discussed it as part of a specific PR.

But, I don't know if it is still worth updating CONTRIBUTING.
I think that we are the point at which we should just refactor all the code to use .toWire() instead of str() and then port client, server and other tools.

[psi29a]

I agree with @adiroiban, refactoring to use .toWire() will get Ldaptor far without any additional major refactoring work.

[cwaldbieser]

Yes, I agree that str() -> toWire() is the major work that needs to happen.

What I mean is that "CONTRIBUTING.rst" currently has a helpful bit in it about what tox commands to run to check your work. It kind of assumes you know what those commands do. If you don't, and you take something like tox -e py27-test-dev at face value while you're doing Py3 porting work, you will probably not end up testing against the correct environment(s).

I'm thinking that the time is probably right to update the recommended environment to something like tox -e 'py27-test-dev,py36-test-dev' and give enough context to explain that the "pyXX" parts should represent the local python versions you actually have installed. I am fairly new to tox, myself, and this has tripped me up more than once.

[psi29a]

Sounds good to me. :)

[GrayAn]

I've got a problem when porting LDIF and LDAPServer modules. In short: encoding and decoding back BaseLDAPEntry (and similar objects) with unicode attributes turns it to an object with bytes attributes. For example this object:

BaseLDAPEntry('cn=foo,dc=example,dc=com', {'foo': ['bar'], 'thud': ['quux', 'baz']})

turns into this after encoding and decoding back:

BaseLDAPEntry(b'cn=foo,dc=example,dc=com', {b'foo': [b'bar'], b'thud': [b'quux', b'baz']})

It is perfectly compatible with Python 2. But for Python 3 users it can be confusing as they might expect unicode values for entry attributes. Do we need to leave entry attributes in bytes for backward compatibility?

[psi29a]

Well... https://pythonclock.org/

We'll eventually have to deprecate Python2, but sadly we can't break compatibility.

[cwaldbieser]

I'd say that regardless of Python version, clients expect to get text for LDAP strings, ints for LDAP ints, etc. @GrayAn , do you have a minimal sample script where this happens so I can understand what is going on better?

[adiroiban]

turns into this after encoding and decoding back:

Yep. Ugly stuff.

I think that for now, we need to leave them as bytes :(

You might want to add a separate decoding function which will take bytes and an encoding (can default to UTF-8) and return an LDAP entry in Unicode.

---

I'd say that regardless of Python version, clients expect to get text for LDAP strings, ints for LDAP ints, etc.

I think that the main question here is back compatibility.
People might expect Unicode or Integers, but I think that ldaptor from master will always return bytes.

clients expect to get text for LDAP strings, ints for LDAP ints, etc.

Does ldaptor implement schema / matching rules as in https://tools.ietf.org/html/rfc4517 ?

I am using ldaptor in Python 2.7 and I am handling everything as raw bytes without schemas.

[cwaldbieser]

So if I understand correctly, the crux of the issue is:

• LDAP attributes that are text must be UTF-8 encoded.
• When LDAP messages are encoded to a binary form so they can be transmitted between clients and servers, a subset of Basic Encoding Rules (BER) are used.
• LDAP UTF-8 encoded text attributes are encoded as OCTET STRINGS for the wire format.
• LDAP binary attributes are also encoded as OCTET STRINGS for the wire format.
• When transforming the attributes back, there is no way to determine if the attribute should be binary data or text data without making use of LDAP schemas.
• In Python2.7 this issue wasn't as noticeable because the binary data returned was more or less interchangeable with Python strings, especially if the data used only ASCII encodings.

What about DNs, though? In RFC 4514, section 3:

The string representation of Distinguished Names is restricted to UTF-8 [RFC3629] encoded Unicode [Unicode] characters.

Seems like the objectName attribute ought to be returning unicode ...

[cwaldbieser]

And attribute names-- don't they have to be LDAPStrings?

[adiroiban]

And attribute names-- don't they have to be LDAPStrings?

Yes. I think that Attributes names should be Unicode/text.
And the RFC is clear about UTF-8 encoding.

---

• LDAP binary attributes are also encoded as OCTET STRINGS for the wire format.

I remember seeing some LDAP based tool using base64 to store the profile picture or SSH/GPG public keys.

---

• When transforming the attributes back, there is no way to determine if the attribute should be binary data or text data without making use of LDAP schemas.

Yes. I don't know how else you can do it without a schema.

As a workaround, we can hardcode some conversions based on some well known schemas/attribute names.

I think that unless we have a search method which takes a schema, the values should be bytes and each user will do its conversion... but as a helper, we can have a separate search function which does UTF-8 decoding for all values.

---

I am storing/searching only text in LDAP with ldaptor, but with the current API I work with bytes, and I do explicit encoding/decoding of data when passing / receiving from ldaptor.

I don't have Unicode attribute names.

Any implicit encoding done in any future ldaptor version will break my code...and I guess the same will happen to other users.

So, the default API should return bytes as values in LDAP search.

---

I don't know what to say about the server, as I am only using ldaptor in production for client-side.
I do use the server-side, but only as testing props.

[GrayAn]

do you have a minimal sample script where this happens so I can understand what is going on better?

Here is a small example for illustration:

from ldaptor.protocols.pureldap import LDAPMessage, LDAPSearchResultEntry, \
    LDAPBERDecoderContext, \
    LDAPBERDecoderContext_TopLevel, LDAPBERDecoderContext_LDAPMessage
from ldaptor.protocols.pureber import berDecodeObject, BERDecoderContext


decoder = LDAPBERDecoderContext_TopLevel(
    inherit=LDAPBERDecoderContext_LDAPMessage(
        fallback=LDAPBERDecoderContext(fallback=BERDecoderContext()),
        inherit=LDAPBERDecoderContext(fallback=BERDecoderContext())
    )
)
entry = LDAPSearchResultEntry(
    objectName='dc=example,dc=com',
    attributes=[('a', ['b', 'c'])]
)
msg = LDAPMessage(entry, id=1)
decoded_msg, length = berDecodeObject(decoder, msg.toWire())
decoded_entry = decoded_msg.value

print('Entry: {}'.format(repr(entry)))
print('Type of entry objectName: {}'.format(entry.objectName.__class__.__name__))
print('Decoded entry: {}'.format(repr(decoded_entry)))
print('Type of decoded entry objectName: {}\n'.format(
    decoded_entry.objectName.__class__.__name__
))

assert entry == decoded_entry

The output in Python 2.7:

Entry: LDAPSearchResultEntry(objectName='dc=example,dc=com', attributes=[('a', ['b', 'c'])]
Type of entry objectName: str
Decoded entry: LDAPSearchResultEntry(objectName='dc=example,dc=com', attributes=[('a', ['b', 'c'])]
Type of decoded entry objectName: str

The output in Python 3.5:

Entry: LDAPSearchResultEntry(objectName='dc=example,dc=com', attributes=[('a', ['b', 'c'])]
Type of entry objectName: str
Decoded entry: LDAPSearchResultEntry(objectName="b'dc=example,dc=com'", attributes=[(b'a', ["b'b'", "b'c'"])]
Type of decoded entry objectName: bytes

Traceback (most recent call last):
  File "test.py", line 26, in <module>
    assert entry == decoded_entry
AssertionError

So, the default API should return bytes as values in LDAP search.

Agree. So, bytestrings are decoded to objects with bytes attributes. And if we want them to be decoded to unicode additional flag to_unicode for Decoder objects could be implemented I think.

[GrayAn]

So all tests are passed for Python 2.7 and Python 3.5 now. I checked how the head version works for my project (it uses LDAPServer functionality). Python 3 support required me to make several changes in places where I deal with internal library objects (BaseEntry subclasses, LDAPMessage subclasses, DistinguishedName etc):

• Replacing str with toWire method: str(entry.dn) -> entry.dn.toWire().
• Replacing string literals with explicit bytes literals: if request.baseObject == '' -> if request.baseObject == b''.

It looks like it works :)

[adiroiban]

It looks like it works :)

Great news.
I think that as long as the changes are not extensive, is ok to ask for users to do a few changes.

I would argue that using .toWire() instead of str() is not a complicated change and is a fair think to ask from our users.

Keeping str() would have put a lot of effort on ldaptor maintainers ...and as we can see there are not many resources.

Also, the .toWire() changes are Python2 compatible so you can do the .toWire() changes on Python2, run all your existing tests and then move to Pyhon3.

---

Using string literals in Python3 for b'' is required anyway, even if you don't use ldaptor.

---

@GrayAn Thanks for your example. Much appreciated.

I don't understand objectName="b'dc=example,dc=com'" ....
Should this be objectName=b'dc=example,dc=com' ?

[psi29a]

This should be documented then we can make a big-bang release

[GrayAn]

I don't understand objectName="b'dc=example,dc=com'" ....
Should this be objectName=b'dc=example,dc=com' ?

Yes, it looks better without many quotation marks. LDAPSearchResultEntry.__repr__ method has the repr(str(self.objectName)) code which returns ugly result if self.objectName is a byte string. This problem can be found in different classes and could be fixed but we need to implement unit tests for __repr__ methods as well :)

[adiroiban]

but we need to implement unit tests for __repr__ methods as well :)

I think that __str__ and __repr__ shouldn't be part of the public API. No need to write tests for it :)

So, bytestrings are decoded to objects with bytes attributes. And if we want them to be decoded to unicode additional flag to_unicode for Decoder objects could be implemented I think.

For decoding, you might want to pass a Schema Decoder... so that numbers are decoded as numbers, and text as unicode, and photos will continue to stay as bytes.

From RFC, my understanding is that values should be 'bytes' https://tools.ietf.org/html/rfc4511#section-4.1.5

  A field of type AttributeValue is an OCTET STRING containing an
   encoded attribute value.  The attribute value is encoded according to
   the LDAP-specific encoding definition of its corresponding syntax.
   The LDAP-specific encoding definitions for different syntaxes and
   attribute types may be found in other documents and in particular
   [RFC4517].

        AttributeValue ::= OCTET STRING

   Note that there is no defined limit on the size of this encoding;
   thus, protocol values may include multi-megabyte attribute values
   (e.g., photographs).

   Attribute values may be defined that have arbitrary and non-printable
   syntax.  Implementations MUST NOT display or attempt to decode an
   attribute value if its syntax is not known.  The implementation may
   attempt to discover the subschema of the source entry and to retrieve
   the descriptions of 'attributeTypes' from it [RFC4512].

   Clients MUST only send attribute values in a request that are valid
   according to the syntax defined for the attributes.

.... so, if no explicit decoder is passed to LDAPSearchResultEntry, it should return all values as bytes in both Python 2 and Python3.

---

To answer the original question

I've got a problem when porting LDIF and LDAPServer modules. In short: encoding and decoding back BaseLDAPEntry (and similar objects) with unicode attributes turns it to an object with bytes attributes.

It is perfectly compatible with Python 2. But for Python 3 users it can be confusing as they might expect unicode values for entry attributes. Do we need to leave entry attributes in bytes for backward compatibility?

Yes. We need to leave attributes in bytes.
Otherwise we will break code in ugly ways which are hard to detect.

Python2 and Python3 users should expect that they will get bytes and for now they should do their own conversions.

We can implement a simple utility to decode all BaseLDAPEntry as UTF-8 text.... or implement an TextLDAPEntry which is similar to BaseLDAPEntry but does an implicit utf-8 decoding.

[GrayAn]

I've got a question regarding __repr__ methods. It looks like they also need to be fixed to support the both Python versions. But tox and codecov will complain if these changes will not be covered by tests. Should we write tests for them or just ignore tox and codecov warnings? Maybe there is a way we can mark the methods we do not need to check for test coverage.

These methods can raise exceptions if running on the Python 3. For example, LDAPServer with logging enabled uses __repr__ of pureldap objects:

log.msg('S<-C %s' % repr(msg), debug=True)

Maybe they deserve to be test covered? :)

[adiroiban]

Maybe they deserve to be test covered? :)

Yes. It would be nice to have a bare minimal tests... if you or someone else has time.
Otherwise, you can use the pragma comment https://coverage.readthedocs.io/en/v4.5.x/excluding.html

[graingert]

twisted has dropped support for python 2

[psi29a]

Some serious efforts are going on here:
#174

[graingert]

Because we test against twisted trunk, Python 2 support has been dropped.

Python 3.5 support will be dropped once twisted trunk drops py3.5 support