-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Add support for AttachDBFilename #6446
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.
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
Conversation
Hi @axelheer, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
cc @bricelam |
|
||
builder | ||
.Append(" ON (NAME = '") | ||
.Append(SqlGenerationHelper.EscapeLiteral(operation.Name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the filename and database name can be different. Not sure if it matters here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll rewrite this just to make sure.
Any idea why the newly added tests fail on SQL 2008 and half of them on SQL 2014 (CI build)? I've only Local DB 2016; works fine. |
The CI build server instances probably don't have access to |
Any suggestions? |
Okay, the older SQL Server instance seems to throw a different error, if the database does not exist.
Just adding a check for error number 1832 to the The error message has the corresponding sql error number 5120. I'll try another commit when I'm back home. |
This one. |
Eureka! AppVeyor went green, SQL 2008 seems to like this stuff now too. 🎉 @bricelam please let me know, if there's still something I should change. |
#else | ||
|
||
// TODO: find proper temp directory on .NET Core. Using the current user's temp directory doesn't work, | ||
// since the SQL Server Instance may not have access to it (running under different user credentials)... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be addressed before merging. I think we should use AppContext.BaseDirectory
and only run these tests on LocalDB. Extending the SqlServerCondition enum would be the way to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll use AppDomain.BaseDirectory
for classic .NET then too (consistency).
This time AppVeyor cannot connect to GitHub.com. The CI god ist not with me for this PR. 😔 |
@bricelam should have addressed all the feedback and Travis is green now too. Anything else I can do? |
Everything looks good! I just need to find some time to bash on it a bit with various versions of SQL Server before merging. |
@@ -8,5 +8,7 @@ namespace Microsoft.EntityFrameworkCore.Migrations.Operations | |||
public class SqlServerCreateDatabaseOperation : MigrationOperation | |||
{ | |||
public virtual string Name { get; [param: NotNull] set; } | |||
|
|||
public virtual string FileName { get; [param: NotNull] set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CanBeNull
Would you mind rebase-and-squashing on top of dev? |
There isn't any handling for custom filenames within sql connection strings ('AttachDBFilename'), which leads to obscure error messages. Thus, we just add proper handling of filenames. * AttachDBFilename gets a reset while building master connection strings * Exists accept error 1832 and 5120 too as a hint for non existing database * SqlServerCreateDatabaseOperation gets an additional FileName property
f2b9425
to
6b2de53
Compare
Done. |
Merged. Thanks for the contribution! I made a few tweaks to the tests to help me in my testing (see 656b175) |
using VS2017 CE, C#, SQLServer 2016 LocalDb, .NET 4.6.1, EFCore 1.1.1 developing a WPF desktop app there still seems to be a problem with the connection string. Connection in App.Config is: |
@AzureGulf EF doesn't know anything about DataDirectory--it just gets passed to DbConnection, which then handles it. However, it is my understanding that DataDirectory is not guaranteed to be set in some environments. So it would seem prudent to not use it in connection strings. |
@ajcvickers Thanks for your response and appreciate that what you say is probably correct. However, in my ignorance of DbConnection, the current mechanism is inconsistent in that it works whilst in the VS debugging version but not in the published version. Either it should work in both environments or fail in both, with a suitable error message instead of giving a run-time error in the published version. |
Wether You can find the code here. Maybe publishing "selects" the .NET Standard build? (I've never used ClickOnce before...) |
Hey guys, We just ran into this ourselves; looks like relative paths aren't supported with
Just wanted to express that here for other people who might be bumping into this. The "fix" is use absolute paths. |
Thanks @axelheer - we did try actually. But we just got errors when we tried... Does |
@johnnyreilly none that I know of, but you can just copy the mentioned snippet to your code and test how it works... 🤔 |
Thanks @axelheer - I'll try again and report back.... |
You can influence |
Here is some documentation for it. |
Thanks so much! |
Hey guys, Is there any way to solve the problem of adding multiple AttachDbFilename, very Thanks! |
Also change the value for the Database keyword the second time |
Thanks @ErikEJ -But the second time I couldn't create a new database, this project had only one database: 'Sundial'。DbContext.Database.EnsureCreated() is called CreateCreateOperations() every time,but no AlterDatabaseOperation operation is performed |
@zhosn It's not at all clear to me what you are trying to do, but if you feel there is sill a bug here, then please file a new issue and include a small, runnable project/solution or complete code listing that demonstrates the behavior you are seeing. |
@ajcvickers Thank you. Attached is the code list. The environment: VS2017 , C#, SQLServer 2017 LocalDb, .NET 47.2, EFCore 2.2.6 |
@zhosn Thanks for the code, but I still don't understand what it is you are attempting to do? |
@ajcvickers What I'm doing the project need to put each table in a single Attach Db File, not like a normal database design to unify all tables in a Attach Db File, so I will need to create multiple Attach Db Files in the database, but I don't know how to create multiple data files through the entity framework core, and in the OnModelCreating() I didn't find to create multiple additional data files, the file group and create the tables in the file group of grammar. |
@zhosn As was stated above, there is a one-to-one relationship between database files and database names. In other words, the database file is the database. For a given database, you can't have one table in one file, and a different table in a different file--SQL Server doesn't work like that. |
@ajcvickers Thank you very much for your reply. At present, EF Core may not be able to solve the technical problems in my project. I started trying to use EF core for sqlite and EF core for sqlce, but both failed to solve the technical barrier of accessing multiple databases from the same Dbcontext. Now I need to find another way. |
There isn't any handling for custom filenames within sql connection strings ('AttachDBFilename'), which leads to obscure error messages. Thus, we just add proper handling of filenames.
NOTE: I don't get the difference between "CreationTests" and "CreatorTests", so I just added my tests to the former ones (the other ones seem to test less related stuff).
NOTE: The changes broke a unit test, which I just changed accordingly. I'm not sure, if that's ok.Fixes #2810