Draft: new response handling API #464

mistotebe · 2022-03-24T11:35:09Z

tests
intermediate/exop decoding

encukou · 2022-03-24T12:44:23Z

Modules/LDAPObject.c

+    .name = "_RawLDAPMessage",
+    .doc = "LDAP Message returned from native code",
+    .fields = message_fields,
+    .n_in_sequence = sizeof(message_fields)/sizeof(*message_fields) - 1,


Set this to 2 to preserve backwards compatibility in result. (And ideally, at the same time, deprecate using result() as a tuple, along with resultN, in favor of the named fields).
This would allow old code to continue using type, data = result().

Setting this to a value that will grow as more fields are added defeats the purpose of PyStructSequence – you might as well use a regular object with getset descriptors.

This doesn't feel remotely compatible with LDAPObject.result() anyway, that's one of the reasons the new API is split into a Connection object (which gets a better request API too in a future PR). Not sure whether to make the result* APIs a (heavyweight) wrapper for this or keep it for now.

Keeping n_in_sequence automatic is an oversight from initial development, thanks for pointing that out, I'll fix it in the next version. What is your take on the Response subclassing logic? I will probably have to replicate it for ExtendedResponse and intermediates, can you see any conceptual issues with how it is written/is it a total hack?

Sadly I won't have time to review the whole PR :(
Could you point to the part I should look at?

Turns out I forgot to add the Python files into the commit so my last comment probably didn't make much sense. I meant the Python bits, this in particular.

At first glance, that looks more complicated than it needs to be. i don't see much advantage over setting the missing attributes to None (or keeping them missing). Is there an advantage?

The subclassing allows one to expose response-specific methods (Compare result, allow parsing the extended result) and if the user is on 3.10, allow pattern matching, on others, make the code more readable, especially if controls were subjected to the same treatment.

droideck · 2025-08-06T16:18:45Z

@mistotebe I'm thinking about python-ldap 4.0 release.
Do you think this PR is ready for review? We can include it earlier (as a tech preview or something like that). And while it's being improved and handled as an experimental feature, we can stabilize it for python-ldap 5.0 in the future.
Does it sound good to you?

mistotebe · 2025-08-08T10:39:05Z

Any feedback is welcome, yes. I've been using this branch elsewhere with some success and the tools added here certainly make porting some of my tools (e.g. syncmonitor) from python-ldap0 an to some extent pldap easier. Don't think I'll be abandoning pldap any time soon (there is no replacement to its stub server implementation for protocol level testing) but definitely closer to dropping python-ldap0 after we get something like this in.

There's still a little bit of a kludge in how e.g. searches are represented, so the API might still evolve, and it's only a stepping stone towards:

a more ergonomic async-friendly API
a proper overhaul of the request side

But it's definitely useful already so happy to start cleaning up or breaking it up starting with bits you consider less contentious and go from there.

spaceone · 2025-08-08T12:15:55Z

a more ergonomic async-friendly API
a proper overhaul of the request side

Maybe my project is suitable for this. I am supporting a nice user-friendly API with sync and async, hiding the _s and _ext details but providing all functionality LDAP clients supports (on top of python-ldap) and especially focusing on correctness for example regarding DN and filter escape handling:
See https://docs.freeiam.org/ and https://github.com/Free-IAM/freeiam.

mistotebe · 2025-08-12T16:21:53Z

a more ergonomic async-friendly API
a proper overhaul of the request side

Maybe my project is suitable for this. I am supporting a nice user-friendly API with sync and async, hiding the _s and _ext details but providing all functionality LDAP clients supports (on top of python-ldap) and especially focusing on correctness for example regarding DN and filter escape handling: See https://docs.freeiam.org/ and https://github.com/Free-IAM/freeiam.

You should definitely be able to make use of the new interfaces and response classes added here to make your own life easier.

In terms of request API (and async), yes yours looks somewhat similar to what I wanted, but there is one important difference: any request (not just search) can return any amount of "intermediate" responses (search entry, referral entry, intermediate) and then a final response or exception so a request object should (always?) be iterable. A caller might be able to say they (don't) expect those, they still have a choice to let the whole request finish or they can receive messages one by one and pattern match on the type (SearchResultEntry, SyncInfoNewCookie, generic IntermediateResponse, ...) etc.

There is a partial draft of how the async side could look, what you get is a Future and optionally an iterable for the intermediate responses.

We can definitely see how to converge those two APIs because in some ways your take is close to what I had in mind and a lot of things I can see there should simplify to "syntactic sugar"/shorthand over the new API.

spaceone · 2025-08-13T10:44:17Z

Thanks for the explanation.
I already use provide iterated search results via all=1 , but that seems to be a lot slower, so i renamed it to search_iter().
For the referrals, I thought that wouldn't be a intermediate result but a final result. My plan was to add a connection pool, which also handles referrals.
So I will change my API that the result can be a future and a nice way to decide whether you want a iterable or not.
I also need to find out how I can trigger these intermediate responses with openldap, so I still can get realistic 100% test coverage.

mistotebe · 2025-08-13T11:13:10Z

Thanks for the explanation. I already use provide iterated search results via all=1 , but that seems to be a lot slower, so i renamed it to search_iter().

Slower? Are we both talking about ldap_result() with all=MSG_ONE (and if async, having a select() there)?

For the referrals, I thought that wouldn't be a intermediate result but a final result. My plan was to add a connection pool, which also handles referrals.

I mean SearchReference, not Result.result == LDAP_REFERRAL. Opting to handle referrals is an option, sure, but the client should have an option to return them (and sometimes that makes sense?).

So I will change my API that the result can be a future and a nice way to decide whether you want a iterable or not. I also need to find out how I can trigger these intermediate responses with openldap, so I still can get realistic 100% test coverage.

Often syncrepl control (mode=refreshAndPersist) is the easiest one, or LDAP TXN (RFC 5805). Both exist in OpenLDAP, syncrepl needs the syncprov overlay configured, TXNs need a compatible backend (e.g. back-mdb).

droideck

While I can't see any obvious leaks, the complex control flow around error handling needs careful testing under various failure scenarios. We might need to have an ASAN run here.

And, of course, all tests need to pass consistently before we merge anything.

Modules/LDAPObject.c

droideck · 2025-08-20T03:45:21Z

Lib/ldap/extop/__init__.py

+             defaultClass: Optional[type[ExtendedResponse]] = None,
+             **kwargs)
+
+    The old API is deprecated and will be removed in 4.0.


If you agree with my proposal, this version comment can be changed.

Agreed, this was based on a discussion we had before the plan you've just outlined below.

droideck · 2025-08-20T03:46:53Z

Lib/ldap/extop/__init__.py

+  @classmethod
+  def __convert_old_api(cls, responseName_or_msgid=_NOTSET,
+                        encodedResponseValue_or_msgtype=_NOTSET,
+                        controls=None, *,
+                        result=_NOTSET, matcheddn=_NOTSET, message=_NOTSET,
+                        referrals=_NOTSET, name=None, value=None,
+                        defaultClass: Optional[type['ExtendedResult']] = None,
+                        msgid=_NOTSET, msgtype=_NOTSET,
+                        responseName=_NOTSET, encodedResponseValue=_NOTSET,
+                        **kwargs):
+    """
+    Implements both old and new API:
+    __init__(self, responseName, encodedResponseValue)
+    and
+    __init__/__new__(self, msgid, msgtype, controls=None, *,
+                     result, matcheddn, message, referrals,
+                     defaultClass=None, **kwargs)
+    """


It looks overly complex, IMO.
Can we split it into two?

Something like

@classmethod def from_old_api(cls, responseName, encodedResponseValue) @classmethod def from_new_api(cls, msgid, msgtype, ...)

I'm not happy about it but ExtendedResponse exists already with a fairly pointless signature but if we ignored it, we'd have to duplicate all the decoding code everywhere else? I hadn't found another way of doing it back then but if you think this will work with __init__/__new__, I'll need a little more because I'm still not seeing it...

Maybe we can create a temporary transitional class? Like ExtendedResultOrSomething(ldap.response.ExtendedResult): with the new implementation? And in 5.0, we can swap it with the existedExtendedResponse

But I don't have a hard opinion here. If this is temporary code that dies in 5.0/6.0 anyway, maybe the current approach is fine?

droideck · 2025-08-20T04:03:52Z

Okay, I took some time to review the code. This is impressive work and I like it a lot!

As discussed, we probably should split breaking and non-breaking code between releases.

4.0 should include non-breaking stuff and with some documentation and release notes we can encourage others to use the tools.
5.0 may switch to the new API as default (when it's thoroughly tested and used by the community), while keeping the possibility to switch to the old approach.
6.0 can completely remove the old way of processing (it won't be any time soon, of course).

For python-ldap 4.0 (as experimental/tech preview), I believe we could include the following stable components that provide immediate value without breaking existing code. We should clearly mark these as experimental in the documentation:

The new response classes hierarchy (ldap.response.*) - These are well-designed and provide a much cleaner object-oriented interface. The raise_for_result() pattern is particularly nice.
Control and ExtOp subclass registration - The automatic OID registration via init_subclass hooks? (if I understand correctly)
The improved C-level message parsing (l_ldap_result), though we need to ensure backward compatibility for existing code.
Enhanced Syncrepl support? I don't have much experience with tech though...

For python-ldap 5.0 (full stable release):

Connection class - This is the bigger API change that would benefit from user feedback during 4.x series.
Full async-friendly request API - As you mentioned, this is still evolving and should mature in 5.0.
Breaking changes to existing APIs - Any deprecation of result4() and similar methods should wait for 5.0, IMO.

What do you think about the idea/split?
And which components you want to include in the following 4.0 release?

mistotebe · 2025-08-20T09:25:22Z

Thanks for looking at it. Broadly I agree with the plan except that delaying Connection to 5.0 makes all the 4.0 additions moot because it leaves no way of reaching all the new code/classes.

How about this?

4.0 adds Connection class with a note that the API is experimental and will evolve during 4.x in incompatible ways (e.g. request side of the API, async support)
5.0 when we consider the Connection API stable and we can mark LDAPObject as deprecated (might even reimplement it as a wrapper of Connection while preserving behaviour)
6.0 LDAPObject and old APIs in the C module that aren't needed by Connection can disappear

Does all the mucking with __new__ to redirect to the appropriate subclass feel like a hack? Is there a better scheme to achieve something similar? I think I tried using factories to do this, not sure why I abandoned that approach or if I just never finished hashing it out.

droideck

I was thinking about those components as standalone tools as I thought you want smaller scale of that exposed in 4.0.
But yeah, without Connection there's no easy way to actually use any of the new stuff. Your timeline makes more sense.

So 4.0 with experimental/tech preview Connection, 5.0 stable + deprecate LDAPObject, 6.0 remove old stuff - sounds good to me.

About the __new__ pattern - It's clever but honestly it does feel a bit too magical... The main issues I see:

Readability - When someone calls Response(msgid, msgtype, ...) and gets back a SearchEntry, that's surprising behavior, so it violates the principle of least surprise. The metaclass magic isn't immediately obvious when debugging.
The class-level registry - That __subclasses dict could be problematic if someone ever subclasses Response for their own framework. Plus the assert in __init_subclass__ is a bit aggressive (I left a comment below).

Alternative approaches that might be cleaner:

Explicit factory functions: Just have response_from_message(msgid, msgtype, ...) that does the dispatch. Less magical, easier to debug.
A proper factory class: Something like ResponseFactory.create(msgid, msgtype, ...). You could even make it pluggable for custom response types.
Keep new but make it more explicit: Maybe rename Response to ResponseBase or AbstractResponse so it's clear it's not meant to be instantiated directly?

That said, if the pattern works and you're comfortable with it, I'm not too much against that. The automatic subclass registration via __init_subclass__ is actually pretty neat. Maybe just needs better documentation/comments explaining the magic?

droideck · 2025-08-20T16:39:30Z

Lib/ldap/response.py

+        if not hasattr(cls, 'msgtype'):
+            return
+        c = __class__.__subclasses.setdefault(cls.msgtype, cls)
+        assert issubclass(cls, c)


I think the assert might be a bit too aggressive.
If the API will be used for some CustomSearchEntry(Response) creation and will create two Response subclasses with the same msgtype, then the default SearchEntry is set and assert fails.

It's still not something that should be done usually, but I think it'll be nicer to have something like that here:

if not issubclass(cls, c): raise TypeError(f"Cannot register {cls} for msgtype ..."

droideck · 2025-08-20T17:16:06Z

Lib/ldap/extop/__init__.py

+  @classmethod
+  def __convert_old_api(cls, responseName_or_msgid=_NOTSET,
+                        encodedResponseValue_or_msgtype=_NOTSET,
+                        controls=None, *,
+                        result=_NOTSET, matcheddn=_NOTSET, message=_NOTSET,
+                        referrals=_NOTSET, name=None, value=None,
+                        defaultClass: Optional[type['ExtendedResult']] = None,
+                        msgid=_NOTSET, msgtype=_NOTSET,
+                        responseName=_NOTSET, encodedResponseValue=_NOTSET,
+                        **kwargs):
+    """
+    Implements both old and new API:
+    __init__(self, responseName, encodedResponseValue)
+    and
+    __init__/__new__(self, msgid, msgtype, controls=None, *,
+                     result, matcheddn, message, referrals,
+                     defaultClass=None, **kwargs)
+    """


Maybe we can create a temporary transitional class? Like ExtendedResultOrSomething(ldap.response.ExtendedResult): with the new implementation? And in 5.0, we can swap it with the existedExtendedResponse

But I don't have a hard opinion here. If this is temporary code that dies in 5.0/6.0 anyway, maybe the current approach is fine?

encukou reviewed Mar 24, 2022

View reviewed changes

mistotebe force-pushed the response_api branch from f378ea1 to cd712ab Compare March 25, 2022 17:05

mistotebe force-pushed the response_api branch 5 times, most recently from abb123e to 756fba2 Compare April 6, 2022 12:15

mistotebe mentioned this pull request Jul 21, 2023

asyncio support, or SansIO support #526

Open

mistotebe force-pushed the response_api branch from 756fba2 to a35462a Compare April 11, 2024 17:22

mistotebe force-pushed the response_api branch from a35462a to 90652b1 Compare September 4, 2024 10:37

mistotebe force-pushed the response_api branch 4 times, most recently from 6b8f56e to 12c3b61 Compare December 12, 2024 13:26

mistotebe mentioned this pull request Feb 4, 2025

add ldappool module #582

Closed

droideck mentioned this pull request Aug 8, 2025

When a new version will be released overall ? #591

Open

droideck reviewed Aug 20, 2025

View reviewed changes

mistotebe added 4 commits August 20, 2025 11:51

Control and extop rework

5799b7c

Draft new response handling API

6d0a4ed

wip

1d6a938

TMP: rich support

397e7b1

mistotebe force-pushed the response_api branch from 12c3b61 to 9e878b4 Compare August 20, 2025 10:52

mistotebe force-pushed the response_api branch from 9e878b4 to a40c7d4 Compare August 20, 2025 12:03

Syncrepl session handling

9de48a9

mistotebe force-pushed the response_api branch from a40c7d4 to 9de48a9 Compare August 20, 2025 13:34

droideck reviewed Aug 20, 2025

View reviewed changes

Draft: new response handling API #464

Are you sure you want to change the base?

Draft: new response handling API #464

Uh oh!

Conversation

mistotebe commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droideck commented Aug 6, 2025

Uh oh!

mistotebe commented Aug 8, 2025

Uh oh!

spaceone commented Aug 8, 2025

Uh oh!

mistotebe commented Aug 12, 2025

Uh oh!

spaceone commented Aug 13, 2025

Uh oh!

mistotebe commented Aug 13, 2025

Uh oh!

droideck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mistotebe Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droideck commented Aug 20, 2025

Uh oh!

mistotebe commented Aug 20, 2025

Uh oh!

droideck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mistotebe commented Mar 24, 2022 •

edited

Loading

mistotebe Aug 20, 2025 •

edited

Loading