Closed
Bug 124897
Opened 23 years ago
Closed 17 years ago
Implement editing of a directory in the LDAP XPCOM SDK
Categories
(Directory :: LDAP XPCOM SDK, defect, P4)
Directory
LDAP XPCOM SDK
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: peterv, Assigned: standard8)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 10 obsolete files)
42.29 KB,
patch
|
standard8
:
review+
mscott
:
superreview+
|
Details | Diff | Splinter Review |
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•23 years ago
|
||
Comment 2•23 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•23 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•22 years ago
|
||
Any chance of getting the code on a CVS branch soon? [I need LDAP modify functionality. :) ]
Reporter | ||
Comment 5•22 years ago
|
||
I'll make an updated patch.
Comment 6•22 years ago
|
||
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•22 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•22 years ago
|
||
Haven't tested this one extensively but it seems to work fine.
Attachment #92572 -
Attachment is obsolete: true
Comment 9•22 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•22 years ago
|
||
Yeah, I know. Could use more comments. Sigh.
Attachment #93698 -
Attachment is obsolete: true
Comment 11•21 years ago
|
||
hum, I'm new here and now is kind of blur....
Reporter | ||
Comment 12•21 years ago
|
||
Attachment #108613 -
Attachment is obsolete: true
Comment 13•20 years ago
|
||
Dan, any progress on this patch? The ability to modify ldap directories could be very useful :-)
Reporter | ||
Comment 14•20 years ago
|
||
Updated to trunk.
Assignee: peter.vanderbeken → peterv
Attachment #125079 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Reporter | ||
Updated•20 years ago
|
Attachment #148698 -
Flags: review?(dmose)
Comment 15•19 years ago
|
||
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•19 years ago
|
Component: Address Book → LDAP XPCOM SDK
Product: Thunderbird → Directory
Target Milestone: Thunderbird1.1 → mozilla1.8beta3
Version: Trunk → other
Updated•19 years ago
|
Priority: -- → P4
Updated•19 years ago
|
Target Milestone: mozilla1.8beta3 → ---
Comment 16•19 years ago
|
||
Silly question - why is it not possible to vote for this bug?
Assignee | ||
Comment 17•18 years ago
|
||
Updated patch to remove bitrots.
Attachment #148698 -
Attachment is obsolete: true
Assignee | ||
Comment 18•18 years ago
|
||
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•18 years ago
|
||
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•18 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•18 years ago
|
||
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•17 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•17 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•17 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•17 years ago
|
||
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•17 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•17 years ago
|
||
Patch checked in -> Fixed. Thanks to Peter for providing the main part of the patch for this.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 28•16 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•16 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.
Description
•