[Feature] Implement true paging support with VLV (Virtual List View) Request

[stevebauman]

@KarimGeiger has shown an implementation of a true LDAP pagination request with the ability to set an offset to retrieve a slice of a whole search result from the LDAP server.

Resources:

• https://www.php.net/manual/en/ldap.controls.php
• https://opensource.apple.com/source/apache_mod_php/apache_mod_php-156/php/ext/ldap/tests/ldap_search_sort_controls.phpt
• https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-adts/2667823e-dfd3-422b-b055-40e5b8779b0b

[KarimGeiger]

Just looked at it again and saw that the example also includes sorting, so that another thing that may be quite useful to have.

[stevebauman]

Hey @KarimGeiger! I've just pushed to master with an implementation. Would you be able to test it out whenever you can?

I've named the method slice() since paginate() is already taken (I may rename these methods in v3, haven't decided yet).

Here's a usage example:

use LdapRecord\Models\ActiveDirectory\User;

/** @var \LdapRecord\Query\Slice **/
$slice = User::slice($perPage = 100, $page = 1, $orderBy = 'cn');

var_dump($slice->items());

Also, it turns out applying sorting is mandatory when requesting a virtual list view:

https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-adts/2667823e-dfd3-422b-b055-40e5b8779b0b

This control can only be used if the LDAP_SERVER_SORT_OID (section 3.1.1.3.4.1.13) control is also specified.

So an orderBy argument is available to be set as the last parameter.

Let me know your thoughts and if you have any issues with it! 👍

[KarimGeiger]

That's awesome, thanks for that super quick implementation! Unfortunately I don't have time to test today, but I will definitely do so on Monday.

I do however have one thing I'm noticing while looking at the example right away, regarding the sorting. I understand that it's needed so I'm not sure if there's a better way to do it, but I'm not super fond of the $orderBy parameter:

For one, I would currently be restricted to sorting ascending, so we would need yet another parameter to introduce a direction.

Additionally, if you would have previously defined sorting using the orderBy() method, this would either overwrite the previous call or need to ignore the third parameter. Either way is not quite optimal as it can lead to unexpected behavior. For example, if I'm not aware of this mechanic, I may forget supplying the third parameter and suddenly it sorts by cn even though I did define another sorting previously.

Maybe a better way to approach this would be to check if a sorting order has previously been defined, and if it has not, it will always just default to cn without the option to change it via a parameter. This would reduce confusion and if you would like to still change it, you can do it by first calling orderBy().

[stevebauman]

Ah great catch @KarimGeiger! Thanks for looking through it 👍

I've updated the implementation so an existing order by will not be overridden by the method. I'll wait for your test before I make the release.

[KarimGeiger]

Okay, we now got around to test it. Unfortunately, we immediately got the following error when running the slice() method (and orderBy() didn't work either, it just ignored the parameter completely):

ldap_search(): Search: Critical extension is unavailable

Luckily, we found the reason for that error. As the error message suggests, this has nothing to do with some extension being unavailable. Turns out, the problem was that the OU we were searching in had more than 10,000 entries, which exceeds the MaxTempTableSize variable defined on our AD server, which can result in this error being shown. So that was fun to debug... Maybe it's worth including that in the documentation to save some future dev a few hours of googling.

But, as soon as we switched to an OU with less objects, your implementation worked great and without issues 👍

So, to recap: If you want pagination in Active Directory, you need to find that sweet spot of so many objects that it's worth pagination, but not so many that the server gives up in its default configuration. Fortunately it looks like that variable is configurable, so at least there is a way around it.

Thanks again for the quick implementation, apart from AD being AD everything worked fine, so I'd call it a success!

[stevebauman]

Thanks for your detailed response @KarimGeiger, I really appreciate it! 🙏 I'm glad it's working.

That's lame that AD has a MaxTempTableSize. I wonder if it's possible to bypass the warning or the value by setting the isCritical parameter in the LDAP controls to false?

LdapRecord/src/Query/Builder.php

Line 622 in e9c9363

$this->addControl(LDAP_CONTROL_VLVREQUEST, true, [

Ex:

$this->addControl(LDAP_CONTROL_VLVREQUEST, $isCritical = false, [ 

Not sure if this has any effect. Would you perhaps be able to modify the file and see ? It will be in:

vendor/directorytree/ldaprecord/src/Query/Builder.php

[KarimGeiger]

We already played around with that parameter while debugging the error message and didn't find any difference whether it's set to true or false. From our understanding this parameter is only relevant if the server doesn't understand these features, so it makes sense that it didn't do anything for us in that case.

[stevebauman]

Ok great, thanks for validating that for me @KarimGeiger. I'll close this issue out once I create a new release 👍

[KarimGeiger]

Hi @stevebauman, we just noticed one more thing when querying for only one result while using pagination. The affected line is:

LdapRecord/src/Query/Builder.php

Line 599 in e9c9363

$objects = $index >= $total

The same happens if the last page only contains one result, for example for 41 results with $perPage = 20, the third page will return 0 results, omitting 1.

In this case, the library thinks we have reached the end of the record and therefore discards the only result. We have found that adjusting the line to the following helps to avoid this issue:

$objects = $page > ceil($total / $perPage)

[stevebauman]

Ah great catch thanks @KarimGeiger! 🙏 This has just been resolved.

FYI I've adjusted the parameters to follow the forPage() method instead of them being swapped, i.e.:

Before:

User::slice($perPage = 100, $page = 1);

After:

User::slice($page = 1, $perPage = 100);

I'm creating a new release right now, thanks again for your help! 😄

[stevebauman]

v2.11.0 has just been released 🎉

[KarimGeiger]

Thanks for letting me know of the parameter change. Will check out the latest release shortly. Great work with the library 👍