Last Comment Bug 126749 - Department number search fails for LDAP Directory ?
: Department number search fails for LDAP Directory ?
Status: NEW
[patchlove][has draft patch][gs]
: helpwanted
Product: MailNews Core
Classification: Components
Component: LDAP Integration (show other bugs)
: Trunk
: All All
-- normal with 6 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
http://getsatisfaction.com/mozilla_me...
: 119143 (view as bug list)
Depends on:
Blocks: 213274
  Show dependency treegraph
 
Reported: 2002-02-20 12:34 PST by yulian chang
Modified: 2011-08-24 16:38 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch to generate OR list of all matched LDAP attributes for Search (17.07 KB, patch)
2002-03-07 08:28 PST, John Marmion
no flags Details | Diff | Splinter Review
Remove stray fprintf() from previous patch. (16.99 KB, patch)
2002-03-07 08:56 PST, John Marmion
no flags Details | Diff | Splinter Review
Keep patch up-to-date (16.99 KB, patch)
2002-03-21 05:35 PST, John Marmion
no flags Details | Diff | Splinter Review
Keep patch up-to-date (16.91 KB, patch)
2002-04-04 04:58 PST, John Marmion
no flags Details | Diff | Splinter Review
proof of concept (18.41 KB, patch)
2002-09-16 07:23 PDT, Sean Gao
no flags Details | Diff | Splinter Review
New proof of concept patch incorporating testing for exist (49.60 KB, patch)
2002-10-21 10:07 PDT, John Marmion
no flags Details | Diff | Splinter Review
New patch incorporating Dan's suggestions (14.00 KB, patch)
2002-10-30 09:14 PST, John Marmion
no flags Details | Diff | Splinter Review

Description User image yulian chang 2002-02-20 12:34:22 PST
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
Comment 1 User image yulian chang 2002-02-20 13:31:14 PST
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"
Comment 2 User image John Marmion 2002-02-28 09:36:13 PST
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 User image John Marmion 2002-03-07 08:28:44 PST
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 User image John Marmion 2002-03-07 08:56:01 PST
Created attachment 72982 [details] [diff] [review]
Remove stray fprintf() from previous patch.
Comment 5 User image Kristof Petr 2002-03-19 14:41:09 PST
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 User image John Marmion 2002-03-21 05:35:02 PST
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?
Comment 7 User image John Marmion 2002-04-04 04:58:13 PST
Created attachment 77634 [details] [diff] [review]
Keep patch up-to-date
Comment 8 User image John Marmion 2002-08-09 05:47:01 PDT
*** Bug 119143 has been marked as a duplicate of this bug. ***
Comment 9 User image Sean Gao 2002-09-16 07:23:03 PDT
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 User image John Marmion 2002-09-18 06:39:11 PDT
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 User image John Marmion 2002-09-18 06:45:28 PDT
Sorry that last commment  should read:

(|(ou=*123*)(&(!(ou=*))(departmentNumber=*123*)))
Comment 12 User image John Marmion 2002-09-19 09:05:17 PDT
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 User image John Marmion 2002-10-21 10:07:47 PDT
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.
Comment 14 User image Dan Mosedale (:dmose) 2002-10-23 15:08:52 PDT
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 User image John Marmion 2002-10-30 09:14:04 PST
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.
Comment 16 User image Sean Gao 2002-11-14 05:27:39 PST
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.
 
Comment 17 User image Wayne Mery (:wsmwk, NI for questions) 2005-11-01 08:29:34 PST
is bug and patch still valid? (Gao is gone)
Comment 18 User image Dan Mosedale (:dmose) 2005-11-02 11:21:18 PST
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?
Comment 19 User image Gary Kwong [:gkw] [:nth10sd] 2010-03-11 23:46:44 PST
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:

Note You need to log in before you can comment on or make changes to this bug.