Migrations: Optimise sortable value population for date properties

[AndyButland]

Summary

I found some problems in testing PopulateSortableValueForDatePropertyData (v17.3.0) was timing out on remote SQL Server databases with large umbracoPropertyData tables. Three issues addressed:

• Moved the sub-query to find the relevant property type Ids from the aliases into a separate query.
  • This looks to be the major issue - with a sub-query I was finding the SQL Server optimiser would pick a plan that would apply this filter last, meaning we check all data for values before filtering to only the expected property IDs. We want that to happen first to reduce the result set for the further, more expensive, filtering in the query.
• Broke the UPDATE statement up into batches of 10000, with logging on each batch updated.
• Added this usual long command timeout we set on potentially long-running migration steps.

Testing

I've carried out all the following locally.

• Existing integration test PopulateSortableValueForDatePropertyDataTest passes on both SQLite and SQL Server
• Verify on a database with no date/time property types that the migration logs the skip message and exits quickly
• Verify progress logging appears at information level per batch during a real run

[claude]

Claude finished @AndyButland's task in 3m 12s —— View job

---

PR Review

Target: origin/main · Based on commit: 2e9700c4649223b0ad605daf1dde879ea638a22d

Replaces a single-pass bulk UPDATE with batched UPDATEs against a pre-resolved static property-type-ID list, adding loop-termination guards and per-batch progress logging to fix timeout failures on large SQL Server databases.

• Modified public API: ExecuteMigration(IUmbracoDatabase, DatabaseType, ILogger) — signature preserved, no breaking change.

---

Important

• PopulateSortableValueForDatePropertyData.cs:65-70: The new early-exit path (when no date/time property types are found) has no test coverage. Same for the malformed-date guard (TRY_CAST/datetime() IS NOT NULL). These are new behavioural branches. The existing PopulateSortableValueForDatePropertyDataTest only exercises the happy path and the already-populated idempotency path. Two integration test cases would give confidence: one that seeds no date property types and asserts 0 rows affected + skip log, and one that seeds a row with malformed JSON in textValue and asserts it is permanently skipped (no infinite-loop behaviour). Fix this →

Suggestions

• PopulateSortableValueForDatePropertyData.cs:85-93: GetSortablePropertyTypeIds constructs SQL via string.Join over _dateTimePropertyEditorAliases (hardcoded compile-time constants — no injection risk). The pattern is safe here, but it deviates from the NPoco Sql<ISqlContext> builder convention documented in CLAUDE.md for Infrastructure. A future contributor copying this helper with non-constant input would have an injection vector. Since the alias set is small and static, the NPoco builder with WhereIn/explicit parameters would enforce the safe pattern without extra complexity.

• PopulateSortableValueForDatePropertyData.cs:138-148 (SQLite): datetime(json_extract(textValue, '$.date')) is evaluated twice per matched row — once in the subquery WHERE filter and once in the outer UPDATE SET. SQLite doesn't support UPDATE ... LIMIT or UPDATE ... FROM, so the subquery-based batch is necessary, but worth noting if this pattern is copied elsewhere. No correctness issue.

---

Approved with Suggestions for improvement

Good to go, but please carefully consider the importance of the suggestions. The batching logic, index-seek optimization, and loop-termination guards are well-designed. Adding tests for the new early-exit and malformed-data code paths would round this out nicely before merge.

[Copilot]

Pull request overview

Optimizes the v17.3.0 migration that backfills umbracoPropertyData.sortableValue for date/time JSON property editors, addressing timeouts on large SQL Server tables by batching and improving query selectivity.

Changes:

• Adds batched updates (10,000 rows per batch) with per-batch and total progress logging.
• Resolves relevant cmsPropertyType IDs up front and uses a static IN (...) list to avoid problematic SQL Server plans.
• Adds guards to skip unparseable date values to prevent aborts and avoid non-terminating batch loops.

[Zeegaan]

Looks good to me 😁

We've had many of these migration optimizations lately, maybe we should consider some kind of mandatory testing of a large dataset, when we make migrations 🤔 I know we can't catch every case, but it would be nice to atleast catch some of these before they are in the wild 😁