Skip to content
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

'ef migrations remove' uses designer files to walkback migrations #8880

Closed
jaredcnance opened this issue Jun 16, 2017 · 10 comments
Closed

'ef migrations remove' uses designer files to walkback migrations #8880

jaredcnance opened this issue Jun 16, 2017 · 10 comments

Comments

@jaredcnance
Copy link

Brief Description

dotnet ef migrations remove uses the migration *.Designer.cs file to rollback state of the DbContextModelSnapshot.cs file.

This is likely to cause problems when merging changes across multiple branches.


Steps to reproduce

To reproduce the issue, clone this repository and run: dotnet ef migrations remove

Please see the readme for full documentation

If this is the intended result, I would be very interested to hear how others are managing changes across multiple branches.


Further technical details

EF Core version: 1.1.2
Database Provider: Npgsql.EntityFrameworkCore.PostgreSQL
Operating system: Mac
IDE: VS Code

@jaredcnance jaredcnance changed the title ef migrations remove uses designer files to walkback migrations 'ef migrations remove' uses designer files to walkback migrations Jun 16, 2017
@ajcvickers ajcvickers added this to the Backlog milestone Jun 19, 2017
@ajcvickers ajcvickers modified the milestones: 2.0.0, Backlog Jun 19, 2017
@ajcvickers
Copy link
Contributor

@bricelam to look into workarounds for 2.0. More complete fix will need to go on backlog.

@bricelam
Copy link
Contributor

This is another manifestation of the root cause of issue #996. Having #1911 would help us detect this situation and handle it better.

Workaround

In this situation, you should be able to use the following workflow.

  1. Delete 20170616182356_modelC.cs and 20170616182356_modelC.Designer.cs
  2. Run Remove-Migration (or dotnet ef migrations remove)
    1. A manual deletion is detected
    2. The snapshot from modelB is used
  3. Run Add-Migration to re-add the changes in the current branch

@bricelam bricelam modified the milestones: Backlog, 2.0.0 Jun 20, 2017
@jaredcnance
Copy link
Author

@bricelam thanks for the suggestion. I can see that it should work fine if the issues are caught quickly. Unfortunately, this doesn't help much, since it still requires developers to:

  • be aware of the issue
  • check that there weren't any new migrations (even though there may not be any conflicts) before merging their PRs
  • regenerate and re-validate their existing migration

Not to mention the mess this can cause downstream if the issues are not caught right away. Trying to sort through designer files to identify the source of the issues can be quite time consuming.

Can you confirm that the designer file is not used when generating migrations? And that it will only become a problem when trying to roll them back?

@bricelam
Copy link
Contributor

bricelam commented Jun 20, 2017

Can you confirm that the designer file is not used when generating migrations? And that it will only become a problem when trying to roll them back?

Correct. The model snapshot is used to generate new migrations, and it will get merged properly like any other version controlled file.

The issue here is we need to synthesize the previous model snapshot which gets messy if you've merge new migrations into the mix.

As part of using Migrations, I think developers need to be very aware of when they merge new migrations into a branch. Being able to detect when a merge occurred (issue #1911) would go a long way to prevent errors that may otherwise go undetected. Heck, with that a dotnet ef migrations rebase command could even be made to simplify the process.

@jaredcnance
Copy link
Author

jaredcnance commented Jun 20, 2017

That makes sense, but I think the solution is simpler than that. My expectation was that dotnet ef migrations remove would have used the last defined migration's (based on timestamp) Down method to rollback the model snapshot. I don't see the need to maintain incremental snapshots.

@jaredcnance
Copy link
Author

jaredcnance commented Sep 15, 2017

It's been a little while on this and I know you all are very busy but I was wondering if there was any update on the design. I would love to contribute if there is room. But moreover I think it would be beneficial if the design discussion was open to the community for input. If this is already the case, can you point me to where this is? I believe the decisions on this issue have greater impact than just fixing the described problem. I believe the problem at hand is a symptom of larger design issues. It will have a significant effect on the developer migration experience

@ajcvickers
Copy link
Contributor

@jaredcnance We haven't been working directly on this at the moment. However, we'll have a discussion (probably not for a couple of weeks) and see if we can put some ideas together. Any ideas you have are most welcome.

@ajcvickers ajcvickers removed this from the Backlog milestone Sep 15, 2017
@jaredcnance
Copy link
Author

jaredcnance commented Sep 21, 2017

@ajcvickers thanks for the update. My main concerns revolve around the use/need for incremental snapshots. There should be enough information in the current snapshot and migration definitions to handle forward and backward migrations. The ideal case would be to have a migration API that defined bi-directional operations so a developer only has to define the up migration. But, in lieu of that, the down method can be used to rollback a migration. Migrations should ALWAYS be treated as serial operations and as such shouldn't need any special merge tooling or incremental snapshots. Some of the best prior art, from a developer experience (IMHO), that does this includes Active Record and Ecto.

While I'm not familiar with EntityFrameworkCore's internal architecture, I'd love to do more research on how this can be achieved if there is agreement from the EF team.

Again, thanks for all the hard work you guys are doing!

@ajcvickers ajcvickers added this to the 2.1.0 milestone Sep 25, 2017
@ajcvickers
Copy link
Contributor

Moved this temporarily into 2.1 milestone so that @bricelam can write down some thoughts.

@bricelam
Copy link
Contributor

bricelam commented Jan 5, 2018

I think we discussed this a bit further elsewhere (maybe on Twitter). The conclusions we came to were:

  • The snapshot is only used when automatically scaffolding a new migration. There should be nothing preventing teams from manually creating the migrations without a snapshot.
  • It would be nice to detect certain situations using Migrations: Track Parent Migrations #1911 so the tools can give more guidance on how to resolve them.
  • We could compute snapshots by applying the Up and Down operations to the model, but this feels very unnecessary and adds additional complexity to the pipeline, so it will likely never happen.

Closing as a duplicate of #1911 since we will consider this and other scenarios as part of that work.

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

No branches or pull requests

3 participants