[Ldap] added Ldap entry rename for ExtLdap adapter #20390

fruitwasp · 2016-11-02T14:51:51Z

Q	A
Branch?	"master"
Bug fix?	no
New feature?	yes
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

xunto · 2016-11-02T15:07:36Z

src/Symfony/Component/Ldap/Tests/Adapter/ExtLdap/LdapManagerTest.php

+        $result = $this->executeSearchQuery(1);
+        $entry = $result[0];
+
+        $this->assertEquals($entry->getAttribute('cn'), 'Mr Robot');


Entry::getAttribute returns an array. So it should fail.

fruitwasp · 2016-11-03T09:03:35Z

@csarrazi

csarrazi

As mentioned in my line comment, I would actually move the rename logic in the update method. The goal of the add is basically to copy / save a new object, whereas update aims to provide means to update an existing object (which includes renaming an entry).

csarrazi · 2016-11-03T09:48:11Z

src/Symfony/Component/Ldap/Adapter/ExtLdap/EntryManager.php

+     *
+     * @throws LdapException
+     */
+    public function rename(Entry $entry, $newRdn, $newParent, $shouldDeleteOldRdn = true)


More generally speaking, I'm not really fond of simply exposing the same API as the ldap PHP extension. This would actually go against the goal of this component, which aims to provide a layer of abstraction on top of multiple implementations of Ldap (the PHP Ldap extension being the first available adapter).

Instead, I would improve the logic in the update() method, and add more logic in the Entry class in order to be able to compute a changeset, or simply flag the Entry instance as "dirty".

Currently, the logic for the update() method is pretty poor, and does not check whether the attributes have changed or not, or if the DN has changed or not. This could be pretty much improved, for Symfony 3.3.

csarrazi · 2016-11-08T08:32:07Z

In short, 👍 on the intent of the PR, 👎 on the actual implementation, which should not reflect the implementation details.

csarrazi

Sorry for going back and forth between designs. I think you were right to add a new rename method, but it needs a few tweaks.

Instead, I would do the following:

The update method should be kept as is, and should be used only to update an existing entry's attributes.
A rename method should be added, to explicitly support renaming, taking only the old RDN, the new RDN, and an optional $deleteRdn boolean argument to remove the old entry or not. The $deleteRdn attribute should be true by default, as in most cases we expect the old object to disappear when renaming an entry (think about the OS mv or move command, or an UPDATE statement in SQL).
If you want to create a new entry from an existing one with altered attributes, I would actually recommend creating a new entry and passing the original entry's attributes through $originalEntry->getAttributes() and specifying a new DN in the new entry's constructor, perform further modifications, and then use the EntryManager::save() method.

As I said before, I do not want the LDAP component to be a simple reflection of PHP's ldap extension. We want something which is higher level, and eases developers' life.

With this design, no need to track the objects' changes, except just to prevent updating an entry if it not flagged as "dirty" (in the EntryManager::update() method).

In this case, I would rename the $attributesState property to $dirty, and simply store a boolean value instead (no need for class constants for something binary).

xunto · 2016-12-16T23:50:44Z

@csarrazi we need to track objects' changes anyway because now if you have only read access to some properties, you won't be able to change properties you have access to by changing entry object and persisting it.

As example;

        $records = $ldap->query()->execute();
        $record = $records[0];
        $record->setAttribute("uid", ['data']);
        $ldap->getEntryManager()->update($record);

Won't work even if you have write access to uid property because it tries to update all properties this object have.

I used this workaround:

        $records = $ldap->query()->execute();
        $record = new Entry($records[0]->getDn());
        $record->setAttribute("uid", ['data']);
        $ldap->getEntryManager()->update($record);

It's not place for bug reports but it's connected to the topic :)

fruitwasp · 2017-01-10T11:18:09Z

@csarrazi I have implemented the rename functionality as you suggested. About flagging the Entry as dirty, I think that's one worth to discuss in another pull request.

csarrazi

We're nearly there! :)

Could you just add one more test in LdapManagerTest, and fix the method argument in the interface class?

We should have an additional case with $removeOldRdn = false for checking that the old entry has indeed is still there in this case.

Thanks a lot for that PR, and your patience, @fruitwasp!

csarrazi · 2017-01-10T12:16:05Z

src/Symfony/Component/Ldap/Tests/Adapter/ExtLdap/LdapManagerTest.php

+
+        $result = $this->executeSearchQuery(1);
+        $entry = $result[0];
+        $this->assertEquals($entry->getAttribute('cn')[0], 'Kevin');


We should also check that the old DN no longer exists.

The old DN no longer exists if there's only 1 entry. If more than 1 Entry is found, the assertCount check will fail in executeSearchQuery, therefore I don't think it necessary to have another check.

Good point. I agree

Another thing. You should make sure that the test returns the directory in its original state. If you rename the entry, don't forget to rename it back to its original value after the test.

csarrazi · 2017-01-10T12:17:02Z

src/Symfony/Component/Ldap/Adapter/EntryManagerInterface.php

+     * @param string $newRdn
+     * @param bool   $deleteOldRdn
+     */
+    public function rename(Entry $entry, $newRdn, $deleteOldRdn = true);


Rename the $deleteOldRdn to $removeOldRdn so it is the same as in the implementation class.

csarrazi

Don't forget to reinitialise the directory to its original state. Tests should be independent from each other, and should have no impact. Thus, you should run rename() again to restore the entry in the first test, and run remove() to remove the newly created entry.

csarrazi · 2017-01-10T13:39:14Z

src/Symfony/Component/Ldap/Tests/Adapter/ExtLdap/LdapManagerTest.php

+
+        $result = $this->executeSearchQuery(1);
+        $entry = $result[0];
+        $this->assertEquals($entry->getAttribute('cn')[0], 'Kevin');


Another thing. You should make sure that the test returns the directory in its original state. If you rename the entry, don't forget to rename it back to its original value after the test.

csarrazi · 2017-01-10T13:40:27Z

src/Symfony/Component/Ldap/Tests/Adapter/ExtLdap/LdapManagerTest.php

+        $entryManager = $this->adapter->getEntryManager();
+        $entryManager->rename($entry, 'cn=Kevin', false);
+
+        $this->executeSearchQuery(2);


Same thing. Don't forget to make sure that the new entry is removed after checking that you have two of them.

We do not want tests to put the directory in a state which differs from the startup.

somehow the test fails on this executeSearchQuery(2) even though the old RDN shouldn't be removed. Any idea, @csarrazi?

https://travis-ci.org/symfony/symfony/jobs/190618432#L3686

Maybe instead of $this->executeSearchQuery(2), try to copy part of the logic, and use something to print what you have in the test.

My guess is that there is something wrong with the parent DN or the RDN, which may not be set to the previous value.

I would recommend you try to run the tests locally (by using the csarrazi/symfony-ldap docker image, for example), and check what you have in the LDAP directory using var_dump(), for example.

FYI, the container can be run as follows:

docker run -d --name ldap -p 389:389 -v "/path/to/symfony/source/code/src/Symfony/Component/Ldap/Tests/Fixtures/data:/init-ldap.d" -e "LDAP_SUFFIX=dc=symfony,dc=com" -e "LDAP_ROOTDN=cn=admin,dc=symfony,dc=com" -e "LDAP_ROOTPW=symfony" csarrazi/symfony-ldap

Then you can run the tests as follows (from /path/to/symfony/source/code):

LDAP_HOST=localhost LDAP_PORT=389 ./phpunit src/Symfony/Component/Ldap

Cheers!

@csarrazi Thanks for that. I misunderstood what not deleting the old RDN actually does. All tests pass now.

csarrazi · 2017-01-11T15:07:32Z

@fruitwasp you're welcome! 👍

xabbuh · 2017-01-12T04:54:55Z

src/Symfony/Component/Ldap/Adapter/EntryManagerInterface.php

+     * @param string $newRdn
+     * @param bool   $removeOldRdn
+     */
+    public function rename(Entry $entry, $newRdn, $removeOldRdn = true);


Do we still consider the API of the LDAP component to be internal and subject to change? Otherwise, this change is a BC break.

The readme states that the component is stable since Symfony 3.1 (https://github.com/symfony/ldap/blob/3.2/README.md). So I guess this is a BC break.

Hum, this interface should still actually be marked as private, and was only added in 3.2.
Write support is only in its infancy, and is not completely mature yet, so we may need to add new methods at later points (move(Entry $entry, $newRoot) for example).

We could create another interface which extends the EntryManagerInterface or a separate interface for newer methods, which could be merged in the main interface for Symfony 4.0 (See what was done for LdapClientInterface and LdapInterface, for example).

On the other hand, the query part is stable, and should not change in the upcoming months.

Maybe a MoveEntryInterface would be a good solution?

interface MoveEntryInterface { public function move(Entry $entry, $newRoot); }

Which should be merged in the main interface for Symfony 4.0.

Yup, I do think so. @xabbuh what do you think about this?

move() instead of rename()?

move() is something else, and not related to rename().

So the correct interface would be something like RenameEntryInterface. We could add MoveEntryInterface at a later time, when we actually implement moving an entry to a new parent tree.

fruitwasp · 2017-01-13T12:37:32Z

@csarrazi @xabbuh fixed conflicts with the master branch, implemented the RenameEntryInterface and rebased. Ready for merge?

xabbuh · 2017-01-13T12:46:28Z

@fruitwasp I am not very familiar with LDAP, but my BC concerns have been addressed.

csarrazi · 2017-01-13T12:47:19Z

src/Symfony/Component/Ldap/Adapter/RenameEntryInterface.php

+use Symfony\Component\Ldap\Entry;
+
+/**
+ * @author Kevin Schuurmans <kevin.schuurmans@freshheads.com>


Maybe just add a mention that this interface will be deprecated for 4.0, and merged with EntryManagerInterface.

fabpot · 2017-01-18T05:51:01Z

Thank you @fruitwasp.

…uitwasp) This PR was merged into the 3.3-dev branch. Discussion ---------- [Ldap] added Ldap entry rename for ExtLdap adapter | Q | A | ------------- | --- | Branch? | "master" | Bug fix? | no | New feature? | yes | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- 2d8eeb2 [LDAP] implemented LDAP entry rename for ExtLdap adapter

…maid) This PR was merged into the 4.0-dev branch. Discussion ---------- [Ldap] Remove the RenameEntryInterface interface | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20390 | License | MIT | Doc PR | / Commits ------- 5d35184 Remove the RenameEntryInterface interface

This PR was merged into the 3.3 branch. Discussion ---------- [Ldap] Fix deprecated message | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20390 #23402 | License | MIT | Doc PR | / ``RenameEntryInterface`` is **deprecated** in 3.3 and **removed/merged** in 4.0. Commits ------- bbd0c7f Fix deprecated message

carsonbot added Status: Needs Review Ldap Feature labels Nov 2, 2016

xunto reviewed Nov 2, 2016

View reviewed changes

csarrazi suggested changes Nov 3, 2016

View reviewed changes

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

csarrazi suggested changes Dec 14, 2016

View reviewed changes

csarrazi suggested changes Jan 10, 2017

View reviewed changes

csarrazi reviewed Jan 10, 2017

View reviewed changes

xabbuh reviewed Jan 12, 2017

View reviewed changes

csarrazi reviewed Jan 13, 2017

View reviewed changes

[LDAP] implemented LDAP entry rename for ExtLdap adapter

2d8eeb2

fabpot merged commit 2d8eeb2 into symfony:master Jan 18, 2017

fruitwasp deleted the ldap-entry-rename branch January 18, 2017 10:10

javiereguiluz mentioned this pull request Feb 1, 2017

Ldap component added RenameEntryInterface symfony/symfony-docs#7438

Closed

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

maidmaid mentioned this pull request Jul 5, 2017

[Ldap] Remove the RenameEntryInterface interface #23402

Merged

maidmaid mentioned this pull request Jul 6, 2017

[Ldap] Fix deprecated message #23422

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Ldap] added Ldap entry rename for ExtLdap adapter #20390

[Ldap] added Ldap entry rename for ExtLdap adapter #20390

fruitwasp commented Nov 2, 2016 •

edited

Loading

xunto Nov 2, 2016

fruitwasp commented Nov 3, 2016

csarrazi left a comment

csarrazi Nov 3, 2016 •

edited

Loading

csarrazi commented Nov 8, 2016

csarrazi left a comment •

edited

Loading

xunto commented Dec 16, 2016 •

edited

Loading

fruitwasp commented Jan 10, 2017

csarrazi left a comment

csarrazi Jan 10, 2017

fruitwasp Jan 10, 2017

csarrazi Jan 10, 2017

csarrazi Jan 10, 2017

csarrazi Jan 10, 2017

csarrazi left a comment

csarrazi Jan 10, 2017

csarrazi Jan 10, 2017

fruitwasp Jan 11, 2017 •

edited

Loading

csarrazi Jan 11, 2017

fruitwasp Jan 11, 2017 •

edited

Loading

csarrazi commented Jan 11, 2017

xabbuh Jan 12, 2017

jvandesande Jan 12, 2017

csarrazi Jan 12, 2017

jvandesande Jan 12, 2017

csarrazi Jan 12, 2017

xabbuh Jan 13, 2017

csarrazi Jan 13, 2017

fruitwasp commented Jan 13, 2017

xabbuh commented Jan 13, 2017

csarrazi Jan 13, 2017

fruitwasp Jan 13, 2017

fabpot commented Jan 18, 2017

[Ldap] added Ldap entry rename for ExtLdap adapter #20390

[Ldap] added Ldap entry rename for ExtLdap adapter #20390

Conversation

fruitwasp commented Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

fruitwasp commented Nov 3, 2016

csarrazi left a comment

Choose a reason for hiding this comment

csarrazi Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

csarrazi commented Nov 8, 2016

csarrazi left a comment • edited Loading

Choose a reason for hiding this comment

xunto commented Dec 16, 2016 • edited Loading

fruitwasp commented Jan 10, 2017

csarrazi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csarrazi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fruitwasp Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fruitwasp Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

csarrazi commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fruitwasp commented Jan 13, 2017

xabbuh commented Jan 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Jan 18, 2017

fruitwasp commented Nov 2, 2016 •

edited

Loading

csarrazi Nov 3, 2016 •

edited

Loading

csarrazi left a comment •

edited

Loading

xunto commented Dec 16, 2016 •

edited

Loading

fruitwasp Jan 11, 2017 •

edited

Loading

fruitwasp Jan 11, 2017 •

edited

Loading