Fix wrong sort order at UTC month boundaries and pre-release versions#4687
Conversation
`Enum.sort_by/3` with `:asc`/`:desc` does structural comparison on `DateTime` structs, ordering by struct-key sequence (day before month before year) rather than chronologically. The "Latest Work Order" column relied on that comparison and produced inverted orderings whenever the most recent work orders for two workflows straddled a UTC date boundary, so ordering by that column was wrong roughly the first few hours after UTC midnight every day. Map the timestamp to integer microseconds inside the per-field sorter so the call site stays uniform and any future datetime-valued sort key gets the same treatment. Pin the chronological behaviour with a regression test using fixed `DateTime` values that cross midnight, so the test is independent of wall-clock time, and tighten the existing ascending/descending tests so they assert against a chronological oracle instead of structural sort.
|
The change is purely an in-memory sort comparator fix in Security Review ✅
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4687 +/- ##
=======================================
Coverage 89.88% 89.89%
=======================================
Files 444 444
Lines 21941 21951 +10
=======================================
+ Hits 19722 19733 +11
+ Misses 2219 2218 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The previous wording said "UTC date boundary", which overstates the trigger. Mid-month UTC midnight rollovers do not fire the bug. Only month boundaries do, because that is when the day-of-month resets to a smaller number and the structural comparison inverts.
The test name said "across UTC midnight", which overstates the trigger. Mid-month UTC midnight rollovers do not fire the bug. Only the day-of- month resetting at a month boundary does. Match the language used in the changelog and PR description.
`Enum.sort_by(versions, &Version.parse/1, :desc)` does structural
comparison on the resulting `Version` structs, which orders by struct
keys alphabetically (build, major, minor, patch, pre) rather than
following semver. Pre-release versions sort after their corresponding
release, so a `1.0.0-beta` would appear above `1.0.0` in the dropdown.
Switch to `Enum.sort(versions, {:desc, Version})` so the comparison
goes through `Version.compare/2`. Add a regression test using
pre-release versions, and tighten the existing ordering assertion to
use the same semver oracle.
The support-user variant of `get_projects_overview/2` builds project
rows in memory and sorts them with `Enum.sort_by(rows, key_fn,
sort_direction)`. When the caller passes `order_by:
{:last_updated_at, :desc | :asc}` the key is a `DateTime`, so the
sort uses Erlang structural term order, which walks struct keys
alphabetically (day before month before year) and inverts at month
boundaries.
Map the value through a sort-key helper that converts `DateTime` to
unix microseconds before sorting, leaving non-datetime keys
(`:name`, `:workflows_count`, etc.) untouched. Pin the chronological
behaviour with a regression test that uses a support user and
timestamps straddling a UTC month boundary.
`TableHelpers.sort_items/4` uses `<=`/`>=` as the comparator (via its `sort_compare_fn`), so when a sort_map points to a `DateTime`, `NaiveDateTime`, `Date`, or `Time` value the comparison runs through Erlang structural term order. That walks struct keys alphabetically (day before month before year) and disagrees with chronology at month boundaries, the same root cause we hit on the dashboard and projects- overview sorts. The admin Projects table exposes "Created at" and "Scheduled deletion" sort columns. The admin Users table exposes "Scheduled deletion". Both go through this helper. Centralize the fix in `get_sort_value/2`'s normalizer: convert `DateTime`/`NaiveDateTime`/`Date`/`Time` values to ISO 8601 strings, which sort lexicographically the same as chronologically. Cover the behaviour with a focused test on `sort_items/4` using fixed timestamps that straddle a UTC month boundary.
Add tests for the case clauses introduced alongside the sort fixes that the regression tests did not exercise: - `overview_sort_key/2` in `Lightning.Projects`: nil `last_updated_at` (project with no workflows) and non-DateTime sort keys (`:name`). - `TableHelpers.normalize_sort_value/1`: `NaiveDateTime`, `Date`, `Time`, and nil values (both explicit nil and missing field). - `TableHelpers.get_sort_value/2`'s catch-all clause: sort key absent from the sort_map (input order preserved). - `TableHelpers.sort_items/4` on a plain string field, to pin the default lexicographic compare.
taylordowns2000
left a comment
There was a problem hiding this comment.
Approved, but please update the changelog (link to the issue or the PR itself) and consider these before merging:
- What was using the DateTime that you're now converting to Unix? Do all those downstream sites (maybe UI displays of the dates?) now need updating too?
- Does the
nil -> 0branch map missing timestamps to the Unix epoch? Mixing "absent" with "1970-01-01" would not be great.
The sort-key extractors previously mapped nil to a bare `0` (or `""`),
which collides with the unix-epoch value of a hypothetical
`~U[1970-01-01 00:00:00Z]` row at the comparator level. Switch to a
2-tuple `{0, _}` for nil and `{1, value}` for present, so nil rows can
never tie with a real timestamp regardless of what value falls into
the second slot. Same user-visible ordering (nils still at the low
end), but the theoretical tie is impossible by construction.
Cover the behaviour with a regression test on each affected helper
that mixes nil and a real `~U[1970-01-01 00:00:00Z]` row in both
directions.
Per review feedback, each CHANGELOG entry should reference the PR (or an issue) so the change can be traced from release notes back to the diff. The four sort fixes ship in #4687.
The previous version had four bullets, each pointing at the same PR. Group the chronological-sort fixes (workflow list, projects overview, admin tables) into one bullet that names the affected columns, and keep the adaptor version sort as a separate bullet because the failure mode (semver vs chronology) is qualitatively different. Drops the internal-type names from the user-facing prose.
midigofrank
left a comment
There was a problem hiding this comment.
Nicely done @elias-ba , I'm just not sure why we need the 0 and 1,
why can't we do DateTime.to_unix("1970-01-01") when it's nil? The tuple makes it feel complicated
|
@midigofrank thanks for the review man! I totally hear you on the tuple feeling complicated, and I went back and forth on it myself. Let me explain where the There's a business requirement we need to honor: a nil row and a hypothetical real The tuple is the smallest change I could find that keeps both worlds:
We also shipped this same shape in the billing app for the same root-cause class, so I'd like to keep it consistent here too. |
…atetime-sort # Conflicts: # CHANGELOG.md
|
Aaah, okay. Makes sense |
Description
Several places in the codebase used
Enum.sort_by(.., :asc | :desc)(or the equivalentEnum.sort/2form, or<=/>=as a comparator) on a key whose value was aDateTimeor aVersionstruct. That form does Erlang structural term comparison, which on these structs walks fields alphabetically and disagrees with chronological / semver order. Same root cause across four surfaces.order_by.TableHelpers.sort_items/4helper, which used<=/>=(structural compare) and produced the same inversion at month boundaries.Each fix maps the sort key through a domain-aware comparator:
DateTimebecomes integer microseconds (or ISO 8601 strings inside the shared table helper), version strings go throughVersion.compare/2. The dashboard, projects-overview, and admin-table fixes have real user impact at month boundaries. The adaptor version fix is a real-but-currently-zero-impact correction (OpenFn adaptors essentially always ship plainX.Y.Zsemver, so today no user is hitting it; included here because it is the same root cause).Validation steps
Run the affected test files together:
All tests pass on this branch, including four new regression tests that pin chronological / semver ordering across boundaries.
Quick spot-check in
iex -S mix:Additional notes for the reviewer
AI Usage
Pre-submission checklist
/reviewwith Claude Code):owner,:admin,:editor,:viewer)