Skip to content

Fix migration bug with PostgreSQL 9.5 #607

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
austindrenski merged 1 commit into from
Aug 23, 2018
Merged

Fix migration bug with PostgreSQL 9.5 #607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -918,6 +918,32 @@ protected override void Generate(
builder.EndCommand();
}

protected override void Generate(CreateSequenceOperation operation, IModel model, MigrationCommandListBuilder builder)
Copy link
Member

@roji roji Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to simply call base.Generate() with a CreateSequenceOperation that has ClrType overridden to long? This would prevent the duplication of that code in this function...

(@austindrenski it's better to wait for me to review too, I know I'm a bit busy these days but still...)

Copy link
Contributor

@austindrenski austindrenski Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roji Apologies on moving forward with the merge before your review. I'll open a follow-up PR to de-duplicate where possible.

Copy link
Contributor Author

@slavanap slavanap Sep 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roji How would you suggest to clone CreateSequenceOperation operation class with all it's properties? Or you suggest modify its ClrType property "in-place" and return back to original value after calling base.Generate() method?

Copy link
Member

@roji roji Sep 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing in-place is a possibility, yes. I'm not even sure it's necessary to return back to the original value as these operations are only used for SQL generation - but for correctness's sake it's a good idea.

You could also copy properties one-by-one to a new instance, including annotations, but that would be tedious and brittle in case something gets added in the future.

{
Check.NotNull(operation, nameof(operation));
Check.NotNull(builder, nameof(builder));

if (VersionAtLeast(10, 0))
{
// "CREATE SEQUENCE name AS type" expression is supported only in PostgreSQL 10 or above.
// The base MigrationsSqlGenerator.Generate method generates that expression.
// https://github.com/aspnet/EntityFrameworkCore/blob/master/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs#L533-L535
base.Generate(operation, model, builder);
return;
}

// Generate CREATE SEQUENCE without "AS" part.
builder
.Append("CREATE SEQUENCE ")
.Append(_sqlGenerationHelper.DelimitIdentifier(operation.Name, operation.Schema))
.Append(" START WITH ")
.Append(_typeMappingSource.GetMapping(typeof(long)).GenerateSqlLiteral(operation.StartValue));

SequenceOptions(operation, model, builder);
builder.AppendLine(_sqlGenerationHelper.StatementTerminator);
EndStatement(builder);
}

#endregion Standard migrations

#region Utilities
Expand Down
33 changes: 31 additions & 2 deletions test/EFCore.PG.FunctionalTests/NpgsqlMigrationSqlGeneratorTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ public virtual void CreateDatabaseOperation_with_tablespace()
Sql);
}

[Fact]
public override void CreateSequenceOperation_with_minValue_and_maxValue()
{
base.CreateSequenceOperation_with_minValue_and_maxValue();
Expand All @@ -218,12 +219,24 @@ public override void CreateSequenceOperation_with_minValue_and_maxValue()
Sql);
}

[Fact]
public override void CreateSequenceOperation_with_minValue_and_maxValue_not_long()
{
// In PostgreSQL, sequence data types are always bigint.
// http://www.postgresql.org/docs/9.4/static/infoschema-sequences.html
base.CreateSequenceOperation_with_minValue_and_maxValue_not_long();
Assert.Equal(
"CREATE SEQUENCE dbo.\"EntityFrameworkHiLoSequence\" AS integer START WITH 3 INCREMENT BY 1 MINVALUE 2 MAXVALUE 816 CYCLE;" + EOL,
Sql);

using (TestHelpers.WithPostgresVersion(new Version(9, 5)))
{
base.CreateSequenceOperation_with_minValue_and_maxValue_not_long();
Assert.Equal(
"CREATE SEQUENCE dbo.\"EntityFrameworkHiLoSequence\" START WITH 3 INCREMENT BY 1 MINVALUE 2 MAXVALUE 816 CYCLE;" + EOL,
Sql);
}
}

[Fact]
public override void CreateSequenceOperation_without_minValue_and_maxValue()
{
base.CreateSequenceOperation_without_minValue_and_maxValue();
Expand Down Expand Up @@ -1050,6 +1063,20 @@ public void CreateSequenceOperation_with_data_type_smallint()
Assert.StartsWith(
"CREATE SEQUENCE public.short_sequence AS smallint",
Sql);

using(TestHelpers.WithPostgresVersion(new Version(9, 5)))
{
Generate(
new CreateSequenceOperation {
Name = "short_sequence",
Schema = "public",
ClrType = typeof(short)
});

Assert.StartsWith(
"CREATE SEQUENCE public.short_sequence",
Sql);
}
}

#endregion Sequence data types
Expand Down Expand Up @@ -1154,6 +1181,8 @@ public void FixedLength()
");" + EOL, Sql);
}

protected new NpgsqlTestHelpers TestHelpers => (NpgsqlTestHelpers)base.TestHelpers;

public NpgsqlMigrationSqlGeneratorTest()
: base(NpgsqlTestHelpers.Instance)
{
Expand Down
30 changes: 27 additions & 3 deletions test/EFCore.PG.FunctionalTests/TestUtilities/NpgsqlTestHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,45 @@
using Microsoft.EntityFrameworkCore;
using System;
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.TestUtilities;
using Microsoft.Extensions.DependencyInjection;
using Xunit;

namespace Npgsql.EntityFrameworkCore.PostgreSQL.TestUtilities
{
public class NpgsqlTestHelpers : TestHelpers
{
protected NpgsqlTestHelpers()
Version _postgresVersion;

public class VersionScope : IDisposable
{
NpgsqlTestHelpers _helpers;
Version _oldVersion, _newVersion;

internal VersionScope(NpgsqlTestHelpers helpers, Version version)
{
_helpers = helpers;
_oldVersion = helpers._postgresVersion;
_newVersion = helpers._postgresVersion = version;
}

public void Dispose()
{
Assert.Equal(_helpers._postgresVersion, _newVersion);
_helpers._postgresVersion = _oldVersion;
}
}

public VersionScope WithPostgresVersion(Version version) => new VersionScope(this, version);

protected NpgsqlTestHelpers() {}

public static NpgsqlTestHelpers Instance { get; } = new NpgsqlTestHelpers();

public override IServiceCollection AddProviderServices(IServiceCollection services)
=> services.AddEntityFrameworkNpgsql();

protected override void UseProviderOptions(DbContextOptionsBuilder optionsBuilder)
=> optionsBuilder.UseNpgsql(new NpgsqlConnection("Database=DummyDatabase"));
=> optionsBuilder.UseNpgsql(new NpgsqlConnection("Database=DummyDatabase"),
options => options.SetPostgresVersion(_postgresVersion));
}
}