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

Ensure that the order of migrations is reliable #12966

Closed
beppemarazzi opened this issue Aug 10, 2018 · 4 comments
Closed

Ensure that the order of migrations is reliable #12966

beppemarazzi opened this issue Aug 10, 2018 · 4 comments
Assignees
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported punted-for-2.2 type-bug
Milestone

Comments

@beppemarazzi
Copy link
Contributor

beppemarazzi commented Aug 10, 2018

AFAIK the order on wich migrations are executed is important and it's derived from lexicographical order of migration id. Since each migration id has a fixed length prefix with the timestamp, then the migrations are supposed to execute in that order.

But digging into the code, i've the suspect that there is no real guaranty about this order: in fact it appears to me that the migrations order relies on the order of insertion in a Dictionary.

https://github.com/aspnet/EntityFrameworkCore/blob/4f65388bf68312e16fe07e62f79d8fad4e242f2a/src/EFCore.Relational/Migrations/Internal/MigrationsAssembly.cs#L49-L73

but FCL documentation states:

For purposes of enumeration, each item in the dictionary is treated as a KeyValuePair<TKey,TValue> structure representing a value and its key. The order in which the items are returned is undefined.

Isn't better to replace
var result = new Dictionary<string, TypeInfo>();
with
var result = new SortedList<string, TypeInfo>();

IMHO the same issue arises where MigrationsAssembly.Migrations is used:

https://github.com/aspnet/EntityFrameworkCore/blob/4f65388bf68312e16fe07e62f79d8fad4e242f2a/src/EFCore.Relational/Migrations/Internal/Migrator.cs#L180-L234

here you can add an explicit .OrderBy(m => m.Key) also when calculating migrationsToApply

Thanks

@ajcvickers
Copy link
Member

@bricelam to investigate and consider for patch.

@inlineHamed
Copy link

inlineHamed commented Aug 13, 2018

Another thing is the system culture settings, If someone in a team use a calendar other than Georgian, migration orders get mixed. I have created an issue about it here (#12978)

@ajcvickers ajcvickers removed this from the 2.1.5 milestone Sep 11, 2018
@ajcvickers
Copy link
Member

Triage; putting this into 2.2 since we should be more robust here.

@ajcvickers ajcvickers added this to the 2.2.0 milestone Sep 12, 2018
@ajcvickers ajcvickers modified the milestones: 2.2.0-preview3, 2.2.0 Oct 15, 2018
@ajcvickers ajcvickers modified the milestones: 2.2.0, 3.0.0 Oct 26, 2018
@bricelam bricelam added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Nov 26, 2018
@bricelam
Copy link
Contributor

Conclusion: The order is reliable on currently supported runtime implementations, it's just documented as undefined. Updating with the proposed changes to be more robust/explicit.

bricelam pushed a commit that referenced this issue Nov 26, 2018
No longer relies on an undefined behavior about the enumeration order in a Dictionary.

Fix #12966
@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview2 Feb 6, 2019
@ajcvickers ajcvickers changed the title Is the order of migrations reliable? Ensure that the order of migrations is reliable Mar 1, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0-preview2, 3.0.0 Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported punted-for-2.2 type-bug
Projects
None yet
Development

No branches or pull requests

4 participants