FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat(ldap.dn): Add support for different formats in `ldap.dn2str()` via flags by spaceone · Pull Request #466 · python-ldap/python-ldap · GitHub

/ python-ldap Public

feat(ldap.dn): Add support for different formats in ldap.dn2str() via flags - #466

Merged
droideck merged 4 commits into
python-ldap:mainfrom
spaceone:dn2str-flags
Aug 7, 2025
ConversationCommits (4)ChecksFiles changed
Merged

feat(ldap.dn): Add support for different formats in ldap.dn2str() via flags#466
droideck merged 4 commits into
python-ldap:mainfrom
spaceone:dn2str-flags

Conversation

spaceone commented Apr 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

In C dn2str() supports flags which works by providing LDAP_DN_FORMAT_UFN, LDAP_DN_FORMAT_AD_CANONICAL, ….
These symbols do exist in Python, but could not be used ultimately because the Python counterpart was pure Python and did not pass to dn2str(3).

Fix #257

spaceone force-pushed the dn2str-flags branch 2 times, most recently from 6948a13 to f25fa60 Compare April 1, 2022 14:01

droideck commented Aug 31, 2022

Copy link
Copy Markdown
Member

@spaceone hi!
The PR says 'WIP', - is it still true?

spaceone commented Sep 25, 2022

Copy link
Copy Markdown
Contributor Author

@spaceone hi! The PR says 'WIP', - is it still true?

@droideck Well, I would say it needs code clenaup but the functionality is done. But before I try to cleanup I would like to get feedback.

droideck commented Sep 26, 2022

Copy link
Copy Markdown
Member

Overall, I think it'll be nice to have the feature.
@mistotebe what do you think?

mistotebe commented Sep 27, 2022

Copy link
Copy Markdown
Contributor

I agree, getting better DN handling would be nice.

spaceone commented Sep 28, 2022
View reviewed changes
Comment thread Tests/t_ldap_dn.py Outdated
Comment thread Tests/t_ldap_dn.py
Comment thread Lib/ldap/dn.py Outdated
spaceone changed the title WIP: Add support for flags in ldap.dn2str() feat(ldap.dn): Add support for different formats in ldap.dn2str() via flags Jun 3, 2025
spaceone requested a review from mistotebe June 3, 2025 21:51
spaceone force-pushed the dn2str-flags branch 3 times, most recently from ebdee71 to 7c80d5c Compare June 4, 2025 06:25

spaceone commented Jun 4, 2025

Copy link
Copy Markdown
Contributor Author

Performance comparison:

C:

python3 -m timeit -s 'import ldap.dn' "ldap.dn.dn2str([[('uid', 'test, 42', 1)],[('ou', 'Testing', 1)],[('dc', 'example', 1)],[('dc', 'com', 1)]], ldap.DN_FORMAT_LDAPV3)"
200000 loops, best of 5: 1.41 usec per loop

Python:

python3 -m timeit -s 'import ldap.dn' "ldap.dn.dn2str([[('uid', 'test, 42', 1)],[('ou', 'Testing', 1)],[('dc', 'example', 1)],[('dc', 'com', 1)]], 0)"
50000 loops, best of 5: 4.1 usec per loop

mistotebe previously approved these changes Jun 10, 2025
View reviewed changes

mistotebe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, maybe be consistent about using raw strings (r'cn=\C3...' vs. 'cn=\\C3...')

spaceone commented Jun 11, 2025

Copy link
Copy Markdown
Contributor Author

LGTM, maybe be consistent about using raw strings (r'cn=\C3...' vs. 'cn=\\C3...')

Thanks for approval. I added a commit which unifies the whole file, to use raw-strings.

spaceone requested a review from mistotebe June 13, 2025 21:53

spaceone commented Jul 31, 2025

Copy link
Copy Markdown
Contributor Author

@droideck maybe also a review from your side?

droideck previously approved these changes Jul 31, 2025
View reviewed changes

droideck left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good to me!

Could you please add a couple of more tests?
For NUL handling, for huge DNs, and for invalid flags.

If not, no worries, we can improve it later.

Comment thread Modules/functions.c Outdated

spaceone commented Aug 1, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Looks good to me!

Thank you!

Could you please add a couple of more tests?
For NUL handling

I added every possible combination I can think of :-)

for huge DNs

Okay, added multiple string lengthes.

and for invalid flags.

this already existed.

spaceone requested a review from droideck August 3, 2025 13:07
droideck previously approved these changes Aug 6, 2025
View reviewed changes

droideck left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good to me!

Please, squash the commits if you want (or confirm if you want to have it as separate commits).

spaceone commented Aug 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

The commits should not be squashed, they follow conventional commit specification.

spaceone and others added 4 commits August 7, 2025 09:35
test(ldap.dn): Add test cases for ldap.dn.dn2str() with different for…
2767c29 
…mat flags
feat(ldap.dn): Add support for different formats in `ldap.dn.dn2str()…
6bece3e 
…` via flags

In C `dn2str()` supports `flags` which works by providing one of `LDAP_DN_FORMAT_UFN`, `LDAP_DN_FORMAT_AD_CANONICAL`, `LDAP_DN_FORMAT_DCE`, `LDAP_DN_FORMAT_LDAPV3`.
These symbols do exist in Python, but could not be used ultimately because the Python counterpart was pure Python and did not pass to `dn2str(3)`.

Fix python-ldap#257
feat(ldap.dn): add ldap.dn.normalize()
4dd2b52
style(Tests/t_ldap_dn): use raw strings instead of escape sequences
290dfe9
droideck merged commit 1d978c6 into python-ldap:main Aug 7, 2025
16 checks passed
spaceone mentioned this pull request Aug 8, 2025
Closed
droideck mentioned this pull request Oct 7, 2025
Merged

spaceone commented Apr 17, 2026

Copy link
Copy Markdown
Contributor Author

@droideck When can I expect a release containing this (backwards compatible) feature? Since month there are no commits again. And even after this is contained in a release, I have to wait a specific time, so that it appears in Debian, too. It would enhance our build process a lot if we could use official releases instead of building everything on our own.

mistotebe added this to the 3.5.0 milestone May 14, 2026
spaceone mentioned this pull request Jul 22, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

droideck droideck approved these changes
mistotebe Awaiting requested review from mistotebe

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.5.0

Development

Successfully merging this pull request may close these issues.

ldap.dn.dn2str() does not support flags

3 participants

Back | FazBrowse Home | New Git URL