Fix pagination for JSON:API related routes

[Dgoz101]

Contributor checklist

Leave anything that you believe does not apply unchecked.

• [X ] I accept the AI Policy, or AI was not used in the creation of this PR.
• [ X] Bug fixes include regression tests
• Chores
• Documentation changes
• Features include unit/acceptance tests
• Refactoring
• Update dependencies

This fixes related routes not correctly being paginated. Will now correctly attach the pagination rules to the query. Running mix format changed documentation/dsls/DSL-AshJsonApi.Domain.md and documentation/dsls/DSL-AshJsonApi.Resource.md for some reason. Wasn't sure if they should be restored to the originals or not so I left them in.

[zachdaniel]

This is interesting. Seems like maybe we actually do support pagination already? Can you confirm your tests don't already pass against this original behavior?

[Dgoz101]

I'll take a look. From what I thought was happening was pagination for related routes wasn't quite working but it was for primary routes. I'll work on it now.

[Dgoz101]

Ok so I reverted get_related.ex and fetch_related in helpers.ex to match the version that still had load_params + put_context(:override_domain_params, load_params) and re-ran:

index_pagination_test.exs → still passed
get_related_test.exs → the two “related endpoint with pagination” cases fail: with
?page[limit]=2&page[offset]=0 the response still returns all 5 related records instead of 2 (same for the offset case).
So that path wasn’t applying pagination to the related-route load; primary/index pagination was already working.

With the changes (fetch_pagination_parameters in GetRelated, for_read + Ash.Query.page on the destination query), those tests pass again.

[zachdaniel]

Got it, so the main thing I want to make sure of is that on these related endpoints that we aren't accidentally changing the behavior in some way when page is not supplied, like on actions where pagination is marked as required etc. As long as its not changing existing behavior than all is well 😄 (aside from fixing the usage of page param)

[Dgoz101]

I see. When page isn’t in the query, it doesn't call Ash.Query.page/2. Only Request.opts with a real page (from page[...]) triggers that. The existing related-route tests that don’t pass page are still passing, so its not changing that path just to “always page.” The only subtle difference is now for_read runs on the destination query. So for the most part it shouldn't, might for some edge cases. If you want, I can add an acceptance test or something along the sort to guarantee.

[zachdaniel]

Speaking of that, I'm not sure that it should run on each call. |> Ash.Query.for_read(request.action.name, request.arguments, read_opts) Is request.action.name referring to the action on the destination resource? If its a related route the action would come from the relationship, and I IIRC that action name refers to the root read action's name? Would need to double check. But I don' thtink we need to run for_read to make this work do we?

[Dgoz101]

Sorry this is a bit long of a reply, I want to be thorough. After playing around a bit and researching here's what I've come up with:

Is request.action.name referring to the action on the destination resource?

• request.action.name is the read action’s name from the route (e.g. :read). It is not “the destination resource’s Ash.Action struct” on request—that struct is often the base resource’s read. When you call Ash.Query.for_read on a query for relationship.destination, Ash resolves and runs the destination resource’s read whose name is that atom. So the name lines up with the destination read; the request.action value is still usually the base resource’s action object.

If its a related route the action would come from the relationship, and I IIRC that action name refers to the root read action's name?

• The read name is whatever the route says (e.g. related :comments, :read → :read). That names the read you intend to use on the related resource; it isn’t a separate “relationship action” object—it's the route DSL tying the related endpoint to a read by name. In the router we resolve request.action with Ash.Resource.Info.action(base_resource, route.action), so the request.action struct is often the root/base resource’s read with that name. The important part for the nested query is that we build the query on relationship.destination and call for_read(..., request.action.name, ...), so Ash runs the destination resource’s read named :read (or whatever the route specified), not the root’s read logic on the child. So: the name matches the route (and usually the child read you care about); the resolved action on request can still be the root read struct because of how the route is looked up.

But I don't think we need to run for_read to make this work do we?

• We might get limit/offset to apply in a toy example by only building filter / sort / load and then Ash.Query.page/2, but that skips everything the read action normally wires up (arguments, preparations, the read’s pagination definition, etc.) and diverges from how index builds its query. For this bug, the behavior we wanted was “related GET paginates like index,” and the path that actually fixed it in tests was for_read on the destination query plus Ash.Query.page/2 when page is in opts. A lighter query without for_read was what we had before and didn’t paginate the related load reliably. So: not strictly impossible in theory, but we do want for_read here for correctness and parity with index.

[zachdaniel]

request.action.name is the read action’s name from the route (e.g. :read). It is not “the destination resource’s Ash.Action struct” on request—that struct is often the base resource’s read. When you call Ash.Query.for_read on a query for relationship.destination, Ash resolves and runs the destination resource’s read whose name is that atom. So the name lines up with the destination read; the request.action value is still usually the base resource’s action object.

Right, so this is a problem. You're building a query against the destination resource, using an action name from the source resource. This is only working in the tests because the actions happen to have the same name.

As for for_read, can we just try removing it? I'm pretty sure this will all "just work" without it.

[Dgoz101]

Ahh I see what you are saying. Yes I think you are right. Let me make that change.

[Dgoz101]

Removed for_read and it still passes everything. I tried running spark.cheat_sheets and spark.formatter again but it still isn't changing anything. Deps are up to date so I'm not sure why it isn't working.

[Dgoz101]

Also wanted to mention I've tried to run mix spark.cheat_sheets in a couple different clones of my repo but it's not letting me push the changes afterwards for whatever reason and then it won't do anything when I go to run it again. Same with mix format.

[zachdaniel]

🚀 Thank you for your contribution! 🚀