milux / ctldap

LDAP Wrapper for ChurchTools
GNU General Public License v3.0
12 stars 8 forks source link

Fetch Persons and Groups paginated #44

Closed miltechniks closed 1 year ago

miltechniks commented 1 year ago

Fetch all persons and groups via the paginated API to fix the Issue with Churchtools 3.100, where the limit -1 isn't possible anymore for groups, see #43

djschilling commented 1 year ago

just to clarify. page=-1 never really worked and was never intended to return back all groups.

miltechniks commented 1 year ago

@djschilling I think you mean limit=-1? When I try it on the api-Page, I get an Error back from the MariaDB Server, so there seems to be still an Issue somehow for me that this value is not handled correct. As well, when the number is too big (e.g. limit=200), the returned message is "message": "Die Nummer kann maximal {max} sein.", so there seems to be an Issue as well, as the {max} isn't defined here. Same with person.

Shyru commented 1 year ago

I can confirm the issue, I also got the Internal Server Errors from ChurchTools since the last update. The fix from this pull-request worked fine and login was possible again afterwards. So thank you very much for the fix! ❤️

milux commented 1 year ago

@miltechniks Thanks for your PR. Unfortunately, it will cause very poor performance (speed-wise) because you are serializing the paginated requests, especially for large setups! It will also - at any rate - cause very heavy server load because the per-request overhead in CT is massive, that was the very reason for my hackish approach!

@djschilling As suggested, I would again like to kindly ask you whether you can increase the limit further or make it somehow configurable via settings. Forced pagination just doesn't make any sense here, except for protecting server resources. A limit of 10k or 100k would seem fine and sensible to me for that.

milux commented 1 year ago

just to clarify. page=-1 never really worked and was never intended to return back all groups.

It used to work, although you're absolutely right in saying that this was unintended behavior never meant to work. :wink: It used to return all values minus one, which reduced the necessary number of requests to 2.

milux commented 1 year ago

Fixed in 3.0.2, closing.