Skip to content

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

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
droideck merged 4 commits into from
Aug 7, 2025
Merged

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

droideck merged 4 commits into from
Aug 7, 2025

Conversation

@spaceone
Copy link
Contributor

@spaceone spaceone commented Apr 1, 2022 •
edited
Loading

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 spaceone force-pushed the dn2str-flags branch 2 times, most recently from 6948a13 to f25fa60 Compare April 1, 2022 14:01
@droideck
Copy link
Contributor

droideck commented Aug 31, 2022

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

@spaceone
Copy link
Contributor Author

spaceone commented Sep 25, 2022

@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
Copy link
Contributor

droideck commented Sep 26, 2022

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

@mistotebe
Copy link
Contributor

mistotebe commented Sep 27, 2022

I agree, getting better DN handling would be nice.

spaceone
spaceone commented Sep 28, 2022
View reviewed changes
@spaceone 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 spaceone requested a review from mistotebe June 3, 2025 21:51
@spaceone spaceone force-pushed the dn2str-flags branch 3 times, most recently from ebdee71 to 7c80d5c Compare June 4, 2025 06:25
@spaceone
Copy link
Contributor Author

spaceone commented Jun 4, 2025

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
mistotebe previously approved these changes Jun 10, 2025
View reviewed changes
Copy link
Contributor

@mistotebe mistotebe left a comment

Choose a reason for hiding this comment

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

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

@spaceone
Copy link
Contributor Author

spaceone commented Jun 11, 2025

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 spaceone requested a review from mistotebe June 13, 2025 21:53
@spaceone
Copy link
Contributor Author

spaceone commented Jul 31, 2025

@droideck maybe also a review from your side?

droideck
droideck previously approved these changes Jul 31, 2025
View reviewed changes
Copy link
Contributor

@droideck droideck left a comment

Choose a reason for hiding this comment

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

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.

@spaceone
Copy link
Contributor Author

spaceone commented Aug 1, 2025 •
edited
Loading

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 spaceone requested a review from droideck August 3, 2025 13:07
droideck
droideck previously approved these changes Aug 6, 2025
View reviewed changes
Copy link
Contributor

@droideck droideck left a comment

Choose a reason for hiding this comment

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

Looks good to me!

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

@spaceone
Copy link
Contributor Author

spaceone commented Aug 7, 2025 •
edited
Loading

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

spaceone and others added 4 commits August 7, 2025 09:35
@spaceone
test(ldap.dn): Add test cases for ldap.dn.dn2str() with different for…
2767c29 
…mat flags
@spaceone
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
@spaceone
feat(ldap.dn): add ldap.dn.normalize()
4dd2b52
@spaceone
style(Tests/t_ldap_dn): use raw strings instead of escape sequences
290dfe9
@droideck droideck merged commit 1d978c6 into python-ldap:main Aug 7, 2025
16 checks passed
@spaceone spaceone mentioned this pull request Aug 8, 2025
Closed
@droideck droideck mentioned this pull request Oct 7, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@droideck droideck droideck approved these changes

@mistotebe mistotebe Awaiting requested review from mistotebe

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ldap.dn.dn2str() does not support flags

3 participants

@spaceone @droideck @mistotebe