Conversation
|
This is really great! I believe there is really only one main thing to add to this, but it is a very complex thing. Ash automatically applies relationship filters as part of the filters when you do something like this: Resource
|> Ash.Query.filter(related_thing.foo == bar)
|> Ash.read!()implicitly becomes: Resource
|> Ash.Query.filter(related_thing.foo == bar and related_thing.<the_read_policies>)
|> Ash.read!()In the case of these changes, what has to happen is: Resource
|> Ash.Query.filter(related_thing.foo == bar and related_thing.<the_read_policies> and first_through.<the_read_policies> and second_through.<the_read_policies> ....)
|> Ash.read!()I'm not sure if that will be enough to go off of on its own, but you can see where we generate these filters here: {:ok, relationship_path_filters} <-
Ash.Filter.relationship_filters(
query.domain,
pre_authorization_query,
opts[:actor],
query.tenant,
agg_refs(query, data_layer_calculations ++ [{nil, filter}]),
opts[:authorize?]
),in This is super cool and definitely looks like a basis by which we can proceed. The other PRs look pretty good, I haven't quite thought through the SQL bits but I'm confident they can be worked through 😄 |
|
@zachdaniel I made some changes to support policies, it wasn't that complex but I might have missed something? Should I expand the tests to try to find some edge cases? 🤔 |
|
Wow, I think you did it 🥳. Only thing left is to write a bunch of tests, especially for the authorization changes 😄 |
|
Where should I add more tests, in ash or ash_postgres? 🤔 I'll assume in ash_ postgres, stupid question really. 😅 |
|
In both really. Or are these all already tested in the |
|
I have basic tests in ash_postgres |
|
Maybe I could expand some tests in ash.. 🤔 |
|
We definitely need good tests around this, ideally in both places, and 100% around the authorization logic 🙇 |
#72
#1780
Related to: ash-project/ash_postgres#686 & ash-project/ash_sql#212
Finally made a second crack at this. 🐛
Zach, what have I missed? What should be done differently? What more tests are needed to assert this works good?
Contributor checklist
Leave anything that you believe does not apply unchecked.