Support for EFMigration and Database creation #91

Merged
merged 10 commits into from Feb 20, 2014

Projects

None yet

9 participants

@DavidKarlas
Contributor

After following this guide for EntityFramework http://fxjr.blogspot.com/2013/06/npgsql-code-first-entity-framework-431.html I realized there is no support for automatic creation of database and tables... So i created support for DatabaseExist, DatabaseDelete and DatabaseCreate in NpgsqlServices.cs. Also added NpgsqlMigrationSqlGenerator in constructor so it's loaded by dependency injector...

Wrote NpgsqlMigrationSqlGenerator which does not handle Procedures and switching type from int to serial(doesn't add default value to new sequence...) plus I'm not sure how well Indexes work...

I will soon start using this migration on project I'm starting so I'm making this pull request to make people aware of it if someone wants to play with it. You could also merge it it effects only #if ENTITIES6...

My point is... its not complete yet...

@DavidKarlas
Contributor

I'm working with classic BloggingContex example with classes Blog and Post... This is how my app.config looks like:

<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <configSections>

    <section name="entityFramework" type="System.Data.Entity.Internal.ConfigFile.EntityFrameworkSection, EntityFramework, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089" requirePermission="false" />
  <!-- For more information on Entity Framework configuration, visit http://go.microsoft.com/fwlink/?LinkID=237468 --></configSections>
  <startup>
    <supportedRuntime version="v4.0" sku=".NETFramework,Version=v4.5" />
  </startup>
  <entityFramework>
    <providers>
      <provider invariantName="Npgsql" type="Npgsql.NpgsqlServices, Npgsql.EntityFramework"></provider>
    </providers>
    <defaultConnectionFactory type="Npgsql.NpgsqlFactory, Npgsql" />
  </entityFramework>
    <connectionStrings>
        <add name="BloggingContext" providerName="Npgsql" connectionString="server=localhost;port=5432;userid=npgsql_tests;password=npgsql_tests;database=ef6_code_first_sample" />
    </connectionStrings>
    <system.data>
        <DbProviderFactories>
            <remove invariant="Npgsql" />
            <add name="Npgsql Data Provider" invariant="Npgsql" support="FF" description=".Net Framework Data Provider for Postgresql" type="Npgsql.NpgsqlFactory, Npgsql" />
        </DbProviderFactories>
    </system.data>
</configuration>
@franciscojunior
Member

Hi, David!

Thank you for your pull request! This will be a great addition to Npgsql EF support.

I'll check it and if I have any problems I'll contact you. Then I'll merge it in the current master branch!

@MMcM MMcM and 3 others commented on an outdated diff Nov 23, 2013
src/Npgsql/NpgsqlMigrationSqlGenerator.cs
+ }
+
+ #endregion
+
+ #endregion
+
+ #region Misc functions
+
+ private void AppendColumn(ColumnModel column, StringBuilder sql)
+ {
+ sql.Append('"');
+ sql.Append(column.Name);
+ sql.Append("\" ");
+ AppendColumnType(column, sql, true);
+
+ if (column.IsNullable ?? false)
@MMcM
MMcM Nov 23, 2013

I think this test is backwards.

@DavidKarlas
DavidKarlas Nov 24, 2013 Contributor

Why do you think that if IsNullable is not set should mean that column has to be NOT NULL? I think that if is not set it should be nullable...

@jkells
jkells Nov 24, 2013

I think "NOT NULL" should only be added if IsNullable has a value and it's value is false.

You're adding NOT NULL when IsNullable is true.

@jkells
jkells Nov 24, 2013

if(column.IsNullable != null && !column.IsNullable) {...

@DavidKarlas
DavidKarlas Nov 24, 2013 Contributor

Thank you. Feeling much stupid atm :)

@aneves
aneves Nov 26, 2013

You could also simply do if(column.IsNullable == false).

@franciscojunior
Member

Excellent!

I tested it here and it worked perfectly!

Do you think it would be possible to add some unit tests to the NpgsqlMigrationSqlGenerator class? Maybe adding some handcrafted migrationOperations ? I mean, we could create some fake migrationOperations and compare the expected command creation with the actual commands created? This way we wouldn't need to create a full blown EF project but we still could test the command generation correctness?

What do you think?

@DavidKarlas
Contributor

I might do UnitTests this weekend if i get some time ;)
Would you prefer to test generated strings or to connect to database and compare with

select column_name, data_type, is_nullable... from information_schema.columns where table_name = 'table';

One is faster other is more effective in my opinion...

@franciscojunior
Member

We should go with the second option. I agree with you that it is more effective.

@franciscojunior
Member

Another detail I noticed is that those changes fail to build in the Npgsql project for visual studio 2013 when using the net35 configuration. This is the reported error:

[Csc] Npgsql\NpgsqlServices.cs(113, 33): error CS0115: 'Npgsql.NpgsqlServices.DbDatabaseExists(System.Data.Common.DbConnection, int?, System.Data.Metadata.Edm.StoreItemCollection)': no suitable method found to override
[16:35:52][Csc] Npgsql\NpgsqlServices.cs(126, 33): error CS0115: 'Npgsql.NpgsqlServices.DbCreateDatabase(System.Data.Common.DbConnection, int?, System.Data.Metadata.Edm.StoreItemCollection)': no suitable method found to override
[16:35:52][Csc] Npgsql\NpgsqlServices.cs(137, 33): error CS0115: 'Npgsql.NpgsqlServices.DbDeleteDatabase(System.Data.Common.DbConnection, int?, System.Data.Metadata.Edm.StoreItemCollection)': no suitable method found to override

I tested it here and I think you can wrap this code with a #ifdef NET40 letting the UsingPostgresDBConnection out of the #if.

@DavidKarlas DavidKarlas commented on an outdated diff Dec 1, 2013
src/Npgsql/NpgsqlMigrationSqlGenerator.cs
+ }
+ else
+ {
+ if (addedSchemas.Count == 1)
+ {
+ //TODO: CREATE PROCEDURE that checks if schema already exists
+ }
+ //TODO: Execute above procedure
+ AddStatment("CREATE SCHEMA " + schemaName);
+ }
+ }
+
+ private void CreateExtension(string exensionName)
+ {
+ //This is compatible only with server 9.1+
+ if (serverVersion.Major > 9 || (serverVersion.Major == 9 && serverVersion.Minor >= 1))
@DavidKarlas
DavidKarlas Dec 1, 2013 Contributor

If someone will work on this code notice that extension like UUID will not be created on older server versions...

@DavidKarlas DavidKarlas commented on an outdated diff Dec 1, 2013
src/Npgsql/NpgsqlMigrationSqlGenerator.cs
+ {
+ int dotIndex = schemaName.IndexOf('.');
+ if (dotIndex != -1)
+ schemaName = schemaName.Remove(dotIndex);
+ if (addedSchemas.Contains(schemaName))
+ return;
+ addedSchemas.Add(schemaName);
+ if (serverVersion.Major > 9 || (serverVersion.Major == 9 && serverVersion.Minor >= 3))
+ {
+ AddStatment("CREATE SCHEMA IF NOT EXISTS " + schemaName);
+ }
+ else
+ {
+ if (addedSchemas.Count == 1)
+ {
+ //TODO: CREATE PROCEDURE that checks if schema already exists
@DavidKarlas
DavidKarlas Dec 1, 2013 Contributor

This also needs some work to work with older servers properly

@franciscojunior
Member

Hi, David!

We merged pull request #110 which made your pull request not apply cleanly anymore. Would you mind to rebase it on current master?

Thanks in advance.

@DavidKarlas
Contributor

I reset my Master to upstream Master and added NpgsqlMigrationSqlGenerator.cs only in Npgsql.EntityFramework project(since this is only EF6+ working code(MigrationSqlGenerator had some changes between EF4 and EF6)).
I also changed NpgsqlServices to be mergable again plus added #if EF6 around DB creation methods to compile on .Net35 build properly(EF6 does not support .Net 3.5)...

I will now start looking into creating UnitTests...

@franciscojunior
Member

Excellent, David!

Your pull request is one I'd like to include in next 2.1 release before we
do the feature freeze. As we are doing a big push in the ef area with this
release, I think your patch to allow dB creation and migration is also
desirable to be included.

I'm looking forward your tests. I could only test the db creation in my
test app.
Em 07/12/2013 08:43, "David Karlaš" notifications@github.com escreveu:

I reset my Master to upstream Master and added
NpgsqlMigrationSqlGenerator.cs only in Npgsql.EntityFramework project(since
this is only EF6+ working code(MigrationSqlGenerator had some changes
between EF4 and EF6)).
I also changed NpgsqlServices to be mergable again plus added #if EF6
around DB creation methods to compile on .Net35 build properly(EF6 does not
support .Net 3.5)...

I will now start looking into creating UnitTests...


Reply to this email directly or view it on GitHubhttps://github.com/npgsql/Npgsql/pull/91#issuecomment-30052303
.

@DavidKarlas
Contributor

It seems like current NpgsqlTests project does not have EntityFramework included... Will you guys create NpgsqlTests.EntityFramework project which will have references to Npgsql.EntityFramework and EntityFramework nuget package?

@roji
Member
roji commented Dec 7, 2013

@DavidKarlas, there is no reason not to add a project reference from NpgsqlTests to Npgsql.EntityFramework (and even to Npgsql.EntityFrameworkLegacy), no need for extra projects... Go ahead and do so in your pull request,

@franciscojunior
Member

Hi David.

There is this pull request #119 which adds some changes to some EF support files. Do you think they have impact in your database creation support? I also asked Kenji who created #119 to have a look so he can give us some feedback about that.

@kenjiuno
Contributor

Hi David.

I have pull request #119 for updating Npgsql's provider manifest. It will affect EF support.

I have watched your changes. I think your patch won't be affected.

If you can prepare unit tests for your pull request, I'm willing to test it with my patch. :)

@DavidKarlas
Contributor

To be honest I'm not 100% what #119 is doing. But I think there won't be problems.

I was working on how to make unit tests for this migrations during weekend. And I will probably make one or two tests against DB and rest will be string compare based testing because It's hard to create migrations test because have to modify classes and then run AddMigration in Nuget console...(this sounded simple but It's nasty)

@franciscojunior
Member

To be honest I'm not 100% what #119 is doing. But I think there won't be problems.

I also think there won't be problems.

I was working on how to make unit tests for this migrations during weekend. And I will probably make one or two tests against DB and rest will be string compare based testing because It's hard to create migrations test because have to modify classes and then run AddMigration in Nuget console...(this sounded simple but It's nasty)

Ok. I think string comparison will work too.

@roji
Member
roji commented Dec 12, 2013

@DavidKarlas, don't mean to press but have you had any time to work on the unit tests? I'm only asking because we're waiting on this pull request to start the 2.1 release cycle

@DavidKarlas
Contributor

I will do it this weekend ;) Hope it's not too late...

@roji
Member
roji commented Dec 13, 2013

Thanks :) of course it isn't too late...

@roji
Member
roji commented Dec 20, 2013

I think we really should be freezing version 2.1 soon, and are waiting only for this. I'd say if you can't get this done this weekend, we can leave it for the next release, ok?

@DavidKarlas
Contributor

Sorry for delaying this for one week but could we skip this feature for 2.1 release because I done only few tests so far plus I'm not happy about only supporting server 9.3(because CREATE SCHEMA IF NOT EXISTS)... Plus there are probably some bugs... Plus it seems like this not very demending feature atm...

@roji
Member
roji commented Dec 20, 2013

No problem @DavidKarlas, if you're not 100% happy with it, take your time and work on it for the next release...

@DavidKarlas
Contributor

I'm still alive :) I was hyped on NetDuino and some other stuff...
I did all basic UnitTests and found few bugs... Now testing on existing nicely sized context with few migrations all is fine except this MVC application is using AspNet authorization which creates Tables at runtime but there is bug in EF I'm hoping you guys can upvote this on EF issue tracker TY
https://entityframework.codeplex.com/workitem/371

@franciscojunior
Member

I'm still alive :) I was hyped on NetDuino and some other stuff...

Welcome back! :)

I did all basic UnitTests and found few bugs... Now testing on existing nicely sized context with few migrations all is fine except this MVC application is using AspNet authorization which creates Tables at runtime but there is bug in EF I'm hoping you guys can upvote this on EF issue tracker TY
https://entityframework.codeplex.com/workitem/371

Excellent!!

I'll have a look at this EF bug.

Please, let us know if you have any problems merging your code back to Npgsql code base. We made a lot of changes.

Thanks in advance.

@DavidKarlas
Contributor

Just wanted to say that I rebased with master and created UnitTests there are still few details missing like backward(even 9.2 won't work in one case) support for older backends can be added with sql functions and switching field from integer to serial type since I would have to create function which would create sequence and put default to nextval(newSequence) but I'm not 100% sure how to do this... I will probably try to fix this in future... Atm will focus on more testing...

@franciscojunior
Member

That's great, David!

I'll make some tests and will let you know if I have any issues.

@franciscojunior
Member

Hi, David!

I just started playing with your changes and they worked very well!

One initial comment I have is that we would need to be able to get the bloggingcontext connection dynamically like the connection string from the other tests. Currently the tests only work for backends listening in the default port.

Today we have in our tests the ability to run in many backend versions. This is achieved by creating at runtime the connection string to each backend we want to test. Today we test from 8.4 version way up to 9.3. Thanks Shay for this wonderful setup!
Those backend are differentiated by the post they are listening. 8.4 listens in the 5484, 9.3 listens in the 5493 and so on.

From what I saw, we would need to change the BloggingContext constructor to accept a connectionstring which would be passed to the base class.

We also would need to add support to TestFixtureSetup expose the new connection string. Maybe we could call it ConnectionStringEF as we already have the ConnectionString property. This would be used in the two calls to new BloggingContext inside the entity framework tests.

What do you think?

Other than that I think your work is excellent and we will start to have unittests for entityframework code which we don't have until now.

Thank you very much for your work, David!

@DavidKarlas
Contributor

Something like this? :P

@franciscojunior
Member

Yes, something like this! :)

Although I was thinking that it would be better if we didn't use the database "npgsql_tests" used in the other tests. Maybe we could change the connectionstring at runtime to use "npgsql_tests_ef" instead?

I'm saying that because I think that an error in the entity framework tests which drop the database can compromise all the other tests and not just the entity framework tests.

For example, when I tested in a 8.4 version I have here, the test CreateBloggingContext failed because I didn't have the uuid support:

LOG:  statement: CREATE TABLE "dbo"."Posts"("PostId" serial4 NOT NULL,"Title" te
xt,"Content" text,"UniqueId" uuid NOT NULL DEFAULT uuid_generate_v4(),"Rating" i
nt2,"BlogId" int4 NOT NULL DEFAULT 0,CONSTRAINT "PK_dbo.Posts" PRIMARY KEY ("Pos
tId"))
NOTICE:  CREATE TABLE will create implicit sequence "Posts_PostId_seq" for seria
l column "Posts.PostId"
ERROR:  function uuid_generate_v4() does not exist
HINT:  No function matches the given name and argument types. You might need to add explicit type casts.

Maybe we could create a version of the test with an entity which has a uuid property for postgresql 9.1 and above and another test for postgresql 8.4 without an uuid property.
When I removed the uuid property, the test passed ok when running on 8.4.

Another thing I noticed is that I need to be superuser in order to install the uuid module. I stumbled upon that when I tried to run the tests on a 9.3 I have here. Do you think this could be a problem? I mean, I think the majority of Npgsql users who will be using EF won't be superusers and they will get the exception when trying to run it. But I don't know if there is a better way to handle this besides throwing the exception and adding documentation about the need for this extension when using uuid datatype. May we assume that if the user wants to use a property of type uuid she would already have uuid module installed?

What do you think?

@DavidKarlas
Contributor

I'm thinking about CREATE EXTENSION...

@franciscojunior
Member

Very good!

Regarding the EFTestBase.cs I was thinking about something like:

In EntityFrameworkMigrationTests.cs file: 


        private string connectionStringEF;

        /// <summary>
        /// New ConectionString property crafted to change the database name from original TestBase.ConnectionString to append a "_ef" suffix.
        /// i.e.: the TestBase.ConnectionString database is npgsql_tests. Entity Framework database will be npgsql_tests_ef.
        /// </summary>
        protected new string ConnectionString 
        { 
            get 
            { 
                if (connectionStringEF == null)
                {
                    //Reuse all strings just add _ef at end of database name for 
                    var connectionSB = new NpgsqlConnectionStringBuilder(base.ConnectionString);
                    connectionSB.Database += "_ef";
                    connectionStringEF = connectionSB.ConnectionString;
                }
                return connectionStringEF; 
            } 
        }

Instead of creating the EFTestBase.cs file.
I did some testing here and the new property worked like a charm.

What do you think?

@roji
Member
roji commented Feb 12, 2014

@franciscojunior, a small comment on using another database...

If possible it would be best to use a single database for all tests; the connection string selection is already a bit complex, supporting multiple backend versions like you said. If we have to multiply this by two (EF tests vs. non-EF tests) it will be even more complex...

How about simply using other tables in the EF tests? I assume that EF won't magically go deleting all tables in the database, so there's probably no danger to the non-EF table we're using right now?

@franciscojunior
Member

Hi Shay!

Initially, David was using the same database. But there were some tests which needed to drop the database and recreate it. That's why I told him it would be better if we didn't use the same database as an error in the drop database test could make the other tests start to fail.

@roji
Member
roji commented Feb 12, 2014

Oh... We need to run EF tests that actually drop the entire database?? In that case I guess you're right...

@DavidKarlas
Contributor

I created new base class to prevent conflicts with opening database connection by TestBase.cs(forgot that we have separate database now...) so now I kind of expanded to support also EF children with ConnectionStringEF...

About dropping database... Beside migrations I also implemented EF interface for Creating, Dropping and checking if database Exsists so to test this methods...

BTW: Why does user has to create "npgsql_tests" database manually? It could be checked and created by TestFixtureSetUp...(by connecting to postgres database same as in NpgsqlServices.cs)

@franciscojunior
Member

Oh... We need to run EF tests that actually drop the entire database?? In that case I guess you're right...

Yes. In order to test the scripts which create the entities tables and auxiliary tables like migration.
But not all tests need to drop the database.

@franciscojunior
Member

Oh, I just saw that David already answered you, Shay! :)

@roji
Member
roji commented Feb 12, 2014

@DavidKarlas: I think the main reason I had in my mind for requiring manual creation of npgsql_tests was not to require admin privileges for the npgsql test user - the idea was for it to be a normal restricted user which can operate only within the test database. I don't think it's terribly important though.

It all makes sense to me now, if you're testing database creation and dropping then obviously a separate EF database is a good idea, thanks for explaining...

@franciscojunior
Member

Well remembered, Shay! We will need to change the privileges of the npgsql_tests users to include the createdb permission or else the drop and createdb operations from EF tests will fail.

@roji
Member
roji commented Feb 12, 2014

OK. So if the npgsql_tests user has to have database create/drop permissions for the EF tests, we might as well make it create the non-EF npgsql_tests database as well like @DavidKarlas suggested...

@franciscojunior
Member

On Wed, Feb 12, 2014 at 3:38 PM, Shay Rojansky notifications@github.comwrote:

OK. So if the npgsql_tests user has to have database create/drop
permissions for the EF tests, we might as well make it create the non-EF
npgsql_tests database as well like @DavidKarlashttps://github.com/DavidKarlassuggested...

Agreed!

+1


Reply to this email directly or view it on GitHubhttps://github.com/npgsql/Npgsql/pull/91#issuecomment-34894161
.

Regards,

Francisco Figueiredo Jr.
Npgsql Lead Developer
http://www.npgsql.org
http://gplus.to/franciscojunior
http://fxjr.blogspot.com
http://twitter.com/franciscojunior

@franciscojunior
Member

Shay, do you think it would be too much annoying to raise the requirements
to run our tests the use of a user with createdb permissions?

On Wed, Feb 12, 2014 at 4:22 PM, Francisco Figueiredo Jr. <
francisco@npgsql.org> wrote:

On Wed, Feb 12, 2014 at 3:38 PM, Shay Rojansky notifications@github.comwrote:

OK. So if the npgsql_tests user has to have database create/drop
permissions for the EF tests, we might as well make it create the non-EF
npgsql_tests database as well like @DavidKarlashttps://github.com/DavidKarlassuggested...

Agreed!

+1


Reply to this email directly or view it on GitHubhttps://github.com/npgsql/Npgsql/pull/91#issuecomment-34894161
.

Regards,

Francisco Figueiredo Jr.
Npgsql Lead Developer
http://www.npgsql.org
http://gplus.to/franciscojunior
http://fxjr.blogspot.com
http://twitter.com/franciscojunior

Regards,

Francisco Figueiredo Jr.
Npgsql Lead Developer
http://www.npgsql.org
http://gplus.to/franciscojunior
http://fxjr.blogspot.com
http://twitter.com/franciscojunior

@roji
Member
roji commented Feb 12, 2014

Not at all, Francisco, I think it's OK... It's only developers who run the unit tests and it's a reasonable requirement, especially since the tests actually test createdb capabilities... I'd say go for it.

@franciscojunior
Member

Hi, @DavidKarlas ! I'm just configuring our database server to give permission to our tests user to create database so those new tests don't raise errors. As soon as I get that resolved, I'll merge your code. I think it is very good and will be an excellent addition to EF users! Thank you very much!

@DavidKarlas
Contributor

Glad to hear that ;) I will keep watching issues on Github and resolve any problems with migrations ASAP when users find them. I hope you put some -Pre Nuget binary on soon :)

@franciscojunior
Member

Great!
We are working in the 2.1.0 rc1 release and soon it will be a stable release.
Database migration is set to be part o fthe 2.2 release which will start to create betas very soon after 2.1 final is release. But I'm already talking about your pull request and asking users to give it a try: https://groups.google.com/forum/#!topic/npgsql-help/97X1QX5WE5k

I'll also write a blog post about this new feature so more people will start to use and test it! :)

@roji
Member
roji commented Feb 17, 2014

A small note... Right now this PR doesn't compile for me (and the build server) because references point to EF6.0.2, but in the packages directory I can see EF6.0.1.

Can you confirm that you've switched to nuget "package restore" and this is why I'm not seeing EF6.0.2 in git? If so, the old EF6.0.1 needs to be deleted and I will enable package restore on our build server.

@DavidKarlas
Contributor

Yes I switched to EF6.0.2.

  • Should I switch back to EF6.0.1 and let you do transition?
  • Should I delete packages/EntityFramework.6.0.1 in my pull request?
  • Should I delete packages/EntityFramework.6.0.1 and add packages/EntityFramework.6.0.2 in my pull request?
  • Or you was just asking to confirm and you will do what has to be done?
@roji
Member
roji commented Feb 17, 2014

Yeah, unless you have a specific need for EF6.0.2 right now, please switch back to EF6.0.1 for now. After the PR is merged I'll take care of moving to EF6.0.2 with clean nuget package restore etc. Thanks.

@franciscojunior
Member

I also think it is better to use the 6.0.1 version.

@DavidKarlas
Contributor

Is this build server of yours a bit lazy? :P

@franciscojunior
Member

On Mon, Feb 17, 2014 at 6:08 PM, David Karlaš notifications@github.comwrote:

Is this build server of yours a bit lazy? :P

:D

We didn't enable automatic build of pull requests yet. =)

Only merges are automatically being built.


Reply to this email directly or view it on GitHubhttps://github.com/npgsql/Npgsql/pull/91#issuecomment-35323017
.

@franciscojunior
Member

Hi, @DavidKarlas ! I'm still having problems compiling your changes even after you reverted to use EF 6.0.1. I'm checking what is happening and I will merge your changes when I get the builds going. Sorry for delay... :(

@franciscojunior
Member

I think I got it! I tried a custom build where I do a clean checkout before the build and now it is compiling!

It was giving errors when trying to compile the EntityFrameworkTests as it couldn't find the EntityFramework.dll assembly.

@franciscojunior
Member

Everything is almost ready. Compilation problem is now solved. Now I need to fix the permission of npgsql_tests user to be able to create database. I forgot to make this configuration change :)

Right now I'm at my job and the firewall here doesn't allow me connect to the build server to do this change. I'll make it later and then I hope everything will be ready to merge! :)

@DavidKarlas
Contributor

Glad to hear this!

@franciscojunior
Member

Hi, @DavidKarlas !

I just added the permission to create database to our tests user. Create database tests and a lot of others are passing ok.
Unfortunately, I'm still getting the following errors in the tests:

[NpgsqlTests.dll] NpgsqlTests.EntityFrameworkMigrationTests("8.4").TestCreateTableOperation
[20:04:25]
[NpgsqlTests.EntityFrameworkMigrationTests("8.4").TestCreateTableOperation] Test(s) failed.   Expected string length 38 but was 24. Strings differ at index 14.
  Expected: "CREATE SCHEMA IF NOT EXISTS someSchema"
  But was:  "CREATE SCHEMA someSchema"
  -------------------------^
[20:04:25]
[NpgsqlTests.EntityFrameworkMigrationTests("8.4").TestCreateTableOperation] Test(s) failed.   Expected string length 38 but was 24. Strings differ at index 14.
  Expected: "CREATE SCHEMA IF NOT EXISTS someSchema"
  But was:  "CREATE SCHEMA someSchema"
  -------------------------^

   at NUnit.Framework.Assert.That(Object actual, IResolveConstraint expression, String message, Object[] args)
   at NpgsqlTests.EntityFrameworkMigrationTests.TestCreateTableOperation()
[20:04:25][NpgsqlTests.dll] NpgsqlTests.EntityFrameworkMigrationTests("8.4").TestDefaultTypes
[20:04:25][NpgsqlTests.dll] NpgsqlTests.EntityFrameworkMigrationTests("8.4").TestDropColumnOperation
[20:04:25][NpgsqlTests.dll] NpgsqlTests.EntityFrameworkMigrationTests("8.4").TestDropForeignKeyOperation
[20:04:25]
[NpgsqlTests.EntityFrameworkMigrationTests("8.4").TestDropForeignKeyOperation] Test(s) failed.   Expected string length 58 but was 48. Strings differ at index 40.
  Expected: "ALTER TABLE "someTable" DROP CONSTRAINT IF EXISTS "someFK""
  But was:  "ALTER TABLE "someTable" DROP CONSTRAINT "someFK""
  ---------------------------------------------------^
[20:04:25]
[NpgsqlTests.EntityFrameworkMigrationTests("8.4").TestDropForeignKeyOperation] Test(s) failed.   Expected string length 58 but was 48. Strings differ at index 40.
  Expected: "ALTER TABLE "someTable" DROP CONSTRAINT IF EXISTS "someFK""
  But was:  "ALTER TABLE "someTable" DROP CONSTRAINT "someFK""
  ---------------------------------------------------^

   at NUnit.Framework.Assert.That(Object actual, IResolveConstraint expression, String message, Object[] args)
   at NpgsqlTests.EntityFrameworkMigrationTests.TestDropForeignKeyOperation()
[20:04:25][NpgsqlTests.dll] NpgsqlTests.EntityFrameworkMigrationTests("8.4").TestDropIndexOperation
[20:04:25][NpgsqlTests.dll] NpgsqlTests.EntityFrameworkMigrationTests("8.4").TestDropIndexOperationTableNameWithSchema
[20:04:25][NpgsqlTests.dll] NpgsqlTests.EntityFrameworkMigrationTests("8.4").TestDropPrimaryKeyOperation
[20:04:25][NpgsqlTests.dll] NpgsqlTests.EntityFrameworkMigrationTests("8.4").TestDropTableOperation
[20:04:25][NpgsqlTests.dll] NpgsqlTests.EntityFrameworkMigrationTests("8.4").TestHistoryOperation
[20:04:25][NpgsqlTests.dll] NpgsqlTests.EntityFrameworkMigrationTests("8.4").TestMoveTableOperation
[20:04:25]
[NpgsqlTests.EntityFrameworkMigrationTests("8.4").TestMoveTableOperation] Test(s) failed.   Expected string length 41 but was 27. Strings differ at index 14.
  Expected: "CREATE SCHEMA IF NOT EXISTS someNewSchema"
  But was:  "CREATE SCHEMA someNewSchema"
  -------------------------^
[20:04:25]
[NpgsqlTests.EntityFrameworkMigrationTests("8.4").TestMoveTableOperation] Test(s) failed.   Expected string length 41 but was 27. Strings differ at index 14.
  Expected: "CREATE SCHEMA IF NOT EXISTS someNewSchema"
  But was:  "CREATE SCHEMA someNewSchema"
  -------------------------^

   at NUnit.Framework.Assert.That(Object actual, IResolveConstraint expression, String message, Object[] args)
   at NpgsqlTests.EntityFrameworkMigrationTests.TestMoveTableOperation()
[20:04:25][NpgsqlTests.dll] NpgsqlTests.EntityFrameworkMigrationTests("8.4").TestMoveTableOperationNewSchemaIsNull
[20:04:25]
[NpgsqlTests.EntityFrameworkMigrationTests("8.4").TestMoveTableOperationNewSchemaIsNull] Test(s) failed.   Expected string length 31 but was 17. Strings differ at index 14.
  Expected: "CREATE SCHEMA IF NOT EXISTS dbo"
  But was:  "CREATE SCHEMA dbo"
  -------------------------^
[20:04:25]
[NpgsqlTests.EntityFrameworkMigrationTests("8.4").TestMoveTableOperationNewSchemaIsNull] Test(s) failed.   Expected string length 31 but was 17. Strings differ at index 14.
  Expected: "CREATE SCHEMA IF NOT EXISTS dbo"
  But was:  "CREATE SCHEMA dbo"
  -------------------------^

   at NUnit.Framework.Assert.That(Object actual, IResolveConstraint expression, String message, Object[] args)
   at NpgsqlTests.EntityFrameworkMigrationTests.TestMoveTableOperationNewSchemaIsNull()

I think server version checks need to be made inside the tests too just like the creation statements. :)

Those errors appear on all server versions tested (8.4, 9.0, 9.1 and 9.2) but 9.3

What do you think?

Other than that, everything seems to be going very well. :)

@franciscojunior
Member

Hi, @DavidKarlas !
Running tests on our build server right now. So far, tests are passing smoothly!

@franciscojunior
Member

We are almost there!

On .net 4.0 and 4.5 all tests pass ok.

EntityFrameworkBasicTests are failing on .net 3.5 and 2.0:

[NpgsqlTests.dll] NpgsqlTests.EntityFrameworkBasicTests("8.4")
[07:44:49][NpgsqlTests.EntityFrameworkBasicTests("8.4")] SetUp method failed. SetUp : System.InvalidOperationException : The Entity Framework provider type 'Npgsql.NpgsqlServices, Npgsql.EntityFramework' registered in the application config file for the ADO.NET provider with invariant name 'Npgsql' could not be loaded. Make sure that the assembly-qualified name is used and that the assembly is available to the running application. See http://go.microsoft.com/fwlink/?LinkId=260882 for more information.
[07:44:49]
[NpgsqlTests.EntityFrameworkBasicTests("8.4")] SetUp method failed. SetUp : System.InvalidOperationException : The Entity Framework provider type 'Npgsql.NpgsqlServices, Npgsql.EntityFramework' registered in the application config file for the ADO.NET provider with invariant name 'Npgsql' could not be loaded. Make sure that the assembly-qualified name is used and that the assembly is available to the running application. See http://go.microsoft.com/fwlink/?LinkId=260882 for more information.
   at System.Data.Entity.Infrastructure.DependencyResolution.ProviderServicesFactory.GetInstance(String providerTypeName, String providerInvariantName)
   at System.Data.Entity.Internal.AppConfig.<.ctor>b__2(ProviderElement e)
   at System.Linq.Enumerable.WhereSelectEnumerableIterator`2.MoveNext()
   at System.Collections.Generic.List`1..ctor(IEnumerable`1 collection)
   at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
   at System.Data.Entity.Internal.AppConfig.<.ctor>b__1()
   at System.Lazy`1.CreateValue()
   at System.Lazy`1.LazyInitValue()
   at System.Data.Entity.Internal.AppConfig.get_DbProviderServices()
   at System.Data.Entity.Infrastructure.DependencyResolution.AppConfigDependencyResolver.RegisterDbProviderServices()
   at System.Data.Entity.Infrastructure.DependencyResolution.AppConfigDependencyResolver.GetServiceFactory(Type type, String name)
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at System.Data.Entity.Infrastructure.DependencyResolution.AppConfigDependencyResolver.GetService(Type type, Object key)
   at System.Data.Entity.Infrastructure.DependencyResolution.DbDependencyResolverExtensions.GetServiceAsServices(IDbDependencyResolver resolver, Type type, Object key)
   at System.Linq.Enumerable.<SelectManyIterator>d__14`2.MoveNext()
   at System.Linq.Enumerable.<ConcatIterator>d__71`1.MoveNext()
   at System.Linq.Enumerable.<OfTypeIterator>d__aa`1.MoveNext()
   at System.Data.Entity.Utilities.IEnumerableExtensions.Each[T](IEnumerable`1 ts, Action`1 action)
   at System.Data.Entity.Infrastructure.DependencyResolution.DbConfigurationManager.<.ctor>b__1()
   at System.Lazy`1.CreateValue()
   at System.Lazy`1.LazyInitValue()
   at System.Data.Entity.Infrastructure.DependencyResolution.DbConfigurationManager.GetConfiguration()
   at System.Data.Entity.DbContext.InitializeLazyInternalContext(IInternalConnection internalConnection, DbCompiledModel model)
   at NpgsqlTests.EntityFrameworkBasicTests.TestFixtureSetup()

I think BasicTests are missing the #if NET40 like you did with MigrationTests.

@franciscojunior
Member

Excellent work, David! All tests pass now! Merging now...

@franciscojunior franciscojunior merged commit 0cfe3b4 into npgsql:master Feb 20, 2014

1 check passed

default Finished TeamCity Build Npgsql :: All : Running
Details
@jgilm
jgilm commented Apr 8, 2014

I hope I am not asking in the wrong place. I have built the master branch so that I could begin to use EF Migration support (excellent work BTW). Two things that I have noticed, but I'm not sure if I am doing something wrong.

First, all of my string entity properties are being rendered (when doing add-migration) as

c.String(maxLength: 1073741823, fixedLength: true)

which causes PostgreSQL/npgsql to throw an exception. I would like them to be either:

c.String(fixedLength: false)

or

c.String(storeType: "text")

Is there any way to default it to one of the above methods when doing an add-migration?

Second, it appears when one table depends upon another, in my example I have a "country" table and a "user" table that has a foreign key to "country.id". Again, when doing add-migration the migration works very well, but the tables being depended upon are listed after the tables that depend. In my example, the user table CreateTable is being called before the country CreateTable. It is easy to rearrange the order, but I was wondering if this was expected or if it is something that can be fixed?

@DavidKarlas
Contributor

End goal here is that you don't care if you are targeting MS SQL or Postgresql so EVERYTHING must be fixed :) (Except stored procedures)

I would prefer if you could open two separate issues for this for better tracking and stuff...

It would also be awesome if you could copy-paste output of "Update-Database –Verbose" command where actual Postgresql commands are displayed ;)

Btw tnx for giving it a try ;)
I'm a bit busy atm so I will take deeper look into this when you open new issues if posible with that output... tnx

@Emill

SQL injection possible?

Contributor

http://msdn.microsoft.com/en-us/library/system.data.entity.migrations.model.migrationoperation(v=vs.113).aspx aka. Entity Framework Migrations APIs are not designed to accept input provided by untrusted sources (such as the end user of an application). If input is accepted from such sources it should be validated before being passed to these APIs to protect against SQL injection attacks etc.

Point is that it would be very unusual for database creation/migrations to be generated from untrusted sources...

Member
Emill replied Aug 7, 2014

Well, yes it's true that it would be very unusual to generate databases from untrusted sources.
But let me change the question: what if the user would like the default value of the string to contain a '?
It's not that it's that hard to simply escape value ;)

Contributor

I don't want to leave question open and floating... So I will be straight whit you... :)
EF totally disappointed me when I ran into this: https://entityframework.codeplex.com/workitem/47 and became totally useless to me(aka. dead)
So I'm not in mood to think to much about this... If you think it should be escaped... Escape it :)

Sorry...

Member
Emill replied Aug 7, 2014

Haha ok. You know you can use something like this as workaround:?

context.Things.Select(t => new { Thing = t, Stuff = t.Stuff.Where(s => ...) })
@Emill
Member
Emill commented on 66081cc Oct 21, 2014

Shouldn't the schema name also be double-quoted?

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