feat(aggregates): add multitenancy bypass option for aggregates

[shahryarjb]

Based on #2372, it covers the aggregates for know, I will try for calculations

I covered these test cases

• Action WITH bypass + Aggregate WITH bypass
• Action WITHOUT bypass + Aggregates WITH and WITHOUT bypass
• Mixed aggregates (bypass and non-bypass) with regular action
• Action WITH bypass + Aggregate WITHOUT bypass
• Without tenant in opts (return error This is the limit of bypass actions; it should still error)

Please review carefully; I might have missed something. Many parts were involved, but I tried to keep codebase changes minimal. All Ash tests passed successfully. In my own project, cases where I previously had to use raw queries in calculations are now easily handled with aggregates. Also, many of my previous duplications are now completely removed.

Contributor checklist

Leave anything that you believe does not apply unchecked.

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

[zachdaniel]

This does look generally good, but I think we're going to have to test this in ash_postgres using context based multi-tenancy as well for resources that support global as well 😓 That is a bit harder, but it will have to affect the way that those join.

[shahryarjb]

@zachdaniel

This does look generally good, but I think we're going to have to test this in ash_postgres using context based multi-tenancy as well for resources that support global as well 😓 That is a bit harder, but it will have to affect the way that those join.

Sure 🙏🏻 please tell me what must do, i will try it
By the way, I don’t have a context-based model in my codebase.
My CMS project has around 4k tests, most of which use global: false, as well as attributes.
I tested it there together with AshJson.
Could you please clarify exactly what I need to do and where I should do it?

I did some changes you told me please re-check it 🙏🏻♥️; By the way i tested the ash_postgres without adding any new tests with new Ash options i added and it has just 2 errors in mix test, but i think these are not about this update! 🤔

For example

  1) test aggregate with parent filter and limited select FAILS when combining select() + limit() with aggregate using parent() in filter (AshSql.AggregateTest)
     test/aggregate_test.exs:1865
     ** (Ash.Error.Unknown)
     Bread Crumbs:
       > Error returned from: AshPostgres.Test.Chat.read

     Unknown Error

     * ** (Postgrex.Error) ERROR 42703 (undefined_column) column s0.last_read_message_id does not exist

And

2) test calculations that refer to aggregates can be authorized (AshPostgres.CalculationTest)
    test/calculation_test.exs:471
    match (=) failed
    code:  assert %{has_future_comment: true} =
             Post
             |> Ash.Query.load([:has_future_comment, :latest_comment_created_at])
             |> Ash.Query.for_read(:allow_any, %{})
             |> Ash.read_one!(authorize?: false)
    left:  %{has_future_comment: true}
    right: %AshPostgres.Test.Post{
             has_future_comment: false,

Thanks!

[zachdaniel]

@shahryarjb there wouldn't be any failing tests yet, because ash_postgres tests wouldn't be using this option. A new test would need to be added to verify that this option works (I don't think it will).

[shahryarjb]

Hi dear @zachdaniel i think my PR in the Ash Postgres needs so much attention, its required-database knowledge is more than me 🥲

PR: ash-project/ash_postgres#649

Thank you in advance

[zachdaniel]

@shahryarjb that PR makes me think that we should just make it so that we only allow this for attribute multitenancy, and that is a change we can make here. WDYT?

[shahryarjb]

Hi dear @zachdaniel i put a comment in the ash postgres, I agree with you as well. ♥️🙏🏻
ash-project/ash_postgres#649 (comment)

[zachdaniel]

Excellent. So with that in mind, what we need to do then is validate when building an aggregate that all resources along the relationship path either have no multitenancy, or have attribute multitenancy. If there is context multitenancy anywhere then we can just raise an error.

[shahryarjb]

Excellent. So with that in mind, what we need to do then is validate when building an aggregate that all resources along the relationship path either have no multitenancy, or have attribute multitenancy. If there is context multitenancy anywhere then we can just raise an error.

@zachdaniel this condtion should be in this pr or should be ash postgres? Hamm it is an easy condition or some think you have in your mind?

[zachdaniel]

@shahryarjb yeah it should be here in this PR 😄 can go somewhere around here: https://github.com/ash-project/ash/pull/2427/files#diff-b7b7a5528d3683c2b41b27b34431108ab641369372ec60ac1c908fa81beb04ffR2792

You should be able to look at each resource along the aggregate.relationship_path and check its multitenancy to ensure its not set to :context.

[shahryarjb]

Hi dear @zachdaniel , could you please re-check my last 2 updates, if is there any consideration i should care please let me know to update!
By the way i decided to have raise Ash.Error.Query.InvalidQuery and force user not to use bypass for context even not using ! in Ash function
Thank you in advance

[zachdaniel]

See my comments. I think we're almost there 🥳

[shahryarjb]

Hi dear @zachdaniel, I did all changes you told me! and hope they will be okey 🧙‍♂️
Thank you in advance for re-checking the PR and last changes ♥️🙏🏻
It passed my tests (in my project) and the Ash tests ☃️

[zachdaniel]

🚀 Thank you for your contribution! 🚀