124897 - Implement editing of a directory in the LDAP XPCOM SDK

Status: RESOLVED FIXED

(Directory :: LDAP XPCOM SDK, defect, P4)

Description

[Peter Van der Beken [:peterv]]

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

[Peter Van der Beken]

Attachment: First stab

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

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

[Peter Van der Beken]

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

[Robert John Churchill]

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

Comment 5

[Peter Van der Beken [:peterv]]

I'll make an updated patch.

Comment 6

[Peter Van der Beken]

Attachment: 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?

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

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

[Peter Van der Beken]

Attachment: Use nsILDAPBERValue

Haven't tested this one extensively but it seems to work fine.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 10

[Peter Van der Beken]

Attachment: v3

Yeah, I know. Could use more comments. Sigh.

Comment 11

[Chan Min Wai]

hum, I'm new here and now is kind of blur....

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: v3.1

Comment 13

[David :Bienvenu]

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

Comment 14

[Peter Van der Beken [:peterv]]

Attachment: v3.1

Updated to trunk.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Taking, as I hope to finally get this reviewed and driven into the tree for
Thunderbird 1.1.

Comment 16

[David Hagood]

Silly question - why is it not possible to vote for this bug?

Comment 17

[Mark Banner (:standard8)]

Attachment: v3.1a

Updated patch to remove bitrots.

Comment 18

[Mark Banner (:standard8)]

Attachment: 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.

Comment 19

[Mark Banner (:standard8)]

Attachment: 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.

Comment 20

[Mark Banner (:standard8)]

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.

Comment 21

[Mark Banner (:standard8)]

Attachment: v4.2

This patch tidies up (whitespace etc) and optimises the previous version, should be good for review now.

Comment 22

[Mark Banner (:standard8)]

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?

Comment 23

[David :Bienvenu]

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?

Comment 24

[Peter Van der Beken [:peterv]]

Yeah, thanks for taking this on. Could you please change my email address in the license headers to [e-mail]? And there's some QueryElementAt calls that really should be do_QueryElementAt.

Comment 25

[Mark Banner (:standard8)]

Attachment: Patch v5

I've addressed David's and Peter's comments with this version. Requesting sr.

Comment 26

[Scott MacGregor]

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!

Comment 27

[Mark Banner (:standard8)]

Patch checked in -> Fixed. Thanks to Peter for providing the main part of the patch for this.

Comment 28

[Mahesh Jethanandani]

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?

Comment 29

[Mark Banner (:standard8)]

(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.