ldap addressbook book results are not sorted when autocompleting is in action

RESOLVED FIXED in Thunderbird 12.0

Status

MailNews Core
LDAP Integration
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: Torsten Werner, Assigned: Tobias Koenig)

Tracking

unspecified
Thunderbird 12.0
x86
All

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.3) Gecko/20070310 Iceweasel/2.0.0.3 (Debian-2.0.0.3-1)
Build Identifier: 1.5.0.7

- TB is configured to use a LDAP address book
- the LDAP server itself does not sort the query result
- TB is not able to sort the results before displaying the to the user in the autocomplete widget

Reproducible: Always

Steps to Reproduce:
1. type a substring of an email address in the To: field
2. the displayed results are not sorted which is annoying when using large address books (> 10000 entries)



Both the address sidebar and the standalone address book window are able to sort the LDAP query results - only the autocomplete widget is broken.
(Assignee)

Comment 1

10 years ago
Created attachment 285302 [details] [diff] [review]
Patch which implements the sorting of the LDAP results

Hi,

the attached patch implements the sorting of the LDAP result.

I'm not sure whether my simple sorting algorithm is good enough for
larger result sets, at least it is the same which is used in the
addressbooksession class.

Ciao,
Tobias

Comment 2

10 years ago
review?

Comment 3

10 years ago
I'm no LDAP expert... can you request items in a particular sort order?
Do we send the results to autocomplete all at once or individually?
I don't think autocomplete allows subsequent reordering of returned items.

Updated

10 years ago
Assignee: mscott → nobody
Status: UNCONFIRMED → NEW
Component: Message Compose Window → MailNews: LDAP Integration
Ever confirmed: true
Product: Thunderbird → Core
QA Contact: message-compose → ldap-integration

Updated

10 years ago
Assignee: nobody → tokoe
(Assignee)

Comment 4

10 years ago
Some LDAP servers support server side sorting of the result entries, however
that functionality is just an extension, so it's not supported by all servers
(and it's on the TODO list of OpenLDAP for several years now :().

The patch attached in comment #1 solves that problem by client side filtering.
Whenever it receives new result chunks from the LDAP server, it sorts them into
the internal result list of Thunderbird.
We have tested it with the LDAP server of bund.de, a directory which contains
address data of nearly all employees of the german government, so it's really
huge. Sorting worked fine.
(Assignee)

Updated

10 years ago
Attachment #285302 - Flags: review?(richm)

Comment 5

10 years ago
Actually I was trying to work out whether it was safe to sort at this point.

Your algorithm is inefficient but I haven't seen the addrbooksession one.
(Assignee)

Comment 6

10 years ago
Hej Neil,

no idea whether it is worth to tweak the insert algorithm (shouldn't be difficult thought). At least with our tests here I couldn't realize any performance problems. The network was the limiting factor...

Why do you think it's not safe?

Comment 7

10 years ago
(In reply to comment #6)
>Why do you think it's not safe?
I don't any more ;-) but I had to study the code first to reassure myself.

Comment 8

10 years ago
I don't know the AB code, just the ldapsdk code - you probably want to get Mark B. or dmose to review.
Comment on attachment 285302 [details] [diff] [review]
Patch which implements the sorting of the LDAP results

I'll try and take a look at it next week.
Attachment #285302 - Flags: review?(richm) → review?(bugzilla)
Comment on attachment 285302 [details] [diff] [review]
Patch which implements the sorting of the LDAP results

The general idea looks good, I've not tried it out yet, however here are some initial comments:

+    rv = mResultsArray->Count(&nbrOfItems);

Please check the error value like we do elsewhere in this function.

+        nsCOMPtr<nsIAutoCompleteItem> currentItem;

This would be better outside the loop.

+        nsresult rv = mResultsArray->QueryElementAt(insertPosition, NS_GET_IID(nsIAutoCompleteItem),
+                                                    getter_AddRefs(currentItem));

There is no need to reclare rv here, please drop that.

Additionally, please replace this with:

currentItem = do_QueryElementAt(mResultsArray, insertPosition, &rv);

This is better than the QueryElementAt function.

+	  currentItem->GetValue(currentItemValue);

This line has a tab in it, which we don't want. Please indent new/changed code at 2-space indentation as that is now the accepted standard.

+          if (itemValue < currentItemValue) 
+	  {
+              break;
+	  }

misaligned if, additionally you don't need the braces here.

+    rv = mResultsArray->InsertElementAt(item, insertPosition);
     if (NS_FAILED(rv)) {
         NS_ERROR("nsLDAPAutoCompleteSession::OnLDAPSearchEntry(): "
                  "mItems->AppendElement() failed");

The error message needs updating to say InsertElementAt rather than AppendElement.
Comment on attachment 285302 [details] [diff] [review]
Patch which implements the sorting of the LDAP results

This seems to work as expected, but please fix my comments in comment 10 and re-submit to me for approval.
Attachment #285302 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 12

10 years ago
Hej Mark,

sorry for the late reply, I was sidetracked by another project.

Thanks for your review and comments, the attached patch should include all
these changes
(Assignee)

Comment 13

10 years ago
Created attachment 312260 [details] [diff] [review]
Updated patch for ldap-sorting integration

The updated patch, according to Mark's request
Attachment #312260 - Flags: superreview?
Attachment #312260 - Flags: review?

Comment 14

10 years ago
Comment on attachment 312260 [details] [diff] [review]
Updated patch for ldap-sorting integration

Suggesting some reviewers for this patch.
Attachment #312260 - Flags: superreview?(dmose)
Attachment #312260 - Flags: superreview?
Attachment #312260 - Flags: review?(bugzilla)
Attachment #312260 - Flags: review?
Comment on attachment 312260 [details] [diff] [review]
Updated patch for ldap-sorting integration

Sorry for the delay in reviewing this.

         NS_ERROR("nsLDAPAutoCompleteSession::OnLDAPSearchEntry(): "
-                 "mItems->AppendElement() failed");
+             "mResultsArray->InsertElementAt() failed");

The new line here should align at the same place the old line did.

r=me with that fixed.
Attachment #312260 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 16

9 years ago
Created attachment 322913 [details] [diff] [review]
Updated patch which fixes issue from review+
Attachment #322913 - Flags: superreview?
Attachment #312260 - Attachment is obsolete: true
Attachment #312260 - Flags: superreview?(dmose)
Attachment #285302 - Attachment is obsolete: true
Comment on attachment 322913 [details] [diff] [review]
Updated patch which fixes issue from review+

Tobias, you need to fill in an email address when requesting review (it also helps if you mark old patches obsolete).
Attachment #322913 - Flags: superreview? → superreview?(dmose)
(Assignee)

Comment 18

9 years ago
Hej,

any updates on the patch? Will it make it upstream?

Ciao,
Tobias
Product: Core → MailNews Core
(Assignee)

Comment 19

8 years ago
Anyone for superreview?
I'll bump this up my priority list; sorry for the delay.

Comment 21

8 years ago
any news on this?
Sorry, I've been busy with Thunderbird 3 blocking stuff, I'll try and clear out my review queue after we ship.  My main concern with this patch is what the performance is going to be like for the case where we have a large number of matches.  It may be fine, but I'd like to look at the algorithm in detail.
Argh, so I've just remembered the work on bug 452232 (which I'm hoping we can get landed after TB 3 would obsolete the patch, and possibly make it harder to implement as well :-(

Updated

6 years ago
Attachment #322913 - Flags: superreview?(dmose) → superreview?(mbanner)
(In reply to Mark Banner from comment #23)
> Argh, so I've just remembered the work on bug 452232 (which I'm hoping we
> can get landed after TB 3 would obsolete the patch, and possibly make it
> harder to implement as well :-(
Doesn't the patch in bug 452232 already sort the autocomplete results?
Created attachment 592116 [details] [diff] [review]
Fixes bitrot

Neil is right, I did put a sort in that patch. So we should just get this landed - I updated the patch to apply to current trunk.
Attachment #592116 - Flags: review+
Checked in: http://hg.mozilla.org/comm-central/rev/aaaa9a12e543
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 12.0
Attachment #322913 - Attachment is obsolete: true
Attachment #322913 - Flags: superreview?(mbanner)
Comment on attachment 592116 [details] [diff] [review]
Fixes bitrot

>+    currentItem = do_QueryElementAt(mResultsArray, insertPosition, &rv);
This would need an #include "nsMsgUtils.h" to work with the external API.
You need to log in before you can comment on or make changes to this bug.