-
-
Notifications
You must be signed in to change notification settings - Fork 364
feat: relationship through #2553
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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.