yulian chang Reporter
	Description • 23 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

	yulian chang Reporter
	Comment 1 • 23 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"

	John Marmion
	Comment 2 • 23 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

	John Marmion
	Comment 3 • 23 years ago

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.

	Kristof Petr
	Comment 5 • 23 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},

	John Marmion
	Comment 6 • 23 years ago

Thanks Petr,

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

Have you tested this patch?

	John Marmion
	Comment 8 • 22 years ago

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

	Sean Gao
	Comment 9 • 22 years ago

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.

	John Marmion
	Comment 10 • 22 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*)))

	John Marmion
	Comment 11 • 22 years ago

Sorry that last commment  should read:

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

	John Marmion
	Comment 12 • 22 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..

	John Marmion
	Comment 13 • 22 years ago

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.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 14 • 22 years ago

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?

	John Marmion
	Comment 15 • 22 years ago

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.

	Sean Gao
	Comment 16 • 22 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.
 

	Wayne Mery (:wsmwk)
	Comment 17 • 19 years ago

is bug and patch still valid? (Gao is gone)

	Dan Mosedale (:dmosedale, :dmose)
	Comment 18 • 19 years ago

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?

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 19 • 15 years ago

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:

	Wayne Mery (:wsmwk)
	Comment 20 • 14 years ago

http://getsatisfaction.com/mozilla_messaging/topics/new_attribute_in_the_query_string_via_ldap