Fix issue with Alter column in Oracle #392

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

swalters commented Mar 22, 2013

Alter Column defaulted text to "Not Null" if the column was not nullable
(the default)
Alter.Column("SystemName")
.OnTable(AlertDefs)
.AsString(255);
would result in
ALTER TABLE stAlertDefs MODIFY SystemName NVARCHAR2(255) NOT NULL

This is not allowed by Oracle because the column is already not null.

Solution was to make IsNullable a Nullable and only add "Not Null"
to the alter if .NotNullable() was specified. This should only affect
Oracle.

swalters added some commits Mar 21, 2013

Fix issue with Alter Column for Oracle
Alter Column defaulted text to "Not Null" if the column was not nullable
(the default)
Alter.Column("SystemName")
.OnTable(AlertDefs)
.AsString(255);
would result in
ALTER TABLE stAlertDefs MODIFY SystemName NVARCHAR2(255) NOT NULL

This is not allowed by Oracle because the column is already not null.

Solution was to make IsNullable a Nullable<bool> and only add "Not Null"
to the alter if .NotNullable() was specified.  This should only affect
Oracle.
Default Oracle Identifier quoting is no quotes
Still need to add a context property to specify which quoter to use
update all projects to dotnet 4.0 - Do not push to main repository
Not sure why, but the default build says it will create a distribution
of dotnet 4.0, but they are compiled to 3.5.
Contributor

daniellee commented May 20, 2013

@swalters Is this good to merge? Are you going to test this @tommarien? Same old problem that I can't really review Oracle stuff. Looking at the diffs it looks fine. Whether the project is .NET 4 or 3.5 doesn't make a huge difference as long as we don't use any .NET 4 features that will break compatibility with 3.5.

And what's happening with the case-sensitivity problem? Should I pull in JuliusOnGitHub/fluentmigrator@master...OptionalOracleQuoter instead of this PR (as it includes it)?

Contributor

swalters commented May 20, 2013

I think it would be fine to pull JuliusOnGitHub/fluentmigrator@master...OptionalOracleQuoter instead of this PR.

We've been running Alter Column PR in production against SqlServer and Oracle with no issues.

Contributor

daniellee commented May 20, 2013

Hi @swalters and @JuliusOnGitHub,

Getting errors for the Oracle factory tests:

Oracle.DataAccess.Client.OracleException : The provider is not compatible with the version of Oracle client

I presume the Oracle.DataAccess.dll we have in the lib directory is as old as the hills. Can I just update it or do I have to install the Oracle client for the factory tests to work? 😱

Contributor

daniellee commented May 20, 2013

@swalters are you running the @JuliusOnGitHub branch or your own PR in production? Can you test the provider switch logic for the Oracle quoter?

Contributor

swalters commented May 20, 2013

No, I'm running my own branch. I'll try to test the @JuliusOnGitHub branch by tomorrow afternoon US Central time.

Concerning the Oracle client exception, you can use your version of Oracle.DataAccess.dll that you have installed with your Oracle client. Easiest thing to do is copy your oracle dll to the bin/debug folder.

Contributor

daniellee commented May 20, 2013

@swalters that's the problem. I don't have any Oracle client installed. I know absolutely nothing about Oracle. Any tips on where to get started? Should I install the Express Edition? I was hoping @tommarien would test the Oracle changes but I guess the time has come for me to bite the bullet.

When you are testing the branch could you have a look at the merge conflict in OracleProcessor.cs. An example:

The current version:

return Exists("SELECT 1 FROM \"USER_TABLES\" WHERE \"TABLE_NAME\" = '{0}'",FormatSqlEscape(tableName));

Your version:

return Exists("SELECT 1 FROM USER_TABLES WHERE upper(TABLE_NAME) = '{0}'", tableName.ToUpper());

Is it correct to use FormatSqlEscape here? Should the merged version look like this:

return Exists("SELECT 1 FROM USER_TABLES WHERE upper(TABLE_NAME) = '{0}'", FormatSqlEscape(tableName.ToUpper()));

Regarding the Oracle.DataAccess.dll version, I'm thinking that when we divide FluentMigrator up into database-specific packages that we should fetch this dependency from the ODP.NET nuget package? Rather than having a useless out-of-date dll in our lib folder.

Contributor

swalters commented May 20, 2013

@daniellee

The Oracle.DataAccess.dll is only there for compilation purposes. From my experiences, providing the correct Oracle.DataAccess.dll that would work for everyone would be impossible. It depends on the exact version of Oracle client that is installed. We have to put Assembly binding redirects in our apps and change those at each installation depending on the Oracle client version of the customer.

As far as Oracle integration testing, Oracle Express would be the way to go. If you need any help installing it or getting the tests running, shoot me an email swalters @ singletreetech .com

Contributor

swalters commented May 22, 2013

@daniellee , I was able to test the @JuliusOnGitHub branch against Oracle and all the tests passed. I also tested it against all of our migrations and found no issues.

Contributor

daniellee commented May 22, 2013

Great. I'll get that merged in this evening then.

Contributor

daniellee commented May 22, 2013

Merged in @JuliusOnGitHub's branch manually. Hope all the Oracle stuff still works now!

@daniellee daniellee closed this May 22, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment