Fix ldap.MOD_ADD, ldap.MOD_DELETE and ldap.MOD_REPLACE operations on multi-value attribute descriptions #21
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can you include tests?
…-Tim Abbott (mobile)
On Fri, Nov 27, 2020, 12:37 Mike Gabriel ***@***.***> wrote:
------------------------------
You can view, comment on, or merge this pull request online at:
#21
Commit Summary
- fakeldap.py: Fix ldap.MOD_ADD operations for multi-value lists.
- fakeldap.py: Fix ldap.MOD_DELETE operations for multi-value lists
(esp. when only deleting individual values from an multi-value attribute
description.
File Changes
- *M* fakeldap.py
<https://github.com/zulip/fakeldap/pull/21/files#diff-1794df221415c07d72ffbd4e80385d34d326a7c812847d5920c9343023007d74>
(35)
Patch Links:
- https://github.com/zulip/fakeldap/pull/21.patch
- https://github.com/zulip/fakeldap/pull/21.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#21>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAU6NWSRTLT4Y6MTA2PTIYTSSAEYVANCNFSM4UFJGUNA>
.
|
…p. when only deleting individual values from an multi-value attribute description.
…e always stored as tuples in the internal fake LDAP directory.
sunweaver
force-pushed
the
pr/fix-ldapmodify-operations
branch
from
87a15d1 to
96b2f3f
Compare
|
@timabbott unit tests have now been added. I also had to pull in the patch from #20 into this PR to make tests work. |
The MockLDAP class's internal recording system of LDAP changes and the system for customizing Python LDAP API replies are implemented via storing and returning data into/from Python dictionary-like entities. For putting elements into these entities (namely, MockLDAP.directory, MockLDAP.calls and MockLDAP.return_value_map), all presented data elements (modlists, dn strings, Python LDAP data structures, etc.) need to be hashable. However, Python lists and dictionaries are not hashable. Thus, we now send every incoming data item into a generically designed _tupelize() function. By using the new _tupelize() function, we are now able to drop various individual tuple() calls on list objects inside the processed data elements (thus, we don't need to unwrap these data elements anymore). The _tupelize() function works for all basic Python variable types (booleans, strings, numbers, tuples, lists and dictionaries).
sunweaver
force-pushed
the
pr/fix-ldapmodify-operations
branch
from
12e6441 to
1e87c4e
Compare
sunweaver
changed the title
| if op is 0: | ||
| # FIXME: Can't handle multiple entries with the same name | ||
| # its broken right now | ||
| # do a MOD_ADD, assume it to be a list of values |
There was a problem hiding this comment.
nitpick - Can we leave the piece of the comment saying this is MOD_ADD?
Suggested change
| # do a MOD_ADD, assume it to be a list of values | |
| # do a MOD_ADD |
mateuszmandera
approved these changes
Jul 17, 2021
There was a problem hiding this comment.
These all look like valuable improvements and the implementation looks correct for fakeldap (we're not intending to be covering all the edge cases and accurately mimicking real LDAP behavior for them - and this expands the range of well-handled scenarios), so I think it should be good to merge @timabbott
|
On Sa 17 Jul 2021 16:36:03 CEST, Mateusz Mandera wrote:
@mateuszmandera approved this pull request.
These all look like valuable improvements and the implementation
looks correct for `fakeldap` (we're not intending to be covering all
the edge cases and accurately mimicking real LDAP behavior for them
- and this expands the range of well-handled scenarios), so I think
it should be good to merge @timabbott
I am ok with all suggested improvements. However, I can't do the PR
update, as I am going on holiday tomorrow early morning.
Please add the suggested comment changes as a commit on top of adjust
my PR branch (you should be able to force-push to it).
Thanks+Greets,
Mike
--
DAS-NETZWERKTEAM
c\o Technik- und Ökologiezentrum Eckernförde
Mike Gabriel, Marienthaler Str. 17, 24340 Eckernförde
mobile: +49 (1520) 1976 148
landline: +49 (4351) 850 8940
GnuPG Fingerprint: 9BFB AEE8 6C0A A5FF BF22 0782 9AF4 6B30 2577 1B31
mail: ***@***.***, http://das-netzwerkteam.de
|
|
@mateuszmandera feel free to make that tweak and merge (I just made sure you have access; should be easy with our |
|
Merged, thanks a lot @sunweaver ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.