8684: Merge 1.10.x into dev

[BenedekFarkas]

8684

❗❗❗ Don't squash-merge this PR here, instead, do a manual merge in the repository with a merge commit to provide continuous history from 1.10.x to dev. ❗❗❗

TODO: Review Migrations class changes where version numbers returned in 1.10.x and dev are in conflict:

• src/Orchard.Web/Core/Common/Migrations.cs
  • 1.10.x UpdateFrom6 is dev UpdateFrom8
  • 1.10.x UpdateFrom7 is dev UpdateFrom6
  • 1.10.x UpdateFrom8 is dev UpdateFrom7
• src/Orchard.Web/Modules/Orchard.Projections/Migrations.cs
  • 1.10.x UpdateFrom5 is dev UpdateFrom6
  • 1.10.x doesn't have dev's UpdateFrom5

[BenedekFarkas]

TODO: Review Migrations class changes where version numbers returned in 1.10.x and dev are in conflict:

• src/Orchard.Web/Core/Common/Migrations.cs

  • 1.10.x UpdateFrom6 is dev UpdateFrom8
  • 1.10.x UpdateFrom7 is dev UpdateFrom6
  • 1.10.x UpdateFrom8 is dev UpdateFrom7

• src/Orchard.Web/Modules/Orchard.Projections/Migrations.cs

  • 1.10.x UpdateFrom5 is dev UpdateFrom6
  • 1.10.x doesn't have dev's UpdateFrom5

@sebastienros what do you think about handling this conflict? Basically, if you are currently running your application on 1.10.x code (after 1.10.3), but then upgrade to recent dev (or even 1.11, hopefully it will soon become a reality), then the migration will fail. Do we need to handle that or just keep the migration continuous on the dev branch?

[BenedekFarkas]

TODO: Review Migrations class changes where version numbers returned in 1.10.x and dev are in conflict:

• src/Orchard.Web/Core/Common/Migrations.cs

  • 1.10.x UpdateFrom6 is dev UpdateFrom8
  • 1.10.x UpdateFrom7 is dev UpdateFrom6
  • 1.10.x UpdateFrom8 is dev UpdateFrom7

• src/Orchard.Web/Modules/Orchard.Projections/Migrations.cs

  • 1.10.x UpdateFrom5 is dev UpdateFrom6
  • 1.10.x doesn't have dev's UpdateFrom5

@sebastienros what do you think about handling this conflict? Basically, if you are currently running your application on 1.10.x code (after 1.10.3), but then upgrade to recent dev (or even 1.11, hopefully it will soon become a reality), then the migration will fail. Do we need to handle that or just keep the migration continuous on the dev branch?

@MatteoPiovanelli-Laser @AndreaPiovanelliLaser what do you think? Would this make a difference for you?

[MatteoPiovanelli-Laser]

This is on my todo-list to check

[sebastienros]

A migration shoult not fail, or if this is expected then catch the exception and handle it correctly (I believe it's not the issue here)

[BenedekFarkas]

A migration shoult not fail, or if this is expected then catch the exception and handle it correctly (I believe it's not the issue here)

OK! There would be an exception or at least an inconsistency, because simply merging the code would cause one of the steps to be repeated or skipped. I think the solution here is to fork the logic within the affected steps by detecting what was already executed and run what wasn't. I'll dig in more and let you know if it's ready to review then!

[AndreaPiovanelli]

@BenedekFarkas and @sebastienros What if, inside the migration steps, we add checks to see if e.g. GUIdentifier column in LayoutRecord table (from Orchard.Projections.Migrations.UpdateFrom5) already exists and handle this kind of cases properly? This way we should, in theory, be able to avoid inconsistencies after merging branches and running those migrations.

[BenedekFarkas]

@BenedekFarkas and @sebastienros What if, inside the migration steps, we add checks to see if e.g. GUIdentifier column in LayoutRecord table (from Orchard.Projections.Migrations.UpdateFrom5) already exists and handle this kind of cases properly? This way we should, in theory, be able to avoid inconsistencies after merging branches and running those migrations.

Yeah, that's what I was referring to by forking the logic, so the point here is that no matter which branch you were on, the correct steps should run based on some checks when you update to 1.11. We also need to make sure that minor branch migration changes are merged into dev ASAP, but we also need to cherry-pick migration steps in the other way around to avoid conflicts. But simply getting rid of the minor version branch after 1.11 might be easier.

[BenedekFarkas]

@sebastienros @MatteoPiovanelli-Laser @AndreaPiovanelliLaser

Please see the changes at the bottom of https://github.com/OrchardCMS/Orchard/blob/issue/8684/src/Orchard.Web/Core/Common/Migrations.cs (last 3 update steps) and https://github.com/OrchardCMS/Orchard/blob/issue/8684/src/Orchard.Web/Modules/Orchard.Projections/Migrations.cs (last 2 update steps).

I intentionally overengineered some parts of the code so it's easier to understand what's going on but the point here is that now it doesn't matter which version of 1.10.x you're upgrading from (to dev). All the necessary migration steps will be executed and the ones that have already been executed are detected and skipped.

The logic to detect that an index or column already exists was inspired by https://github.com/OrchardCMS/Orchard/blob/4043df7a0d158dfc70cea65480b61fbd8191bbb3/src/Orchard.Web/Modules/Upgrade/Services/UpgradeService.cs#L119

IMO all these helper functions (TableExists, IndexExists, ColumnExists) should be available in a central location - best would be if they were immediately available in any migration class. That can be achieved by modifying DataMigrationImpl and also the code that initializes the migration classes (so that ISessionFactoryHolder and ShellSettings are available): https://github.com/OrchardCMS/Orchard/blob/4043df7a0d158dfc70cea65480b61fbd8191bbb3/src/Orchard/Data/Migration/DataMigrationManager.cs#L195

What do you think?

[BenedekFarkas]

@MatteoPiovanelli-Laser @AndreaPiovanelli can you please check if you can upgrade to the latest commit on this branch? This is to test the resolved merge conflicts mentioned in the PR description. I'm done with my local testing that included upgrading from 1.10.x and and a client project running on 1.10.2.

[BenedekFarkas]

There's an issue with calling connection.GetSchema in some cases: If there are other migrations steps executing before the ones affected by the merge conflict, then the GetSchema call will time out due the isolation level being ReadCommitted. That's because the Columns/Indexes schema information is dirty, so it's waiting for the transaction to be committed, but the code is waiting for this call to execute and thus cannot proceed towards completing the transaction.

Requiring a new transaction with ReadUncommitted isolation level works in that case, but messing with transactions in migrations is a no-go, as it throws DataMigrationManager off in terms of tracking what steps were executed by not updating the corresponding DataMigrationRecord entry.

I see these possible solutions:

1. A completely new DB connection with isolation level ReadUncommitted is used to read the schema information. Is this OK/possible in a migration while not interfering with the rest of the migration?
2. SchemaBuilder.ExecuteSql: Maybe it's possible to check the existence of those tables/indexes with raw SQL that works with each DB provider? But isolation level would still affect this, I think.
3. We can somehow force DataMigrationManager to run and persist everything up to the migration steps affected by the merge conflict, and then run the affected ones in a new transaction (that doesn't need to be ReadUncommitted).
4. The migration steps affected by the merge conflict are executed separately/manually, similarly to the Upgrade module.
5. We use a property in both affected migrations, something like IsUpgradingFromOrchard_1_10_x_After_1_10_3, because you're only affected if you're upgrading to dev/1.11 from 1.10.x code that is newer than 1.10.3 (or if you applied some 1.10.x patches to your earlier code). Default is false (meaning that you're upgrading from dev or 1.10.x code earlier than 1.10.3) and can be overridden to true in HostComponents.config. The value of this property defines which changes should be executed to make sure that the right changes are attempted. There's probably not many applications running on affected code, so with proper instructions added to the release notes, this should be fine.

@sebastienros what do you think? Option 5 seems like the least amount of headache to me.

[BenedekFarkas]

I've implemented option 5 and tested the various upgrade scenarios with two different projects other than "synthetic" tests with Orchard itself. Looks like the most reliable solution to me and it's easy to set up the configuration to avoid the migration conflict, if necessary.