dradovic / MigSharp

Mig# (MigSharp) is a .NET framework for database migrations and versioning
Other
106 stars 34 forks source link

Assigning IExistingTable to variable causes issue with IDatabase.Execute #60

Closed DanielStout5 closed 8 years ago

DanielStout5 commented 9 years ago

This may be a known issue, but I spent awhile trying to figure out why my migration wasn't working, so if this is desired functionality, I would think there should at least be something in the docs telling you not to do this.

So I wanted to avoid having to keep typing db.Tables["Table"] for each time I needed to access the table, so I just stored it in a variable.. but then executing SQL manually seems to go kind of wonky, saying columns that I haven't dropped yet don't exist. (System.Data.SqlClient.SqlException: Invalid column name 'Identifier'.)

This isn't my actual code of course, but it's a simple version of the problematic section:

var userTable = db.Tables["User"];

db.Execute(ctx => 
{
    var guids = ctx.Connection.Query<Guid>("SELECT Identifier FROM User", transaction: ctx.Transaction);
    // I get an exception before getting to here, even though the column shouldn't be dropped yet. (Invalid column name 'Identifier'.)
    // If I don't drop the column later on, then this works fine and I get all the GUIDs.
});

userTable.UniqueConstraints["UQ_User_Identifier"].Drop();
userTable.Columns["Identifier"].Drop();

If I change it to:

db.Execute(ctx => 
{
    var guids = ctx.Connection.Query<Guid>("SELECT Identifier FROM User", transaction: ctx.Transaction);
});

db.Tables["User"].UniqueConstraints["UQ_User_Identifier"].Drop();
db.Tables["User"].Columns["Identifier"].Drop();

Then everything works fine

dradovic commented 9 years ago

Hm, this is very surprising indeed. From the top of my head assigning the table to variable shouldn't have any effect and I'm doing it myself (although I would have to check if I have subsequent Executes). Are you absolutely sure about this? Are both cases constantly reproducable?

DanielStout5 commented 9 years ago

Yes, I'm quite certain. The only difference between when it works and when it doesn't is that the table reference is stored in a variable.

I do have a number of other Execute statements happening later on in the migration, but I had commented everything else out but the equivalent of what I posted while testing it

Also, just to clarify: Originally I was just using db.Execute(sql) instead of an Action to do what I'm really doing, which is a non-query (Moving the data in the column to be dropped from one table to another). I changed it to the query version to help figure out why it was saying the column name as invalid, but the problem was the same either way.

dradovic commented 9 years ago

Would you have the capacity to add an integration test that shows the problem? Just add a Migration24 to MigSharp.NUnit and execute for example the unit tests in MigSharp.SqlServer.NUnit.

DanielStout5 commented 9 years ago

I will try. I got a bunch of errors about missing DLLs when I tried to build the sln so I will have to figure that out when I get the time.

dradovic commented 9 years ago

I know. Back in the old days I started out without NuGet. These missing dlls are the SMO libs. I should switch to NuGet. It's on my list: #58.

dradovic commented 9 years ago

I've just stumbled over Migration3. It already testing something similar. I'll have a look into this issue as well.

dradovic commented 9 years ago

Okay. Finally I had a look in the code to see what's going on. It is really the case that the indexer db.Tables[<table_name>] (also other indexers to access existing DB objects) is generating a new command object which collects all commands to support the fluent interface. In other words, calling db.Tables[<table_name>] will not give back the same object although admittedly this is what an indexer would suggest.

I guess, I was aware of this when writing the API but I forgot in the meantime and also I've never run into the situation of assigning the returned table to a variable in conjunction with Execute calls.

Thus, whatever you do with that variable will happen before other commands. Mig# has no way of telling the order of method calls in your migration class (that is unless you have a better idea). It just stacks one command after the other and applies them in that order.

dradovic commented 9 years ago

I have added a comment in the manual to document this. I'm not planning to do anything about it. Of course, clever PRs are welcome.

dradovic commented 9 years ago

Will be reconsidered for 3.0.

dradovic commented 9 years ago

This issue is fixed in 3.0.