Bugzilla – Full Text Bug Listing
| Summary: | When 2 profiles with same driver id, it will get some odd behavior | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] Data Tools | Reporter: | Mingxia Wu <mwu> | ||||||||||||||||||||||||||||||||
| Component: | Connectivity | Assignee: | Brian Fitzpatrick <bfitzpat> | ||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||||||||||||||||||||
| Severity: | normal | ||||||||||||||||||||||||||||||||||
| Priority: | P3 | CC: | bfitzpat, lchan, ledunnel, tzhang | ||||||||||||||||||||||||||||||||
| Version: | Ganymede | ||||||||||||||||||||||||||||||||||
| Target Milestone: | 1.6.1M1 | ||||||||||||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||||||||||||
| OS: | Windows XP | ||||||||||||||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||
That's bizarre. I'll take a look and see what I can come up with. So when you create your "Other Driver" instances for db2 & SQL Server, you're just naming them "Other Driver"? Can you send me some steps to reproduce this in a clean environment? And maybe attach your .metadata\.plugins\org.eclipse.datatools.connectivity\driverStorage.xml file to this bug? Brian, The test case occurs when loading an exported connection profile file in a server runtime environment. So no driver definitions pre-exist in driverStorage.xml. The DriverInstance gets created automatically by DriverManager based on the driver reference properties found in the exported connection profile instances. I think multiple exported files were used, with each saved from a different client workspace that happen to use the same driver name for different vendors. So when the first exported file gets loaded, a new DriverInstance is created in the server workspace for the corresponding vendor. Then when loading the second exported file that contains a connection profile which references the same driver name but different vendor, the DriverManager found the "wrong" DriverInstance by name. Perhaps when checking for existing DriverInstance for a connection profile instance, it should validate against the vendor and version properties as well. It would be nice if a DriverInstance can be uniquely identified by name, vendor and version. If this approach means too much changes at this point, at least an error should be logged and skipped that connection profile instance. It would also help if the Driver template includes a vendor and version prefix to the suggested driver name. I hate to belabor the point, but uniquely identifying a driver instance by name, vendor, and version isn't a good solution unless it's all part of the driver ID. I'll see if I can work that in. But I don't want to lock drivers down to just database vendor/version, since that's not the only place these are used. So let me clarify some things based on your last comment, Linda... You have multiple import files. Let's say for the sake of discussion we have two export files from two different client workspaces. Each has "Other Driver" as a named driver. How are we getting them imported? via the command line app or via the Import dialog? --Fitz What I'd really like to have is two export files so I can easily reproduce this. I think I have a fix, but I need to be able to repro it before I can actually test the fix. Brian, The "importing" was done programmatically thru oda.profile calling ConnectionProfileMgmt.loadCPs( File ). Is your fix going to support creating separate DriverInstance, or just validate and skip a connection profle? Tianli, can you please provide a couple of your connection profile files (that use DTP driver types) that can reproduce this issue? Created attachment 107548 [details]
sqlserver connection profile
SQLServer connection profile
Created attachment 107549 [details]
DB2 connection profile
DB2 connection profile
Created attachment 107552 [details]
Oracle connection profile
I tested it again, this connection profile can also reproduce the issue when working with DB2 connection profile
Thanks for the attachments. I'm working at this from two angles... I now have it working (I think -- will post a patch soon for testing) where you can no longer create entries with duplicate names in the driver list. So that's one part of it. The other part is the import, which I'll tackle now that I have some sample data to play with. Created attachment 107670 [details]
patch
Ok... I think this fixes the issue from the UI perspective, and since the UI uses the same APIs you are using internally, we should be good there too.
This does a couple of things...
1) It changes the driver ID to include the template ID as part of it. This makes each driver definition much more unique, to avoid having duplicate Other Driver instances among other things.
2) It changes the import so that if the driver ID is updated when imported, it successfully sets the driver ID in the connection profile to which it was connected.
3) It fixes a validation issue that was occurring in the Driver Dialog where the order of validation sometimes masked the fact that the name wasn't right (i.e. you could have multiple Other Driver driver definitions with the same name and it would just overwrite them).
So... If you would be so kind as to test this patch, I'd be very appreciative. I've tested it as I said from the UI side, but not from the ODA side. Just want to make sure I didn't break anything there.
Thanks in advance
--Fitz
Created attachment 107671 [details]
mylyn/context/zip
Brian, I'd tried to apply the patch in my workspace (which has latest HEAD version). But some reason, got some unmatched segments on the file ConnectionProfileMgmt.java. I don't know the changes well enough to do a manual merge. Can you attach the complete patched ConnectionProfileMgmt.java ? Created attachment 107679 [details]
Updated Java file
That's odd. I wonder why it had unmatched sections. I've sync'd up a few times today, so I know it should be current.
But anyway... I'm attaching the modified file. The changes are all in that readCPsFromXML1_0 method.
--Fitz
Thanks. Brian. I'd applied the patch files, and tested so far with the IDE and DSE. It looks like there may be some backward compatibility issues. I figure to bring this up first, before testing on the BIRT runtime environment. For example, let's say my IDE workspace has previously created some connection profiles and thus corresponding Driver definitions. Now w/ the patch applied, when I re-start the IDE and DSE with the same workspace, it triggers creating new driver definitions based on the new driver id. So now multiple DriverInstance exist with the same driver name showing up in the Drivers combo box on the New Connection Profile wizard page. The same issue applies to the default driver definition that got created based on the "org.eclipse.datatools.connectivity.ProfileManagerInitializationProvider" extension point. It triggers the creation of new driver definition, if none already exists in workspace (in InternalProfileManager#loadLocalRegisteredDatabases() ). A separate minor issue is that the new id can probably use a separator between the "templateID" and "name" value; e.g. String id = prefix + templateID + "." + name; The new id looks something like this w/o the separator: DriverDefn.org.eclipse.datatools.enablement.mysql.4_0.driverTemplateMySQL JDBC Driver Beautiful. I love backward compatibility issues. <sigh> I'm guessing that there must be a way to only spontaneously create these driver definitions during an import, but I'm not sure what it is yet. Took me a while just to get what you're testing now working. I have to admit no knowledge of the ProfileManagerInitializationProvider, but will take a look. As far as the ID, yeah, that's a minor issue. But an easy one to fix. Let me play with this some more and get back to you sometime tomorrow. Thanks for testing and your feedback. --Fitz Created attachment 107758 [details]
Patch take 2
Ok... I think I've fixed the issues you discovered. Can you give this a shot? It better handles the case of existing driver definitions/profiles in the workspace. And the driver ID has been modified to include a "." to make it a little more readable.
Brian, The "take 2" patch does fix the first use case when re-loading the connection profiles from an existing workspace. However, the issue with creating duplicated DriverInstance still exists for the second use case that creates a driver definition for the default connection profile specified in the ProfileManagerInitializationProvider extension point. In debugging this use case, I find this call sequence stack trace: DriverManager.addDriverInstance(DriverInstance) line: 400 DriverManager.addDriverInstance(IPropertySet) line: 420 DriverManager.createNewDriverInstance(String, String, String) line: 356 InternalProfileManager.getDriverInstance(String, String, String) line: 1331 InternalProfileManager.enableLocalDatabase(IConfigurationElement[], String, String, String, String, String) line: 1250 InternalProfileManager.loadLocalRegisteredDatabases() line: 1209 InternalProfileManager.loadProfiles() line: 918 InternalProfileManager.getProfiles(boolean) line: 135 ProfileManager.getProfiles() line: 46 In #addDriverInstance, line: 400 mDriverInstanceMap.put(di.getId(), di); ends up adding a new entry in the map with the new id format as the key. Before the patch, it used to update the existing map entry (found from the existing workspace) with the old id format as the key. Well at least we're getting closer... I'll take a look at the Profile initializer code today and see if I can figure out what's going on. Thanks for the stack trace! Created attachment 107837 [details]
Internal Profile Manager patch to fix issue with creating BIRT default db
Hey Linda...
After I poked around a bit this morning, I discovered that the code in InternalProfileManager that enables the sample database was doing something very odd. It was looking for the driver definition by the template ID, which is very different than the driver ID. So I switched it to look for it by name and voila, it finds it fine.
So I believe this whole driver ID thing just bubbled up a bug we didn't know about before. This patch should fix it.
Let me know
--Fitz
I'm getting a little confused about what changes are being proposed here. Is your patch now relying on the Driver Definition being uniquely named? If so I think that this will be problematic because there are many ways for Driver Definitions to be created and I am relying on the fact that in my product I can create driver definitions with the same name. I don't think this behavior should be changed in a maintenance release. I don't really understand the logic for the driver definition ID generation. If the definition name is not unique (see above) then the ID will not be unique. Since the purpose of the ID is to uniquely identify the driver definition it could simply be a generated unique number. It seems to me the simplest fix is leave the current ID generation logic and append a number to the end of the driver definition ID when it is created or imported to guarantee uniqueness. As far as the problem with the Internal Profile Manager, is that a different defect or a side-effect of the other proposed patch? If it is a different defect, I would prefer if a new defect were opened so that the engineer that originated the Profile initializer code could take a look into the issue first. My impression of the Profile initializer code was that it was trying to create a unique instance of the Derby driver template in this case. The problem was that the initializer code was checking for DriverInstance driverInstance = DriverManager.getInstance().getDriverInstanceByID(driverTemplateID); And the getDriverInstanceByID method is meant to find a driver definitions by the driver ID, not by the driver template ID. There's a second method for that that returns a list of driver definitions that use that template ID. So we could instead of going by name, search for ANY drivers that use that template and simply take the first one. That would be another way to go. Brian, Instead of changing the behavior by using the name, I suggest we just fix that one line of code to call getDriverInstancesByTemplate(). Created attachment 108007 [details]
Updated patch for InternalProfileManager
This addresses the concerns about uniqueness of the driver instance -- it tests for driver template ID, then sees if the driver instance is valid, then checks the name. if all three conditions are met, we have a bingo. otherwise, it creates a new one.
Brian, In testing with the IBM product with Patch_take_2 and Updated_patch_for_InternalProfileManager, I find that the BIRT sample database generates a new driver definition everytime Eclipse is started (BIRT SampleDb Derby Embedded Driver1, BIRT SampleDb Derby Embedded Driver2, BIRT SampleDb Derby Embedded Driver3...). If I delete *all* the Derby driver definitions then restart, it creates the expected driver definition name (BIRT SampleDb Derby Embedded Driver) without the number and then doesn't generate new driver definitions after that. Brian, I was able to repro the problem in comment #26 using the BIRT all-in-one download as the basis for my dev environment. So even though the original BIRT sample derby driver was valid, it was failing to find it? I'll try it with the BIRT all in one today. Created attachment 108079 [details]
Updated patch, take 2, for InternalProfileManager
Ok... This should fix it. (Have we heard this before? Anybody?)
--Fitz
Brian, I'm not sure which patches I should apply to test the fix. Can you mark any obsolete patches as obsolete? Sorry about that. I've been appending patches via Mylyn's Bugzilla connector and it doesn't have a simple way of marking them obsolete. But it's done now. The Patch Take 2 and Updated patch, take 2 are the two patches to apply. Brian, I get conflicts when I try to apply the second patch. Do you want to create a cumulative patch? The only thing affected by the second patch is the InternalProfileManager. Can you revert to the current version in CVS and then apply the new patch? I can certainly create a cumulative patch, but the first patch doesn't touch InternalProfileManager, so I'm not sure why it would conflict. Brian, I can't really explain it except it I can't apply it. It's acts like the patch was not built against the code in HEAD. If you could make a new cumulative patch, it might work. Created attachment 108099 [details]
Cumulative patch
Here ya go. I just tried this patch by reverting to all HEAD code, then re-applying it and it worked fine for me.
Brian, I tried the cumulative patch in three Ganymede(3.4) and one Europa(3.3) development environments and it still shows a conflict in InternalProfileManager. Created attachment 108106 [details]
Cumulative patch generated in 3.3.2 as opposed to 3.4
Ok... I haven't a clue why the last two patches (the last two patches I created were in 3.4, not 3.3.2) aren't working for you. I'm re-generating the patch in my 3.3.2 environment. I've been able to revert to base and apply it.
Brian, Still no luck. How about you attach your patched copy of InternalProfileManager.java to the defect? I can then apply the first patch and the file. Created attachment 108112 [details]
Patched InternalProfileManager
Wow. I haven't a clue why that's happening. But here's the InternalProfileManager.
Testing with the Patch_take_2 and Patched_InternalProfileManager succeeded. Feel free to deliver the changes. Linda, do the same patches work for you? if so, I'll go ahead and deliver I will test and let you know. I'd tested with the latest patch. It addressed all the issues raised in comment #16. Next we should also test the patch for the original use case in a server deployment environment. Brian, can you create and attach the related plugin jars with the patch applied, so Tianli can test them out? (For some reason, my workspace is having problem with building the plugin jars.) Created attachment 108205 [details]
Jarred plug-ins
Here's the two plug-ins jarred and then zipped up. It's o.e.d.connectivity and o.e.d.connectivity.ui.
Let me know if you have any issues with them. I created a fresh 3.3.2 environment and dropped in the latest 1.6.1 M1 nightly build to generate these two plugins, so hopefully it'll work.
--Fitz
I tested the related cases by using attached patch in #comment 44 in my runtime environment, the issue was resolved. If any other requirement please let me know:) Thanks Tianli... Linda, should I deliver now that you, Larry, and Tianli have all approved the fix? (In reply to comment #46) Please do. Thanks! Delivered to o.e.d.connectivity and o.e.d.connectivity.ui as tag v200807250445 |
Created attachment 107159 [details] snapshot of debug The step to reproduce: 1)Open one db2 profile's report in Birt runtime, it is ok. 2)Open one sqlserver prfile's report, got the exception to say "Connection failed with unspecified error" But it is so amazing if firstly opening sqlsesrver profile's before db profile's, it works. From my investigation, the exception is thrown in org.eclipse.datatools.connectivity.DriverConenctionBase's internalCreateConnection() method, about line 104.The code tried to retireve all the properties and get a connection from jdbc driver.But the DriverInstance we got is db2 profile, not sqlserver.The reason is that the db2 prfiles driverID and sqlserver's is actually same, they were all "DriverDefn.Other Driver".So got the same DriverInstance from the DriverManager instance. Attachment is the snapshot of my debug result.