Skip to content

Validate cursor value types - #669

Merged
woylie merged 5 commits into
woylie:mainfrom
patric-vinicios:validate-cursor-value-types
Jul 20, 2026
Merged

Validate cursor value types#669
woylie merged 5 commits into
woylie:mainfrom
patric-vinicios:validate-cursor-value-types

Conversation

@patric-vinicios

Copy link
Copy Markdown
Contributor

Description

Closes #668

This is the follow-up to the issue above. Since cursor validation only compared the cursor keys with the order_by fields, a cursor with the right keys but a value of the wrong type would pass validation and only fail later with an Ecto.Query.CastError when the query ran.

Cursor values now go through Ecto.Type.cast/2 against the type of their order_by field, reusing the same idea as the filter value casting. If a value can't be cast, validation returns an "is invalid" error on :after/:before — and with replace_invalid_params: true the cursor is simply dropped, as you suggested in the issue.

I was careful not to change behavior where there's nothing to cast: without the for: option, for fields that don't have a known ecto_type (join fields without the option, for example), and for nil values coming from nullable order fields, everything works as before.

Two things I'd like your opinion on: I duplicated the small expand_type/1 helper that already exists (privately) in Flop.Filter to keep the diff small — happy to extract it somewhere shared instead. And the {:ecto_enum, _} clause isn't directly covered by tests, because no test schema has a sortable join field with that type; I can add one if you think it's worth it.

Checklist

  • I added tests that cover my proposed changes.
  • I read, understood, and agree to the contribution guidelines.
  • I certify I have the right to submit this work under the project's license.

@patric-vinicios
patric-vinicios requested a review from woylie as a code owner July 17, 2026 18:29
@woylie

woylie commented Jul 20, 2026

Copy link
Copy Markdown
Owner

I duplicated the small expand_type/1 helper that already exists (privately) in Flop.Filter to keep the diff small — happy to extract it somewhere shared instead.

Yes, I think this function can go to Flop.Misc. Other than that, it looks good to me!

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.58%. Comparing base (3526e1c) to head (05860ae).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #669      +/-   ##
==========================================
+ Coverage   84.45%   86.58%   +2.12%     
==========================================
  Files          15       15              
  Lines        1036     1021      -15     
==========================================
+ Hits          875      884       +9     
+ Misses        161      137      -24     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@woylie

woylie commented Jul 20, 2026

Copy link
Copy Markdown
Owner

Could you bump postgrex to make hex.audit pass? @patric-vinicios

@patric-vinicios

Copy link
Copy Markdown
Contributor Author

Could you bump postgrex to make hex.audit pass? @patric-vinicios

postgrex is bumped to 0.22.3 and hex.audit passes now.

One thing worth mentioning: the lock diff is larger than just postgrex. The mix.exs on main pins credo == 1.7.19 and ecto_sqlite3 == 0.24.1, but the committed mix.lock still had 1.7.18 and 0.24.0. That mismatch causes every mix task to stop with a lock mismatch error before it can run, hex.audit included, so the bump required mix deps.get to resync those two as well. That in turn updated ecto to 3.14.1, db_connection to 2.10.2 and exqlite to 0.39.0.

I first tried limiting the change to a single line in the lock file, but Mix rejects it. If you would prefer to keep this PR focused, I am happy to move the resync into a separate PR.

@woylie
woylie merged commit 6a5ffbf into woylie:main Jul 20, 2026
20 checks passed
@woylie

woylie commented Jul 20, 2026

Copy link
Copy Markdown
Owner

All good, thank you very much!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Tampered cursor passes validation but raises Ecto.Query.CastError when running the query

2 participants