Skip to content
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.

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

[Ldap] Implement pagination #29495

Merged
merged 1 commit into from Mar 31, 2019

Conversation

Projects
None yet
7 participants
@kevans91
Copy link
Contributor

commented Dec 6, 2018

Q A
Branch? master
Bug fix? yno
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? N/A (cannot test at the moment)
Fixed tickets N/A
License MIT
Doc PR N/A

Implement pagination support in the ExtLdap adapter. In a more abstract sense, other adapters should use a query's pageSize option to determine if pagination is being needed. Pagination is required in some environments that frequently query for more results than the remote server is willing to allow.

BC break was avoided by having Query->getResource() return the first result, if available.

A small hack is included to work around php-ldap failing to reset pagination properly; the LDAP_OPT_SERVER_CONTROLS are sent with every request, whether pagination has been 'reset' by sending a 0-page request or not. This appears to be a php-ldap bug that will need to be addressed there, but we can work-around it for now by doing both: setting the 0-page option and unsetting the OID directly. This was resulting in odd results, like queries returning 0 results or returning < server_page_size results for a query that should have returned >= server_page_size.

@kevans91

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

Note that I'm not 100% confident in my assessment of the hack; it works in our environment, though, where we're frequently querying for 6000+ objects against a server that has a 1000-object limit. I will consult with php-ldap folk on that, though, because the advised 'reset' is clearly not enough.

@kevans91 kevans91 force-pushed the kevans91:ldap-pagination branch 5 times, most recently from 52d223d to a610a0f Dec 7, 2018

@kevans91

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

Sorry for the churn- spent some extra time today and got a local test setup going.

The backend in the test configuration must be switched to one that actually supports pagination. I've modified the tests to just skip if we get a failure while querying in these tests, though, in case one needs to run them in an environment that doesn't support pagination.

PHP 7.1 apparently still has issues with resetting the page bits, even with the hack, according to Travis. I'm not sure how to rectify at this point- PHP 7.1 isn't actively supported as of this month, so trying to get it fixed there simply will not be feasible. I could hide those parts behind PHP_MAJOR_VERSION == 7 && PHP_MINOR_VERSION == 1 and we live with the fact that it simply won't work and likely no one will notice... I'm unsure of the best approach to take.

@kevans91

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

I've created php/php-src#3703 to track my progress in addressing the php-ldap bug.

@kevans91 kevans91 force-pushed the kevans91:ldap-pagination branch from a610a0f to ed3dc5c Dec 8, 2018

@kevans91

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2018

After closer assessment of extldap in PHP-7.1, there is no way that we can properly reset state there enough to make non-paged queries work following paged queries on the same connection for all servers for a kind of silly reason: LDAP_OPT_SERVER_CONTROLS in ldap_get_option is unimplemented, so we can't filter the paged result OID out of the current set of server controls. To add to the hilarity, ldap_set_option won't accept an empty array for LDAP_OPT_SERVER_CONTROLS, so even if we could filter it from the current set- we can't set the option if there are no other server controls left. This will be the case nearly 100% of the time anyways, because ldap_control_paged_result clobbers the server controlset when it's called to only the paged control OID.

A fix for this could be adding a fake OID/value/iscritical set. This is an even nastier workaround than I've already implemented, and I'd really prefer to just indicate to users that they should upgrade to a later PHP version if they want to use non-paged queries following paged queries.

@kevans91 kevans91 force-pushed the kevans91:ldap-pagination branch from ed3dc5c to 9d77113 Dec 8, 2018

@chalasr chalasr added this to the next milestone Dec 9, 2018

@kevans91

This comment has been minimized.

Copy link
Contributor Author

commented Dec 29, 2018

Upstream isn't receptive to fixing the broken abstraction, so this possibly will see a small rewrite in the Thursday/Friday timeline to use ldap_search_ext and friends where possible, depending on what's available in PHP 7.1.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

rebase needed please

@kevans91 kevans91 force-pushed the kevans91:ldap-pagination branch from 9d77113 to d798080 Jan 25, 2019

@kevans91

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

Rebased and fixed new style issues; the small rewrite mentioned before won't happen until 7.1 becomes de-supported.

/**
* Resets pagination on the current connection.
*
* @internal

This comment has been minimized.

Copy link
@fabpot

fabpot Mar 4, 2019

Member

not needed as the method is private anyway.

$con = $this->connection->getResource();
$searches = $this->search->getResources();
$count = 0;
foreach ($searches as $search) {

This comment has been minimized.

Copy link
@fabpot

fabpot Mar 4, 2019

Member

Can we keep the old way of counting without pagination to avoid n requests?

This comment has been minimized.

Copy link
@kevans91

kevans91 Mar 4, 2019

Author Contributor

Hmm... sure, but currently without pagination you should only have the single set of results and thus one trip to through the loop and one request. I hadn't done anything special previously because I figured the PHP overhead was fairly minimal.

@fabpot

fabpot approved these changes Mar 31, 2019

@fabpot fabpot force-pushed the kevans91:ldap-pagination branch from d798080 to b963474 Mar 31, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

Thank you @kevans91.

@fabpot fabpot merged commit b963474 into symfony:master Mar 31, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 31, 2019

feature #29495 [Ldap] Implement pagination (kevans91)
This PR was squashed before being merged into the 4.3-dev branch (closes #29495).

Discussion
----------

[Ldap] Implement pagination

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yno
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | N/A (cannot test at the moment)
| Fixed tickets | N/A
| License       | MIT
| Doc PR        | N/A

Implement pagination support in the ExtLdap adapter. In a more abstract sense, other adapters should use a query's pageSize option to determine if pagination is being needed. Pagination is required in some environments that frequently query for more results than the remote server is willing to allow.

BC break was avoided by having Query->getResource() return the first result, if available.

A small hack is included to work around php-ldap failing to reset pagination properly; the LDAP_OPT_SERVER_CONTROLS are sent with every request, whether pagination has been 'reset' by sending a 0-page request or not. This appears to be a php-ldap bug that will need to be addressed there, but we can work-around it for now by doing both: setting the 0-page option *and* unsetting the OID directly. This was resulting in odd results, like queries returning 0 results or returning < server_page_size results for a query that should have returned >= server_page_size.

Commits
-------

b963474 [Ldap] Implement pagination

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.