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] Allow adding and removing values to/from multi-valued attributes #21856

Merged
merged 1 commit into from Apr 4, 2018

Conversation

Projects
None yet
5 participants
@jean-gui
Contributor

jean-gui commented Mar 3, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

EntryManagerInterface::update(Entry $entry) is extremely slow in some specific cases such as adding or removing members to or from huge groupOfNames if you also enable the memberOf overlay in OpenLDAP. Disabling memberOf does make things a lot better, but it is still slow compared to inserting/removing only the elements you want.

This PR adds two methods to Symfony\Component\Ldap\Adapter\ExtLdap\EntryManager taking advantage of ldap_mod_add and ldap_mod_del.

I thought about using them directly in the update method, but since you need to know what values to add and remove, it would be necessary to retrieve the old values from LDAP.

I'm also unsure whether these two methods should be in an interface. I think that adding them to EntryManagerInterface would break BC, but I could create another interface, similarly to RenameEntryInterface.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Mar 4, 2017

ping @csarrazi

@nicolas-grekas nicolas-grekas added this to the 3.x milestone Mar 4, 2017

@csarrazi

This comment has been minimized.

Contributor

csarrazi commented Mar 5, 2017

I think that we could improve this by introducing a new API for handling changes.

Instead of using separate methods for different things, we should rather use a Command pattern, and provide a set of commands which could be implemented without breaking the interface each time we need to implement something.

This way, we could have a runCommand() method in the EntryManager, with a list of operations:

  • UpdateEntry
  • AddAttributes
  • RemoveAttributes
  • RenameEntry

This would support the current set, and we could deprecate the other methods afterwards.

What do you think about this @nicolas-grekas ?

@csarrazi

This comment has been minimized.

Contributor

csarrazi commented Mar 5, 2017

By the way, this means that the add(), update(), remove() and rename() methods should all be deprecated, in favor of the runCommand() (Or executeCommand()) method.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Apr 17, 2017

ping @jean-gui

@jean-gui

This comment has been minimized.

Contributor

jean-gui commented Apr 18, 2017

I was actually waiting for your response to @csarrazi's ping. I'm not sure I have the necessary skills or time to implement this new API though. I'll try to, but am not making any promises.

@csarrazi

This comment has been minimized.

Contributor

csarrazi commented Apr 18, 2017

@jean-gui want me to take over that one?

@csarrazi

This comment has been minimized.

Contributor

csarrazi commented Apr 18, 2017

I won't have much time before next week, though.

@symfony/deciders What do you think about the suggestion, btw?

@jean-gui

This comment has been minimized.

Contributor

jean-gui commented Apr 18, 2017

@csarrazi If you can, sure!

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Apr 18, 2017

Let's see how it goes for 3.4 :)

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Sep 26, 2017

Status: needs work

@fabpot

This comment has been minimized.

Member

fabpot commented Oct 1, 2017

@csarrazi Do you think you will have time to work on this one soon? If not, anybody else?

@csarrazi

This comment has been minimized.

Contributor

csarrazi commented Oct 2, 2017

Hi @fabpot. Unfortunately I won't have time to work on this anytime soon. I can spend some time reviewing PRs, but unfortunately I have little time to do anything else.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Oct 8, 2017

Moving to 4.1. Rebase on master might be needed, where PHP 7.1 features can be used btw.

@nicolas-grekas nicolas-grekas modified the milestones: 3.4, 4.1 Oct 8, 2017

@fabpot

This comment has been minimized.

Member

fabpot commented Mar 31, 2018

Anyway willing to take over this one? If not, I'm going to close.

@jean-gui

This comment has been minimized.

Contributor

jean-gui commented Apr 3, 2018

@fabpot If no one is willing to tackle the big refactoring suggested by @csarrazi, perhaps this PR could be considered for merging.

@csarrazi

This comment has been minimized.

Contributor

csarrazi commented Apr 4, 2018

We can do that. Like I mentioned before, I no longer contribute to Symfony on a regular basis.

One thing though. I would change the method name to removeAttributeValues / addAttributeValues.

Second, this PR should be updated to take into account changes in coding standards for Symfony 4 (PHP > 7.1.3 is used, so scalar type hinting can be used now).

By the way, $values should be type hinted as an array.

@jean-gui

This comment has been minimized.

Contributor

jean-gui commented Apr 4, 2018

I'll try to do that today.

@fabpot

fabpot approved these changes Apr 4, 2018

some small comments (mainly coding standard).

*
* @param Entry $entry
* @param string $attribute
* @param array $values

This comment has been minimized.

@fabpot

fabpot Apr 4, 2018

Member

The 3 @param entries can be removed (we don't add those if they don't add something that is not part of the PHP method signature already).

@@ -67,6 +67,47 @@ public function remove(Entry $entry)
}
}
/**
* Adds values to an entry's multi-valued attribute from the Ldap server.

This comment has been minimized.

@fabpot

fabpot Apr 4, 2018

Member

LDAP

if (!@ldap_mod_add($con, $entry->getDn(), array($attribute => $values))) {
throw new LdapException(sprintf('Could not add values to entry "%s", attribute %s: %s', $entry->getDn(),
$attribute, ldap_error($con)));

This comment has been minimized.

@fabpot

fabpot Apr 4, 2018

Member

should be on one line.

This comment has been minimized.

@fabpot

fabpot Apr 4, 2018

Member

missing dot at the end of the error message.

}
/**
* Removes values from an entry's multi-valued attribute from the Ldap server.

This comment has been minimized.

@fabpot

fabpot Apr 4, 2018

Member

LDAP

*
* @param Entry $entry
* @param string $attribute
* @param array $values

This comment has been minimized.

@fabpot

fabpot Apr 4, 2018

Member

same as above

if (!@ldap_mod_del($con, $entry->getDn(), array($attribute => $values))) {
throw new LdapException(sprintf('Could not remove values from entry "%s", attribute %s: %s',
$entry->getDn(),
$attribute, ldap_error($con)));

This comment has been minimized.

@fabpot

fabpot Apr 4, 2018

Member

should also be on one line, and dot missing at the end of the error message.

@jean-gui

This comment has been minimized.

Contributor

jean-gui commented Apr 4, 2018

Done. Should I add those new methods to EntryManagerInterface? Would that be a BC break?

@fabpot

This comment has been minimized.

Member

fabpot commented Apr 4, 2018

Adding them to the interface would be a BC break.

@fabpot

This comment has been minimized.

Member

fabpot commented Apr 4, 2018

One last thing before merging: can you add a note about these 2 new methods in the composer CHANGELOG file?

@jean-gui

This comment has been minimized.

Contributor

jean-gui commented Apr 4, 2018

Done (if src/Symfony/Component/Ldap/CHANGELOG.md is the file you meant).

I should probably submit a PR for the doc as well.

@fabpot

fabpot approved these changes Apr 4, 2018

A PR on the docs would be the cherry on the cake :)

@fabpot

This comment has been minimized.

Member

fabpot commented Apr 4, 2018

Thank you @jean-gui.

@fabpot fabpot merged commit fa9db29 into symfony:master Apr 4, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 4, 2018

feature #21856 [LDAP] Allow adding and removing values to/from multi-…
…valued attributes (jean-gui)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[LDAP] Allow adding and removing values to/from multi-valued attributes

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT

`EntryManagerInterface::update(Entry $entry)` is extremely slow in some specific cases such as adding or removing members to or from huge groupOfNames if you also enable the memberOf overlay in OpenLDAP. Disabling memberOf does make things a lot better, but it is still slow compared to inserting/removing only the elements you want.

This PR adds two methods to Symfony\Component\Ldap\Adapter\ExtLdap\EntryManager taking advantage of ldap_mod_add and ldap_mod_del.

I thought about using them directly in the update method, but since you need to know what values to add and remove, it would be necessary to retrieve the old values from LDAP.

I'm also unsure whether these two methods should be in an interface. I think that adding them to EntryManagerInterface would break BC, but I could create another interface, similarly to RenameEntryInterface.

Commits
-------

fa9db29 Allow adding and removing values to/from multi-valued attributes

jean-gui added a commit to jean-gui/symfony-docs that referenced this pull request Apr 4, 2018

jean-gui added a commit to jean-gui/symfony-docs that referenced this pull request Apr 10, 2018

jean-gui added a commit to jean-gui/symfony-docs that referenced this pull request Apr 10, 2018

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Apr 16, 2018

minor #9553 Documented how to add or remove values of multi-valued at…
…tributes (jean-gui)

This PR was merged into the master branch.

Discussion
----------

Documented how to add or remove values of multi-valued attributes

Related to symfony/symfony#21856

<!--

If your pull request fixes a BUG, use the oldest maintained branch that contains
the bug (see https://symfony.com/roadmap for the list of maintained branches).

If your pull request documents a NEW FEATURE, use the same Symfony branch where
the feature was introduced (and `master` for features of unreleased versions).

-->

Commits
-------

40df041 Documented how to add or remove values of multi-valued attributes

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment