search

sympa-community / sympa

Sympa, Mailing List Management Software
https://www.sympa.community/sympa
GNU General Public License v2.0
244 stars 96 forks source link

Inclusion from LDAP data sources supports RFC 2696 Paged Results control (#57) #1733

Closed farialima closed 10 months ago

farialima commented 11 months ago

Fix for #57

It includes a new configuration setting "page size" that's used both to enable paging, and to define the page size. I tried to integrate a description in the setting, as I think it's good to keep the documentation in/next to the functionality, but I'd suppose that the description/documentation can be improved, feedback really welcome on this.

Now, The code for the LDAP data sources is quite convoluted, difficult to read, etc... It took me a looong time to understand that "two-level" was practically working out of the box once I had implemented "one-level", LOL). I've the feeling that it could be simplified. I've entered a different ticket on this: #1738 , as this is clearly not part of #57 .

ikedas commented 11 months ago

There are a few things I'm not 100% happy in my implementation:

  • it is not implemented for the two-level LDAP data source, only for one-level. It would be more complex for the two-level (the current code is a little complex right now, it could/should be refactored first to get paging in place), and I don't have an environment where to test it. I think it's OK to merge it only for LDAP one-level, as the main use case is Active Directory which has a "SizeLimit" that cannot be configured, AFAIK, or at least not easily, and thus is easier to make work on the client. But maybe the maintainer would disagree...

What is the cause of the inability to apply pagination to LDAP two-level data source?

I think it would be better to be able to do it because of less astonishments for users.

  • Ideally, the new code should be in DatabaseDriver::LDAP instead, but it would be more complex - database drivers don't control the returned dataset so a whole class would need to be implemented, I think. It would be much more code. So I think it's OK, even better, to have it in the DataSource::LDAP class

I agree. In fact, it does not seem that pagination would be necessary for LDAP use by Sympa other than as a data source.

ikedas commented 10 months ago

I got it.

_load_next() of Sympa::DataSource::LDAP may be called multiple times. This behavior is useful for implementing two-level data source: Multiple search operation may be executed repeatedly in single connection.

Your change for pagination uses this mechanism to repeatedly read multiple pages. As a result, it breaks the two-level data source.

@farialima , I would like to make one suggestion. How about once I refactor the current code and change it to use callbacks, you can submit your changes to it again?

farialima commented 10 months ago

I got it.

_load_next() of Sympa::DataSource::LDAP may be called multiple times. This behavior is useful for implementing two-level data source: Multiple search operation may be executed repeatedly in single connection.

Your change for pagination uses this mechanism to repeatedly read multiple pages. As a result, it breaks the two-level data source.

Yes, exactly.

I think I have it working, but I don't have a test setup yet for testing it... will take a few days.

@farialima , I would like to make one suggestion. How about once I refactor the current code and change it to use callbacks, you can submit your changes to it again?

I'm really not sure that using a callback improves things, but I will look at it.

farialima commented 10 months ago

(...) Your change for pagination uses this mechanism to repeatedly read multiple pages. As a result, it breaks the two-level data source.

I think it [implementing the two-level LDAP data source] would be better to be able to do it because of less astonishments for users.

I think I have it working, but I don't have a test setup yet for testing it... will take a few days.

now done - ready for review

@farialima , I would like to make one suggestion. How about once I refactor the current code and change it to use callbacks, you can submit your changes to it again?

I'm really not sure that using a callback improves things, but I will look at it.

I was indeed able to implemented two-level LDAP with very few changes, so I'd rather not refactor thing, because really the changes are minimal. But I've entered #1738 as a follow-up, I'll look at it

ikedas commented 10 months ago

OK. If we confirm that existing unit test for LDAP2 without pagination passes, i.e. at least the changes won't break existing feature, this PR may be merged.

ikedas commented 10 months ago

@farialima , I have one more question.

Currently, unless the administrator sets pagesize explicitly, Sympa might not retrieve entire results from the server that enforces pagination. How about the pagesize parameter has some positive default value? (I think this value would be better to be smaller than MaxPageSize setting of AD.)

farialima commented 10 months ago

@farialima , I have one more question.

Currently, unless the administrator sets pagesize explicitly, Sympa might not retrieve entire results from the server that enforces pagination. How about the pagesize parameter has some positive default value? (I think this value would be better to be smaller than MaxPageSize setting of AD.)

I've been hesitating indeed to automatically use paging if it's available. But I don't find a generic way to query the MaxPageSize of AD, so I couldn't get to decide what default value to use... I agree it's one more small complexity [for the administrator], but at least at first, I'd rather make it manual.. If you have any idea of how it could work, I'd be a taker, but I haven't found a way to do it, in a way I would find reliable enough...

ikedas commented 10 months ago

I don't have a good idea either... All right, let's start with manual onfiguration first.