LDAP prefs are not correctly migrated

VERIFIED FIXED in mozilla0.9.2

Status

P1
normal
VERIFIED FIXED
18 years ago
10 years ago

People

(Reporter: bugzilla, Assigned: srilatha)

Tracking

(Blocks: 1 bug)

Trunk
mozilla0.9.2
All
Windows NT
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [PDT+] Have a fix)

Attachments

(8 attachments)

(Reporter)

Description

18 years ago
If I use a LDAP server from a migrated profile, it won't works. Apparently the 
prefs for the server URI has changed since 4.x

Updated

18 years ago
QA Contact: esther → yulian

Updated

18 years ago
Component: Composition → Profile Migration

Comment 1

18 years ago
I think Profile Migration is a better component for this.

Comment 2

18 years ago
reassigning to srilatha
Assignee: ducarroz → srilatha
(Reporter)

Comment 3

18 years ago
Here is was I have in my prefs:
user_pref("ldap_2.servers.InfoSpaceDirectory.csid", "UTF-8");
user_pref("ldap_2.servers.InfoSpaceDirectory.description", "InfoSpace Directory");
user_pref("ldap_2.servers.InfoSpaceDirectory.filename", "ldap.infospace.com4");
user_pref("ldap_2.servers.InfoSpaceDirectory.position", 5);
user_pref("ldap_2.servers.InfoSpaceDirectory.replication.lastChangeNumber", 0);
user_pref("ldap_2.servers.InfoSpaceDirectory.searchBase", "c=US");
user_pref("ldap_2.servers.InfoSpaceDirectory.serverName", "ldap.infospace.com");

and here is was I think we are suppose to have:
user_pref("ldap_2.servers.infospace.csid", "UTF-8");
user_pref("ldap_2.servers.infospace.filename", "ldap.infospace.com9");
user_pref("ldap_2.servers.infospace.replication.lastChangeNumber", 0);
user_pref("ldap_2.servers.infospace.searchBase", "o=netscape communications
corp.,c=us");
user_pref("ldap_2.servers.infospace.uri",
"ldap://ldap.infospace.com:389/o=netscape communications corp.,c=us");
(Assignee)

Updated

18 years ago
Blocks: 17880
(Assignee)

Comment 4

18 years ago
only the first directory server is getting migrated and the rest are not.

In your prefs after migration this is what you were supposed to have. 
user_pref("ldap_2.servers.InfoSpaceDirectory.csid", "UTF-8");
user_pref("ldap_2.servers.InfoSpaceDirectory.description", "InfoSpace Directory");
user_pref("ldap_2.servers.InfoSpaceDirectory.filename", "ldap.infospace.com4");
user_pref("ldap_2.servers.infospace.replication.lastChangeNumber", 0);
user_pref("ldap_2.servers.infospace.uri",
"ldap://ldap.infospace.com:389/o=netscape communications corp.,c=us");

I have patch. will post it right away.
Status: NEW → ASSIGNED
(Assignee)

Comment 5

18 years ago
Created attachment 33545 [details] [diff] [review]
patch v1
(Assignee)

Updated

18 years ago
Keywords: nsbeta1
Whiteboard: Have a fix
(Assignee)

Comment 6

18 years ago
ducarroz, can you try the patch and see if this will fix the problem.
You need to set the preference
ldap_2.prefs_migrated to false.
(Assignee)

Comment 7

18 years ago
Cc in bhuvan for a review
(Assignee)

Comment 8

18 years ago
ccing Seth for a sr

Comment 9

18 years ago
Priority P2, TFV 0.9.1.
Priority: -- → P2
Target Milestone: --- → mozilla0.9.1
note, we doing some sort of ldap pref migration already.

see nsPrefMigration.cpp

but, that only happens at profile migration. and if someone migrated from 4.x to
mozilla 0.9 or netscape 6.0, they won't go through that code again when they run
a newer build.

question: your code only gets run when ldap pref UI is brought up.

what happens I had certain prefs in 4.x, migrate to 6.x, but don't bring up the
pref UI first?  won't things fail since my prefs aren't migrated?  or are those
prefs only involved with the UI?

Updated

18 years ago
Priority: P2 → P1
(Assignee)

Comment 11

18 years ago
You are right about code gets run when ldap pref UI is brought up. 
Since, by default no server is selected for autocompletion, you need to go to
preferences to select a server. 
So, the prefs get migrated before you use it.

> by default no server is selected for autocompletio

what if I used an ldap server for autocomplete in 4.x, and then I migrated over
to 6.x?

does that case work as expected?
(Assignee)

Comment 13

18 years ago
Actually, the preference has changed from 4.x
In 4.x we used to have 
user_pref("ldap_2.servers.<server-name>.autocomplete.enabled", true);
but in 6.x it is
user_pref("ldap_2.autocomplete.directoryServer", <server-name>);
So the first time when you use a migrated profile, this preference does not 
exist. you will need to select a server again.
if that's the case, then you haven't fixed the problem.

LDAP prefs are still not correctly migrated.

instead of doing the migration in the .js for the UI, you should figure out the
proper place to migrate the prefs so that if I used LDAP autocomplete in 4.x, it
works in 6.x.

nsPrefMigration.cpp is not the correct place for that, since if the user has
already migrated (to mozilla 0.9 or netscape 6.0) they won't be going through
that code again.

if we do it right, a user will get and install mozilla 1.0 / 6.x, and LDAP
autocomplete will just start working as it did in 4.x, assuming they migrated
their 4.x profile.

(Assignee)

Comment 15

18 years ago
Migrating the LDAP prefs the first time compose window, And in migrate()
checking for the autocompletion.enabled pref for each server and initialize
ldap_2.autocomplete.directoryServer should fix this.

that makes sense.  instead of duplicating the code, make sure to put the code in
the proper service (any ideas where?) and then call it from both places.
(Assignee)

Comment 17

18 years ago
Created attachment 34143 [details] [diff] [review]
This patch fixes this bug and bug #79777

Comment 18

18 years ago
Srilatha,

Your changes look good. Couple of things from my end :

* Don't you still need to incorporate your new LDAP js file in (xpinstall)
packages files to have it exported properly into the builds

* looks like LDAPPrefsService is defined but not used in MsgComposeCommands.js
file..

r=bhuvan

It will be better to get Candice's review on LDAP specific code segments as I am
no expert there.
(Assignee)

Comment 19

18 years ago
You are right, I need add the js file to packages.
Eventhough we are not using LDAPPrefsService in MsgComposeCommands.js, when we 
call the constructor we are migrating the 4.x LDAP prefs and initializing the 
nsLDAPService. This is essential when the user opens the mail compose window 
before going to preferences.
I am building on mac and I have some mac specific stuff to add to the patch.
Will post an updated patch soon.

(Assignee)

Comment 20

18 years ago
Created attachment 34630 [details] [diff] [review]
update the packages files

Comment 21

18 years ago
Setting target milestone to 0.9.2 (check it in anytime, even before, when the
tree is open for). Per PDT triage.
Target Milestone: mozilla0.9.1 → mozilla0.9.2
general comments
-----------------
Any LDAP dependencies need to be conditionalized, so that building
without MOZ_LDAP_XPCOM defined generates a still-working, LDAP-free
build.  This means that most Makefile.in and makefile.in changes will
need to be tweaked, as well as some Mac magic.  On Unix, be sure to
conditionally tweak the REQUIRES lines as well, so that
MOZ_TRACK_MODULE_DEPS builds continue to work.  See
xpfe/components/{build,autocomplete/*} for examples.  It may also be
necessary to have JS detect whether or not this is an LDAP build
before deciding to execute LDAP-dependant code.

When adding new files, at least, can you wrap your code at 80 columns
for readability?

Unfortunately, dump() sends stuff to the console not just in debug
builds, but in release builds as well.  So if you could take out the
calls to dump(), that would be good.

There seem to be a fair number of explicit |catch| clauses which don't
do anything but silently ignore the error, and attempt to continue
executing with obviously horked data (eg null interfaces) rather than
returning.  In many of these cases, I presume the right thing to do is
to either not |catch| at all and let the exception unwind up the
stack, or else give the user some sort of other feedback that there
was a problem.  Can you go through the catches and fix these things in
the appropriate ways?

nsILDAPPrefsService.idl:
------------------------
license nits: you've got an extra < before your name; the copyright
year is wrong.

misc nits: I think "6.x format" by the migrate decl() should really be
"mozilla format"; the "serves" at the interface comment should be
"servers"

There appear to be spacing problems in and around the comments; I bet
there are some tabs there.  

nsISupportsArrays do not map directly to JS arrays.  If you want to
use them from JS, you need createInstance the nsSupportsArray
contractid then call the nsISupportsArray interface methods to
manipulate it.  So if anyone tries to use the getter and setter for
availDirectories, it'll do something evil and won't work.  The arrays
that do map to JS arrays directly are raw XPCOM arrays; see
nsILDAPMessage.idl and nsLDAPDatasource.js for examples of that in
use.  Note that XPIDL doesn't currently supports arrays being
attributes, so getters and setters need to be declared directly.  All
that said, as far as I can tell, none of the attributes listed in
nsILDAPPrefsService.js are actually used outside of that file -- they
seem to be just private internal properties of that component.  So
could they actually be removed from the interface altogether -- or
do you anticipate needing to access them from outside in some case?

It looks to me like the deleteServer method is just there to provide
symmetry with the addServer method.  Rather than hanging these two
methods off nsILDAPPrefsService service, how about just adding a createServer
helper method onto nsILDAPService itself, and then calling into the
nsILDAPService directly for all three things?

.migrate():

It'd be nice if there were a comment somewhere that explained what the
significance of the 2 in ldap_2 is ...

>+      ldapUrl = Components.classes["@mozilla.org/network/ldap-url;1"];
>+      ldapUrl = ldapUrl.getService(nsILDAPURL);

LDAP URLs are not singletons, so instead of getService, use
createInstance.

It looks like at most one directory server will be enabled due to the
use of the enable variable.  I assume that's intentional, but it's not
obvious from reading the code.  If it is intentional, can you please a
comment indicating that?  In general, more comments are likely to make
for faster reviewer turnaround, I suspect.
Depends on: 84004

Comment 23

18 years ago
PDT+ per 6/12 mtg.
Whiteboard: Have a fix → [PDT+] Have a fix
(Assignee)

Comment 24

18 years ago
Created attachment 38313 [details] [diff] [review]
patch based on dmose's comments
(Assignee)

Comment 25

18 years ago
Few things in this patch that are different from the previous one
nsLDPAPrefsService does not have the nsLDAPService stuff since we decided to 
drop ldapservice.
added ifdef MOZ_LDAP_XPCOM to the makefiles.
removed dumps. probably added a couple ( when prefs service or ldap url 
fails).
Fixed the "Catch" caluses
Fixed license brb

(Assignee)

Comment 26

18 years ago
Created attachment 38315 [details] [diff] [review]
removed tabs
Because you're calling migrate() from the constructor, when getService() is
called from MsgComposeCommands.js, it may have the side effect of doing the
migration.  This seems a bit unintuitive to me; if you could add a comment to
MsgComposeCommands.js explicitly mentioning that just getting the service may
have this possible side-effect, you've got r=dmose@netscape.com.  No need to
post another patch here.

I don't think it matters a lot in this particular case, so it's not important to
me that you change this here, but this is worth keeping in mind going forward:

One issue with the XPCOM model is that it's impossible to communicate any sort
of failure back from an object's constructor, because the failure can only get
communicated back to the factory, not to whoever actually asked for the
component to be instantiated.  The general workaround for this is that in XPCOM
components, one never does anything that could fail inside the constructor, but
instead moves such initialization to a separate init() method which any clients
call manually.  
(Assignee)

Comment 28

18 years ago
Created attachment 38951 [details] [diff] [review]
hopefully final patch
(Assignee)

Comment 29

18 years ago
made change to Makefile.in so that it builds on linux
Modified pref calls so that we save Multibyte character values right.

I have built this on mac too. Will post a patch for that soon
Depends on: 71247
I notice that this version no longer modifies pref-addressing.xul.  I assume
this is intentional?

My only concern is this:

++      if (dn && ldapService)
++        ldapUrl.dn = ldapService.UCS2toUTF8(dn);

This means that if the LDAP Service is unavailable, it will migrate everything
_except_ the DN, but mark the preferences as migrated.  This seems like
confusing behavior to me.  Aborting the entire migration if ldapService is
unavailable seems more reasonable to me, since it doesn't leave the user with
half-baked prefs.

r=dmose@netscape.com if you address these concerns; no new patch necessary if
those are the only changes.
(Assignee)

Comment 31

18 years ago
pref-addressing.xul: Yes it is intentional. I had this change in my tree 
(removes javascript error). It is a fix for bug 79780.

Will make the change such that we return if ldapservice is not available
if (dn && ldapService)
  ldapUrl.dn = ldapService.UCS2toUTF8(dn);
else
 return;
(Assignee)

Comment 32

18 years ago
Created attachment 38972 [details] [diff] [review]
patch for mac builds

Comment 33

18 years ago
Please do not use the nsIPref interface. It has been depricated in favor of
nsIPrefService and nsIPrefBranch.
(Assignee)

Comment 34

18 years ago
After talking to Brian, he gave me an o.k on using nsIPref since these methods 
in nsIPrefBranch and nsIPrefService are not usable from js.
(Assignee)

Comment 35

18 years ago
Created attachment 39015 [details] [diff] [review]
patch for the pacakaging stuff
looks good.

I'd add a call to flush out the prefs file to disk after you are done migrating.

something like:

+ gPrefInt.SetBoolPref("ldap_2.prefs_migrated", true);
+ gPrefInt.savePrefFile(null);

that's been the common practice:  migrate, save prefs.

do that, and sr=sspitzer
a=dbaron for trunk checkin (on behalf of drivers) assuming above comments have
been addressed
(Assignee)

Comment 38

18 years ago
Fix checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
(Reporter)

Comment 39

18 years ago
This fix as caused regression bug 86996!
(Reporter)

Comment 40

18 years ago
oops, wrong bug number! This is causing regression bug 86966

Comment 41

18 years ago
Verified on Windows, Linux and Mac platforms.
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.