633150 - Change nsILDAPURL::setAttributes to take a string rather than an array

Status: RESOLVED FIXED

Description

[Mark Banner (:standard8) (afk 9 - 18 Aug)]

Attachment: The fix

In bug 452232, I currently have one nsISupports parameter in which to pass the required attributes.

Currently, we pass an array into nsILDAPURL::setAttributes. If we were to change this to a comma-separated string, it would make the whole process of getting attributes from/to places a lot easier.

Although this means we will need a bit more processing to join and to separate out the attributes in various places, I think the easier to use interface makes it worthwhile.

Comment 1

[Mark Banner (:standard8) (afk 9 - 18 Aug)]

Attachment: The fix v2

Minor change, fixes a couple of unused variables that I'd left hanging around.

Comment 2

[[e-mail]]

Comment on attachment 511356 [details] [diff] [review]
The fix v2

>@@ -100,7 +100,7 @@ nsLDAPURL::GetPathInternal(nsCString &aP
>   if (!mDN.IsEmpty())
>     aPath.Append(mDN);
> 
>-  PRUint32 count = mAttributes.Count();
>+  PRUint32 count = mAttributes.Length();
>   if (count)
>   {
>     aPath.Append('?');
>@@ -108,7 +108,7 @@ nsLDAPURL::GetPathInternal(nsCString &aP
Not the ideal number of lines of context ;-)

>+    rv = SetAttributeArray(const_cast<const char **>(desc->lud_attrs));
Since this is an internal method, you can drop the const.

> NS_IMETHODIMP nsLDAPURL::GetAttributes(PRUint32 *aCount, char ***_retval)
So, the getter and setter are now out of sync? Annoying. Unless there's a possiblity of changing to attribute ACString attributes;

>+    cArray = static_cast<char **>(nsMemory::Alloc(length * sizeof(char *)));
Nit: could switch to NS_Alloc

>+  const char** attributes = aAttributes;
Don't need to copy it, just use aAttributes.

>+  if (!ParseString(aAttributes, ',', mAttributes))
>+    return NS_ERROR_FAILURE;
Not NS_ERROR_OUT_OF_MEMORY?

>+  getAllCardAttributes: function getAllCardAttributes() {
>+    var attrs = "";
>     for each (var attrArray in this.mPropertyMap) {
>+      if (attrArray.length) {
>+        if (attrs)
>+          attrs = [attrs, attrArray].join(",");
>+        else
>+          attrs = attrArray.join(",");
>+      }
>     }
Not sure what this is trying to do. Probably easist would be to join the array once at the end, instead of trying to do it piecemeal.

Comment 3

[Mark Banner (:standard8) (afk 9 - 18 Aug)]

(In reply to comment #2)
> > NS_IMETHODIMP nsLDAPURL::GetAttributes(PRUint32 *aCount, char ***_retval)
> So, the getter and setter are now out of sync? Annoying. Unless there's a
> possiblity of changing to attribute ACString attributes;

The c-sdk requires the array format, so we need to translate it somewhere. We could probably have a GetAttributesArray function, as well as a GetAttributes that isn't used.

Comment 4

[[e-mail]]

(In reply to comment #3)
> (In reply to comment #2)
> > > NS_IMETHODIMP nsLDAPURL::GetAttributes(PRUint32 *aCount, char ***_retval)
> > So, the getter and setter are now out of sync? Annoying. Unless there's a
> > possibility of changing to attribute ACString attributes;
> 
> The c-sdk requires the array format, so we need to translate it somewhere. We
> could probably have a GetAttributesArray function, as well as a GetAttributes
> that isn't used.

I would be fine with this, particularly if the array had a null terminator, although there's also the possibility of getting nsLDAPOperation::SearchExt to create the array format.

Oh, and maybe GetPathInternal could share code with GetAttributes?
void
nsLDAPURL::AppendAttributesTo(nsACString& _retval)
{
  for (PRUint32 index = 0; index < mAttributes.Length(); index++)
  {
    if (index)
      _retval.Append(',');
    _retval.Append(mAttributes[index]);
  }
}
/* ... */
NS_IMETHODIMP
nsLDAPURL::GetAttributes(nsACString& _retval)
{
  _retval.Truncate();
  AppendAttributesTo(_retval);
  return NS_OK;
}

Comment 5

[David :Bienvenu]

Comment on attachment 511356 [details] [diff] [review]
The fix v2

clearing review request since I think an other patch is coming.

Comment 6

[Mark Banner (:standard8) (afk 9 - 18 Aug)]

Attachment: The fix v3

Ok, this was a bit more wide-ranging than I would have liked, but I think this is probably much better now.

I've changed nsLDAPURL to store the attributes as a comma separated string and not provide an array alternative. This had a few knock-on effects of changing storage formats elsewhere (or not actually storing at all where not necessary).

This will change the LDAP sync query stuff to not return all elements when particular elements are not present in the card, but I think that is probably fine.

Comment 7

[[e-mail]]

Comment on attachment 512258 [details] [diff] [review]
The fix v3

>+nsresult nsLDAPURL::SetAttributeArray(char** aAttributes)
>+{
>+  mAttributes.Truncate();
>+  PRBool addedItem = PR_FALSE;
>+
>+  while (aAttributes && *aAttributes)
>   {
>+    if (addedItem)
Nit: can use !mAttributes.IsEmpty() instead

>+    mAttributes.Append(nsDependentCString(*aAttributes));
Nit: can probably Append(*aAttributes) directly

>+    // Check to see if the attribute is already stored. If it is, then also
>+    // check to see if it is the last attribute in the string, or if the next
>+    // character is a comma, this means we won't match substrings.
I think you also need to check whether the previous character is a comma.
I couldn't find many callers of this method, so the simplest way of doing this is probably to prefix and suffix a comma to mAttributes and aAttribute and do the find on those. (I have a vague memory that we did or do do that somewhere else but I can't seem to find where.)
[In theory you could keep the string internally with the commas prefixed and suffixed which would make removing an attribute cleaner since you no longer have any edge cases but since nobody actually removes an attribute it probably makes no sense to do that.]

>+    PRInt32 pos = mAttributes.Find(nsCString(aAttribute),
>+                                   CaseInsensitiveCompare);
Nit: use PromiseFlatCString instead of nsCString

>+  PRInt32 pos = mAttributes.Find(nsCString(aAttribute));
Was this supposed to be case insensitive?

Comment 8

[Mark Banner (:standard8) (afk 9 - 18 Aug)]

Attachment: The fix v4

Updated addressing Neil's comments and fixing the removeAttribute and providing a couple more tests for it.

Comment 9

[[e-mail]]

Comment on attachment 513084 [details] [diff] [review]
The fix v4

>+    if (pos >= 0 && mAttributes.Length() != pos + aAttribute.Length() &&
>+        mAttributes[pos + aAttribute.Length()] != ',')
Oops, I hadn't noticed that this test is wrong. If your attributes are "CommonName,SecondaryEmail" then AddAttribute("SecondaryEmail") adds it again, while AddAttribute("Common") does nothing. Note that you need to fix the HasAttribute test before you copy it, see below, since the test is so broken that AddAttribute("mail") actually works!

>+  else if (mAttributes[pos + aAttribute.Length()] == ',' &&
>+           (pos == 0 || mAttributes[pos - 1] == ','))
RemoveAttribute("Common"), RemoveAttribute("mail") all do nothing as expected.

>+  *_retval = pos >= 0 && (mAttributes.Length() == pos + aAttribute.Length() ||
>+                          mAttributes[pos + aAttribute.Length()] == ',');
But this is missing the (pos == 0 || mAttributes[pos - 1] == ',') clause, which means that HasAttribute("Name"), HasAttribute("mail") return true.

Apparently line 293 has trailing whitespace.

Comment 10

[Mark Banner (:standard8) (afk 9 - 18 Aug)]

Attachment: The fix v5

Ok, a few more tests, plus this time, I've changed the internal storage to have a comma on start and end. That saves the complex if statements.

Comment 11

[[e-mail]]

Comment on attachment 513430 [details] [diff] [review]
The fix v5

>+  NS_ASSERTION(mAttributes[0] == ',' &&
>+               mAttributes[mAttributes.Length() - 1] == ',',
Nit: .First() and .Last() [this happens in a few places]

>+               "ERROR: mAttributes does not being and end with a comma");
Typo: begin
Nit: Probably don't need to include ERROR: given that the message already includes ###!!! ASSERTION in it.

>+  while (aAttributes && *aAttributes)
>+  {
>+    mAttributes.Append(',');
>+    mAttributes.Append(*aAttributes);
>+    ++aAttributes;
>+  }
>+
>+  if (!mAttributes.IsEmpty())
>+    mAttributes.Append(',');
This is correct, but it took me a few seconds to realise that!

>+    mAttributes = ',';
>+    mAttributes.Append(aAttribute);
>+    mAttributes.Append(',');
I like this way of wrapping aAttribute in commas, as opposed to using Insert.

>+    PRInt32 pos = mAttributes.Find(PromiseFlatCString(findAttribute),
Nit: findAttribute is now a flat string, so there's nothing to promise.

>+    mAttributes.Append(Substring(findAttribute, 1, findAttribute.Length() - 1));
Nit: Substring(findAttribute, 1) defaults to the rest of the string.

>+  if (pos == 0)
>+    mAttributes = StringTail(mAttributes,
>+                             mAttributes.Length() - findAttribute.Length() + 1);
Nit: This ends up doing the same as the Cut below.
>+  else
>+    mAttributes.Cut(pos, findAttribute.Length() - 1);

I think RemoveAttribute needs to clear mAttributes when you remove the last one. Maybe if you write
if (mAttributes.Equals(findAttribute, nsCaseInsensitiveCStringComparator()))
  mAttributes.Truncate();

>+  if (mAttributes.IsEmpty())
>+    return NS_OK;
You forgot to write your outparam. (Or you could just skip this check.)

Comment 12

[Mark Banner (:standard8) (afk 9 - 18 Aug)]

(In reply to comment #11)
> Comment on attachment 513430 [details] [diff] [review]
> The fix v5
> 
> >+  NS_ASSERTION(mAttributes[0] == ',' &&
> >+               mAttributes[mAttributes.Length() - 1] == ',',
> Nit: .First() and .Last() [this happens in a few places]

The external API version (nsStringAPI.h) doesn't have any sign of a Last() function (although it does have First!).

Comment 13

[Mark Banner (:standard8) (afk 9 - 18 Aug)]

Attachment: The fix v6

Updated for latest comments.

Comment 14

[[e-mail]]

(In reply to comment #12)
>(In reply to comment #11)
>>(From update of attachment 513430 [details] [diff] [review])
>>>+  NS_ASSERTION(mAttributes[0] == ',' &&
>>>+               mAttributes[mAttributes.Length() - 1] == ',',
>>Nit: .First() and .Last() [this happens in a few places]
>The external API version (nsStringAPI.h) doesn't have any sign of a Last()
>function (although it does have First!).
Ah yes, we added Last() to nsMsgUtils.h but I guess we can't use that here.

Comment 15

[[e-mail]]

Comment on attachment 513782 [details] [diff] [review]
The fix v6

>+        
>+    NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(valueCount, vals);
...
>+  
>+  // If mAttributes isn't empty, cut of the internally stored commas at start
>+  // and end, and append to the path.
Nit: whitespace on the blank lines

As I typed the above nit, I just noticed that the comment I quoted should say "cut off" instead of "cut of".

Comment 16

[Mark Banner (:standard8) (afk 9 - 18 Aug)]

Attachment: The fix v7

Updated for Neil's nits.

Comment 17

[David :Bienvenu]

Comment on attachment 513879 [details] [diff] [review]
The fix v7

sr=me, with some nits:

tab?

-        attrs[aAttrCount] = 0;
-    }
+            PromiseFlatCString(aAttributes).get(), aSizeLimit));

don't need space between ** and attrs:

+    char ** attrs = nsnull;

don't need ns_error when out of memory:

+        NS_ERROR("nsLDAPOperation::SearchExt: out of memory");

wow, this file is all over the place with its spaces :-)
+  char** attributes;
+  nsresult rv = aMessage->GetAttributes(&attrCount, &attributes);

rv is an nsresult here, I think:

 nsLDAPURL::SetPathInternal(const nsCString &aPath)
 {
-  PRUint32 rv, count;
+  PRUint32 rv;

this comment needs to move down a bit to be closer to the code it refers to:

+  // We store the string internally with comma before and after, so strip
+  // them off here.
+  if (mAttributes.IsEmpty())

I think the same comment about starting the string off with a ',' would be appropriate here too, since otherwise the code just looks wrong:-) :

+  if (aAttributes.IsEmpty())
+    mAttributes.Truncate();
+  else
+  {
+    if (aAttributes[0] != ',')
+      mAttributes = ',';

lose the commented out line:
-  nsCStringArray mAttributes;  // List of attributes
+  //  nsTArray<nsCString> mAttributes; // List of attributes
+  nsCString mAttributes;


a comma-separated...:
+   * @return                      an comma-separated list of attribute names

Comment 18

[Mark Banner (:standard8) (afk 9 - 18 Aug)]

(In reply to comment #17)
> Comment on attachment 513879 [details] [diff] [review]
> The fix v7
> 
> sr=me, with some nits:
> 
> tab?
> 
> -        attrs[aAttrCount] = 0;
> -    }
> +            PromiseFlatCString(aAttributes).get(), aSizeLimit));

Nope, just a complicated diff.


> wow, this file is all over the place with its spaces :-)

Well, the function will be consistent with itself.

> +  char** attributes;
> +  nsresult rv = aMessage->GetAttributes(&attrCount, &attributes);
> 
> rv is an nsresult here, I think:
> 
>  nsLDAPURL::SetPathInternal(const nsCString &aPath)
>  {
> -  PRUint32 rv, count;
> +  PRUint32 rv;

ldap_url_parse returns an int, and we translate the error code further down in the function to a proper nsresult.

Comment 19

[Mark Banner (:standard8) (afk 9 - 18 Aug)]

Checked in: http://hg.mozilla.org/comm-central/rev/e9817d0c0d02

Comment 20

[David :Bienvenu]

(In reply to comment #18)
> > rv is an nsresult here, I think:
> > 
> >  nsLDAPURL::SetPathInternal(const nsCString &aPath)
> >  {
> > -  PRUint32 rv, count;
> > +  PRUint32 rv;
> 
> ldap_url_parse returns an int, and we translate the error code further down in
> the function to a proper nsresult.

Ah, I see, sorry - I got confused because we assign the nsresult return of nsLDAPURL::SetAttributeArray to rv, and rv is an unsigned int...