Department number search fails for LDAP Directory ?

NEW
Unassigned

Status

MailNews Core
LDAP Integration
16 years ago
6 years ago

People

(Reporter: yulian chang, Unassigned)

Tracking

(Blocks: 1 bug, {helpwanted})

Trunk
helpwanted

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patchlove][has draft patch][gs], URL)

Attachments

(7 obsolete attachments)

(Reporter)

Description

16 years ago
2002022003 Wins builds

Searching with department number in personal address book succeeds but fails in
LDAP directory.

1. search in AOLTW
   Hostname: nsdirectory.netscape.com
   Base DN: dc=com
2. Match the following
   Department contains 601
Actual result: 0 matches
expected result:x matches found

3. Match the following
   Name contains yu
4. drag&drop one entry containing department# 601 from above search results to
Personal address book
5. in advanced AB search window, Search in Personal Address Book
6. Match the following
   Department contains 601
7. the entry containing department# 601 is found
(Reporter)

Comment 1

16 years ago
Step 3 from above should be corrected to the following:

3. go back to Address Book window, select AOLTW directory and do quick search
with "yu"
(Reporter)

Updated

16 years ago
Whiteboard: nab-search,nab-ldap

Comment 2

16 years ago
This looks like a duplicate of bug #119143. There can exist more than one
Mozilla LDAP attribute mapped to a Mozilla Address Book attribute. This mapping
is determined in:

http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsAbLDAPProperties.cpp

Thus the Mozilla Address Book attribute "Department" is mapped to the following
LDAP attributes:

ou,
orgunit,
department
departmentnumber

This mapping exists in a hashtable and thus when executing the advanced Search,
we have to translate the query into its corresponding LDAP query string. In the
above case, regardless of what LDAP attribute is on the server, we will always
have a query of (ou=*601*) i.e. we map against the first LDAP mapping in the
hashtable.

To fix this, we need to do either:

1. Change the mapping
2. Generate all corresponding LDAP attributes using the OR operator i.e.
(|(ou=*601*)(orgunit=*601*)(department=*601*)(departmentnumber=*601*))
3.Allow the users to choose their own mapping


Comment 3

15 years ago
Created attachment 72980 [details] [diff] [review]
Patch to generate OR list of all matched LDAP attributes for Search

This patch provides a solution for (2) above. This is probably the safest route
at this point in time. The patch itself may raise issues on whether:

a. The extra member of the table could not be dynamically generated. 
b. it will generate a different query than the current quick search but
obviously the same result set.
c. What effect on the performance if any due to generating a possible longer
query string and
d. We need to be aware of this issue when discussing bug #118454.

But overall the patch is a valid attempt to fix this issue.

Comment 4

15 years ago
Created attachment 72982 [details] [diff] [review]
Remove stray fprintf() from previous patch.
Attachment #72980 - Attachment is obsolete: true

Comment 5

15 years ago
Hello John,

there is a little mistypo in your patch = attachment (id=72980)

WebPage1 should be workurl and
WebPage2 should be homeurl.

Nothing critical, but if your CPU will be busy by
compiling something usefull, you can use the free time
to fix it ;-)

Thanks Petr


Original patch looks:----
// ?
-    {MozillaProperty_String, "WebPage1",        "homeurl"},
+    {MozillaProperty_String, "WebPage1",        "homeurl", 1},
     // ?
-    {MozillaProperty_String, "WebPage2",        "workurl"},
+    {MozillaProperty_String, "WebPage2",        "workurl", 1},

Comment 6

15 years ago
Created attachment 75377 [details] [diff] [review]
Keep patch up-to-date

Thanks Petr,

following the landing of fix for bug #128035, this patch was out-of-date.

Have you tested this patch?
Attachment #72982 - Attachment is obsolete: true

Comment 7

15 years ago
Created attachment 77634 [details] [diff] [review]
Keep patch up-to-date
Attachment #75377 - Attachment is obsolete: true

Comment 8

15 years ago
*** Bug 119143 has been marked as a duplicate of this bug. ***

Comment 9

15 years ago
Created attachment 99356 [details] [diff] [review]
proof of concept

We have investigated this bug recently and improved the former patches.
The former patches translate the "NOT" operation on multiple attrbutes using
"OR" operation rather than "AND", that is not right. We fixed it
in this patch.
Besides, by applying the new patch, we find an performance loss on the query
related with "displayname". IOW, queries on displayName will cost much more
time.
e.g. The query (displayname=*Marm*)) always costs more than 4 seconds
while queries (cn=*Marm*) and (commonname=*Marm*) often cost less than 1
seconds. 
To simply remove 'displayname' attribute is obviously not a good way. What's
more, we just test against Sun LDAP Corporate servers and find such performance
loss. We are not sure if this problem is related with the specific type of
servers or a gerneral problem. We should be very appreciated if some guys can
help to do more tests.

Comment 10

15 years ago
mcs@netscape.com responded to this on the directory list with the following:

"The most likely reason for slow performance is that one or more of the
attributes does not have a substring index in your directory server. In general,
end users may not have enough influence to convince the people who run a
corporate LDAP server to add new indexes... it seems like the end user needs
some way to control what kind od search filter is used." 

That makes sense. Also, it now transpires that ("sn|surname"),
("cn|commonname"), ("l|locality"), ("fax|facsimiletelephonenumber") are one
instance of aliases pointing to the same OID in the ldap schema and thus
generating (|(cn=*Marm*)(commonname=*Marm*)) is the same as generating
(|(cn=*Marm*)(cn=*Marm*)). Further as cn is a required field and as it appears
first in the mapping, then selecting(cn|displayname) will always return the
value of cn. So I suggest that we do the following:

1. Wait until there is agreement for bug 116692 and add this bug to its
dependency list.
2. We agree that we have an issue with some ldap attributes in the existing
mapping. We have already identified Department. WorkAddress looks like another. 
3. This bug is solely concerned with generating the correct filter. Support for
multiple value attributes is bug 119199.
4. While we agree that a UI to allow users to select their own mapping is the
best solution, an interim solution would be to employ a solution based in the
latest path to apply to the fields that we have identified in (2) above with the
following amendment. We do not simply use the following e.g. 
if "Department" maps to both "ou" and "departmentNumber" then the solution to
the following query: (Department,contains,"123") is not
(|(ou="*123*")(departmentNumber=*123*)) but rather we want something like this
if ou exists(has a value) then filter on this else
if departmentNumber exists, filter on that.
This would amount to a filter like 

(|(&(ou=*)(ou=*123*))(&(!(ou=*))(departmentNumber=*)(departmentNumber=*123*)))

which can be abbreviated to

(|(ou=*123*)(&(!ou=*))(departmentNumber=*123*)))

Comment 11

15 years ago
Sorry that last commment  should read:

(|(ou=*123*)(&(!(ou=*))(departmentNumber=*123*)))

Comment 12

15 years ago
I want to clarify comment #10. I suggest that we go ahead with providing an
interim solution along the lines that I have outlined. This can be revised when
the solution to bug #116692 is resolved and allow us to fix this until a proper
UI is implemented. 

The latest patch still provides a framework to implement this. There are two
things we need to change:
a. remove the existing identical OIDs from our filter
b. implement the exists then query etc..

Comment 13

15 years ago
Created attachment 103586 [details] [diff] [review]
New proof of concept patch incorporating testing for exist

This patch is a proof of concept only. There is case for getting rid of some of
the duplicate code used by GenerateOperatorFilter() functions in this patch by
using a generic GenerateFilter() in the nsAbBoolExprToLDAPFilter.cpp source.
But first I need to find out if there is any consensus for including such a
change.
Attachment #77634 - Attachment is obsolete: true
Attachment #99356 - Attachment is obsolete: true
This seems like a reasonable approach to me.  However, in your patch, can't all
those Generate*Filter functions be collapsed into a single function with an
operator argument?  Similarly, can't the unrolled 1..4 cases be collapsed into a
for loop?

Comment 15

15 years ago
Created attachment 104635 [details] [diff] [review]
New patch incorporating Dan's suggestions

Thanks for the feedback Dan. Simplifying the patch as suggested. Though looking
at it, I am sure this is a candidate for recursion. Nevertheless, the changes
are now minimized and I need to do some serious performance testing to ensure
that there is no change before progressing this further.
Attachment #103586 - Attachment is obsolete: true

Comment 16

15 years ago
I have tested the latest patch. It does help to get complete result now, but
unfortunenately the performance is still so bad. Compared with a non-patched
version, the patched version will cust much more time. Following is some of my
test results.
1. Search "pager=*1234" (means that pager contains "1234")
   non-patched version costs about 1 seconds;
   patched version will cost about 4 seconds;
2. Search "mobile=*1234*" (means that mobile contains "1234")
   non-patched version costs about 1.2 seconds
   patched version costs near 10 seconds
3. Search Any number contains "1234" 
   non-patched version costs about 12 seconds
   patched version costs near 20 seconds.

However, I must clarify that this result is against a LDAP server with about
50,000 entries on WAN. It does not show noticable performance loss if test with
a server locating in the same LAN and having about 250 entries.

The performance loss is obvious. However, this patch is still an reasonable fix:
1. It does solve the report problem, and it guarantee to search in a complete  
 scope user expect.
2. It has tried a lot to reduce the performance lost. As we have discussed, the
problem maybe not only locate in Clients. 
3. The common searching(including auto-completing) is not affected by the patch,
only the advanced search on LDAP will undertake such loss.

Thus I expect this patch can be in the source tree soon.
 

Updated

15 years ago
QA Contact: yulian → gchan

Updated

14 years ago
Blocks: 213274
Product: MailNews → Core
is bug and patch still valid? (Gao is gone)
Wayne: the patch has almost certainly bit-rotted such that it won't apply in its current form.  However, fixing it up to use the LDAP attribute map service shouldn't be too hard.  It would still have the same performance problems as it has the last time it was tested, however, so we need to figure out the best way to approach this patch.  Is this something you're interested in working on?
Assignee: sspitzer → nobody
OS: Windows NT → All
QA Contact: grylchan → ldap-integration
Hardware: PC → All
(Assignee)

Updated

9 years ago
Product: Core → MailNews Core
Keywords: helpwanted
Whiteboard: nab-search,nab-ldap → [patchlove][has draft patch]
Comment on attachment 104635 [details] [diff] [review]
New patch incorporating Dan's suggestions

Confirming bug has bitrotted.

$ patch --dry-run -p2 < ~/Desktop/126749.diff 
patching file src/nsAbBoolExprToLDAPFilter.h
Hunk #1 FAILED at 42.
Hunk #2 succeeded at 75 (offset 7 lines).
1 out of 2 hunks FAILED -- saving rejects to file src/nsAbBoolExprToLDAPFilter.h.rej
patching file src/nsAbBoolExprToLDAPFilter.cpp
Hunk #1 FAILED at 39.
Hunk #2 FAILED at 187.
Hunk #3 succeeded at 198 with fuzz 2 (offset -8 lines).
Hunk #4 succeeded at 309 (offset -8 lines).
2 out of 4 hunks FAILED -- saving rejects to file src/nsAbBoolExprToLDAPFilter.cpp.rej
can't find file to patch at input line 150
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|--- mailnews/addrbook/src/nsAbLDAPProperties.h Tue Apr  9 10:27:24 2002
|+++ mailnews.patch/addrbook/src/nsAbLDAPProperties.h   Mon Oct 21 18:25:00 2002
--------------------------
File to patch:
Attachment #104635 - Attachment is obsolete: true
http://getsatisfaction.com/mozilla_messaging/topics/new_attribute_in_the_query_string_via_ldap
Whiteboard: [patchlove][has draft patch] → [patchlove][has draft patch][gs]
You need to log in before you can comment on or make changes to this bug.