Implement editing of a directory in the LDAP XPCOM SDK

RESOLVED FIXED

Status

Directory
LDAP XPCOM SDK
P4
normal
RESOLVED FIXED
16 years ago
5 years ago

People

(Reporter: peterv, Assigned: standard8)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 10 obsolete attachments)

(Reporter)

Description

16 years ago
I have been working on this, I'll attach my patch. It's a first attempt at
implementing this, I introduced a new interface (nsILDAPModification) which
allows us to do several operations at once (much like in the LDAP C SDK). Dan,
your thoughts on this would be appreciated.

I'm currently using this from JS like this (for example replacing one of the
values of an attribute):

var mods = new Array();
var mod = Components.classes["@mozilla.org/network/ldap-modification;1"].
    createInstance(Components.interfaces.nsILDAPModification);
mod.operation = Components.interfaces.nsILDAPModification.MOD_REPLACE;
mod.type = attributeName;
var newValues;
var valuesCount = {};
try {
    newValues = message.getValues(type.Value, valuesCount);
    for (var i = 0; i < valuesCount.value; i++) {
        if (newValues[i] == oldValue) {
            newValues[i] = newValue;
        }
    }
}
catch (e) {
}
mod.setValues(newValues.length, newValues);
mods.push(mod);

... add more operations ...

... set up connection etc. ...

var op = Components.classes["@mozilla.org/network/ldap-operation;1"].
    createInstance(Components.interfaces.nsILDAPOperation);

... set up listener ...

op.modifyExt(aDN, mMods.length, mMods);

Comment 1

16 years ago
Created attachment 68909 [details] [diff] [review]
First stab

Comment 2

16 years ago
I don't think we can use an XPCOM array of |string| to represent a BER value,
since BER values aren't necessarily null terminated (eg jpegPhoto).  What do you
think about switching this to use an nsISupportsArray (since this is refcounted,
it avoids unnecessary copies) of nsILDAPBERValue (as proposed in bug 119380)?

Comment 3

16 years ago
nsISupportsArray is not so nice for JS consumers. It doesn't matter when
changing values, since then you can get the nsISupportsArray by calling
nsILDAPMessage::GetBinaryValues and then changing/deleting. However, it would be
nice if one could just create an array in JS with simple strings and use that.
It seems like nsIVariant would allow us to pass in simple arrays of strings,
simple arrays of nsILDAPBERValue's or nsISupportsArray's of nsILDAPBERValue's.
I'm not sure but it does seem to make it a bit more complicated to use for C++
consumers (have to create an nsIVariant and stuff the nsISupportsArray in
there). Thoughts?

Comment 4

15 years ago
Any chance of getting the code on a CVS branch soon?
[I need LDAP modify functionality.  :)  ]
(Reporter)

Comment 5

15 years ago
I'll make an updated patch.

Comment 6

15 years ago
Created attachment 92572 [details] [diff] [review]
First stab

Didn't change a lot, but should build on Mac too. Dmose, I keep forgetting what
we decided to do, switch to nsISupportsArray?
Attachment #68909 - Attachment is obsolete: true

Comment 7

15 years ago
No, we decided to bail on nsISupportsArray and stick with XPCOM arrays. 
However, I think we should still switch from arrays of |string| to arrays of
|nsILDAPBERValue|.

Comment 8

15 years ago
Created attachment 93698 [details] [diff] [review]
Use nsILDAPBERValue

Haven't tested this one extensively but it seems to work fine.
Attachment #92572 - Attachment is obsolete: true

Comment 9

15 years ago
Comment on attachment 93698 [details] [diff] [review]
Use nsILDAPBERValue

General
-------
a) Most of the LDAP XPCOM SDK checks for NULL pointers and returns the
appropriate value explicitly rather than using NS_ENSURE_*.  Can you
do this also?  NS_ENSURE_* is hard on readers new to the code base
because it hides a return.

b) the uses of @exception don't document which errors might occur and
what they mean.


nsILDAPModification.idl
-----------------------
c) I don't think 78848 affects us anymore, because the string types
are now forward declared somewhere in xpcom.  So that hack can
probably be removed.

d) "type" should be an |nsACString| instead of a |string|


nsILDAPOperation.idl
--------------------
e) addExt, deleteExt, modifyExt, and rename should use
|nsAUTF8String| instead of |wstring|


nsLDAPModification.cpp
----------------------

f) I believe that the getters and setters need to do locking here, for
true threadsafety.  Can you either add this locking, or else add
comments both in the IDL and C++ explaining that until this gets
fixed, callers are on their own for locking.  Hmm, as I write this, I
realize that I need to do the same thing to ns(I)LDAPBERValue; doh!

g) Can you make sure all lines wrap before column 80?

::SetValues
-----------
h) str doesn't appear to be used at all

nsLDAPOperation.cpp
-------------------
::AddExt
--------
i) Does LDAPMod **attrs doesn't need to be initialized to 0?

j) It appears that if modcount is 0 or the array is 0, ldap_add_ext()
gets called anyway.  Is this the intent?

k) A number of the new functions, especially the non-yet-IDL versions of
the operations, contain lots of code with almost no spacing and no
commentary.  It would be much easier to read and review if you could
add spacing and commentary similar to that in the rest of the XPCOM
SDK.
Attachment #93698 - Flags: needs-work+

Comment 10

15 years ago
Created attachment 108613 [details] [diff] [review]
v3

Yeah, I know. Could use more comments. Sigh.
Attachment #93698 - Attachment is obsolete: true

Comment 11

14 years ago
hum, I'm new here and now is kind of blur....
(Reporter)

Comment 12

14 years ago
Created attachment 125079 [details] [diff] [review]
v3.1
Attachment #108613 - Attachment is obsolete: true

Updated

13 years ago
Blocks: 86405

Comment 13

13 years ago
Dan, any progress on this patch? The ability to modify ldap directories could be
very useful :-)
(Reporter)

Comment 14

13 years ago
Created attachment 148698 [details] [diff] [review]
v3.1

Updated to trunk.
Assignee: peter.vanderbeken → peterv
Attachment #125079 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Reporter)

Updated

13 years ago
Attachment #148698 - Flags: review?(dmose)

Updated

13 years ago
Blocks: 210870

Updated

13 years ago
No longer blocks: 210870

Updated

13 years ago
Blocks: 210870
Taking, as I hope to finally get this reviewed and driven into the tree for
Thunderbird 1.1.
Assignee: peterv → dmose
Status: ASSIGNED → NEW
Component: LDAP XPCOM SDK → Address Book
Flags: review?(dmose)
Flags: needs-work+
Product: Directory → Thunderbird
Target Milestone: --- → Thunderbird1.1
Version: other → Trunk

Updated

12 years ago
Component: Address Book → LDAP XPCOM SDK
Product: Thunderbird → Directory
Target Milestone: Thunderbird1.1 → mozilla1.8beta3
Version: Trunk → other

Updated

12 years ago
Priority: -- → P4

Updated

12 years ago
Target Milestone: mozilla1.8beta3 → ---

Comment 16

12 years ago
Silly question - why is it not possible to vote for this bug?
(Assignee)

Comment 17

11 years ago
Created attachment 212495 [details] [diff] [review]
v3.1a

Updated patch to remove bitrots.
Attachment #148698 - Attachment is obsolete: true
(Assignee)

Comment 18

11 years ago
Created attachment 246398 [details] [diff] [review]
v4

I've taken the previous version of this patch and changed the array interfaces to nsIArray. This makes it much easier to pass arrays around the system in a reasonable manner and saves extra work.

I've also added a couple of helper functions which allow us to only call one function to set the object values and not three.

I've been able to test the add/modify/delete functions and they seem ok. I haven't got round to testing the rename one yet.
Attachment #212495 - Attachment is obsolete: true
(Assignee)

Comment 19

11 years ago
Created attachment 246706 [details] [diff] [review]
v4.1

Revised patch. Resolves some deadlock and null pointer issues in nsLDAPModification.

Requesting first review. I've been testing add/modify/delete with some code I've written for the address book, with no real problems so far.

Dan: Some of the code in the cpp implementation may not be quite as good as it can be yet - so I suggest we make sure we're happy with the interfaces first.
Assignee: dmose → bugzilla
Attachment #246398 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #246706 - Flags: review?(dmose)
(Assignee)

Comment 20

11 years ago
Comment on attachment 246706 [details] [diff] [review]
v4.1

Cancelling review request. I'm working on tidying up some of the c++ code as I didn't before and there's improvements to be made.

I'd still like any comments on the interfaces though.
Attachment #246706 - Flags: review?(dmose)
(Assignee)

Comment 21

11 years ago
Created attachment 249794 [details] [diff] [review]
v4.2

This patch tidies up (whitespace etc) and optimises the previous version, should be good for review now.
Attachment #246706 - Attachment is obsolete: true
Attachment #249794 - Flags: review?(dmose)
(Assignee)

Comment 22

10 years ago
Comment on attachment 249794 [details] [diff] [review]
v4.2

Dan hasn't got time to work on this, David would you be able to review it?
Attachment #249794 - Flags: review?(dmose) → review?(bienvenu)

Comment 23

10 years ago
Comment on attachment 249794 [details] [diff] [review]
v4.2

this is very exciting, Mark! Thx for doing this.

+  PRUint32 counter;
+  for (counter = 0; counter < modCount; ++counter) {
+    delete attrs[counter];
+  }


you can declare counter inside the for loop and lose the braces, if you want (really a style question - up to you - I guess the prevailing style is to always use braces :-()


+      PRUint32 counter;
+      for (counter = 0; counter < valuesCount; ++counter) {
+        if (counter < valueIndex)
+          delete (*aBValues)[valueIndex];
+      }

this is a little odd - why not just change the for loop condition to counter < valueIndex?
Attachment #249794 - Flags: review?(bienvenu) → review+
(Reporter)

Comment 24

10 years ago
Yeah, thanks for taking this on. Could you please change my email address in the license headers to peterv@propagandism.org? And there's some QueryElementAt calls that really should be do_QueryElementAt.
(Assignee)

Comment 25

10 years ago
Created attachment 257024 [details] [diff] [review]
Patch v5

I've addressed David's and Peter's comments with this version. Requesting sr.
Attachment #249794 - Attachment is obsolete: true
Attachment #257024 - Flags: superreview?(mscott)
Attachment #257024 - Flags: review+

Comment 26

10 years ago
Comment on attachment 257024 [details] [diff] [review]
Patch v5

I saw several braces around single line statements like:

+      if (!attrs[index]) {
+        return NS_ERROR_OUT_OF_MEMORY;
+      }

Not sure if you want to remove those.

Looks like you got good reviews from Peter and David, that's good enough for me.

Thanks Mark!
Attachment #257024 - Flags: superreview?(mscott) → superreview+
(Assignee)

Comment 27

10 years ago
Patch checked in -> Fixed. Thanks to Peter for providing the main part of the patch for this.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 28

9 years ago
I am new to this. How does one get the patch? Do I just checkout the latest tree? I would not mind being one of the testers.

Also when will it show up in a released binary?
(Assignee)

Comment 29

9 years ago
(In reply to comment #28)
> I am new to this. How does one get the patch? Do I just checkout the latest
> tree? I would not mind being one of the testers.

Note, this bug is referencing the LDAP XPCOM SDK. If you're looking for writeable LDAP functionality in SeaMonkey/Thunderbird then you want bug 86405.

Details about the LDAP XPCOM SDK can be found here: https://wiki.mozilla.org/LDAP_C_SDK (for the c-sdk at least), including how to get the source.

> Also when will it show up in a released binary?

If you're talking about LDAP XPCOM SDK, there are no released binaries.

If you're talking about writable LDAP functionality in SeaMonkey/Thunderbird then that is undertermined, comments 66 and 70 on bug 86405 give the current status and it has not progressed further than that.
You need to log in before you can comment on or make changes to this bug.