Fix for Issue #106

[sparkybg]

Fix for Issue 106, as described in the issue.

[roji]

@sparkybg how about adding a test for this?

[sparkybg]

And what should we test for?

I am "testing it" for more than 2 years on production server now.

Here's what happens when I use postgres (with applied resolution): ALTER TABLE "testtable" ADD "Test" int4 NOT NULL DEFAULT 0

SQL Server: ALTER TABLE [testtable] ADD [Test] [int] NOT NULL DEFAULT 0

MYSql: alter table testtable add column Test int not null

MySQL also does not have "DEFAULT 0" at the end, but it still puts zeroes to the existing records. It seems that it has internal default values for at least "int" type. Postgres throws an error instead, which is what issue #106 is all about.

In fact I was looking at SQL Server output for the same migration in order to figure out what should be corrected.

Here's the code from SQL Server provider for EF6 (which IMHO is the "golden standart" for all other implementations):

       protected virtual void Generate(AddColumnOperation addColumnOperation)
        {
            Check.NotNull(addColumnOperation, "addColumnOperation");

            using (var writer = Writer())
            {
                writer.Write("ALTER TABLE ");
                writer.Write(Name(addColumnOperation.Table));
                writer.Write(" ADD ");

                var column = addColumnOperation.Column;

                Generate(column, writer);

                if ((column.IsNullable != null)
                    && !column.IsNullable.Value
                    && (column.DefaultValue == null)
                    && (string.IsNullOrWhiteSpace(column.DefaultValueSql))
                    && !column.IsIdentity
                    && !column.IsTimestamp
                    && !column.StoreType.EqualsIgnoreCase("rowversion")
                    && !column.StoreType.EqualsIgnoreCase("timestamp"))
                {
                    writer.Write(" DEFAULT ");

                    if (column.Type == PrimitiveTypeKind.DateTime) 
                    {
                        //this is because SQL Server does not support DateTime lower than 
                       //1st of January 1753 and ClrDefaultValue for DateTime in C# is 1st of January of year 1
                       //Postgres does not have problem with this, so this check is not needed and
                       //ClrDefaultValue for C# DateTime can also be used directly
                        writer.Write(Generate(DateTime.Parse("1900-01-01 00:00:00", CultureInfo.InvariantCulture)));
                    }
                    else
                    {
                        writer.Write(Generate((dynamic)column.ClrDefaultValue)); //This is what the resolution does
                    }
                }

                Statement(writer);
            }
        }

[sparkybg]

@sparkybg how about adding a test for this?

Uh, sorry for misunderstanding. The tests have to be altered also.

And, two more modifications for "AlterColumnOperation" and "CreateTableOperation" along with their tests must be added also for consistency.

Is there a way to add them to this pull request or should I create a new one?

Edit: Tried. Runs as it should.

[roji]

Is there a way to add them to this pull request or should I create a new one?

It's generally better to evolve an existing PR rather than closing and creating a new one. You can always push more commits to the PR branch - that automatically updates the PR. In some cases you'll want to "rewrite history" (e.g. rebase the PR branch on the latest version of the main branch), and in that case you need to force-push to the branch (git push -f). That's also fine - the PR just shows whatever's in the branch, so modify the branch in any way you want.

[sparkybg]

Yes, I figured it out, but as I said, I closed the other Pull request by mistake and didn't find a way to reopen it.

Anyway, everything considering #106 both in NpgsqlMigrationGenerator.cs and tests should be OK in the new pull request.

Sorry again - my mistake. I am working with TFS most of the time and have some bad habits. :)

[sparkybg]

Issue #106 is present again, and modifications to correct it from this pull request are lost.

Please advice what to do.

[roji]

If the changes are lost in this PR (because of force-pushing), and you don't have them locally in your own git branch, then I'm afraid there's not much we can do...

[sparkybg]

The pull I created after this is still present: https://github.com/npgsql/EntityFramework6.Npgsql/pull/163

But still it seems it is not pushed at all.