Generated filter expression is incorrect - BUG

Hello, I think I found a bug in Ash's query builder. I have the following filter expression in my resource action:

      filter expr(
               shared? == :everyone or
                 (shared? == :organization and organization_id == ^arg(:organization_id)) or
                 (shared? == :school and school_id == ^arg(:school_id))
             )

      filter expr(
               shared? == :everyone or
                 (shared? == :organization and organization_id == ^arg(:organization_id)) or
                 (shared? == :school and school_id == ^arg(:school_id))
             )

As you can see, this expression basically says "If shared? is everyone, return it, or, if shared? is organization and organization_id matches, return it, or, if shared? is school and school_id matches, return it". I was expecting the SQL generated from it being something like this:

WHERE (
  "shared?" = 'everyone'
  OR ("shared?" = 'organization' AND organization_id = null)
  OR ("shared?" = 'school' AND school_id = null)
)

WHERE (
  "shared?" = 'everyone'
  OR ("shared?" = 'organization' AND organization_id = null)
  OR ("shared?" = 'school' AND school_id = null)
)

But what Ash.Query generated was:

WHERE (
  (organization_id = null AND "shared?" IN ('organization','everyone'))
  OR ("shared?" = 'school' AND school_id = null)
)

WHERE (
  (organization_id = null AND "shared?" IN ('organization','everyone'))
  OR ("shared?" = 'school' AND school_id = null)
)

Note: I passed nil as arg(:organization_id) and arg(:school_id) in this case. Which is incorrect since when shared? is everyone, it will only return it if organization_id matches. What I believe happened here is that Ash.Query ignored the parenthesis in this part of the expression (shared? == :organization and organization_id == ^arg(:organization_id)) since if you remove it, then the generated where expression makes sense.

11 Replies

ZachDaniel•2y ago

This looks like an expression optimization bug Specifically we're lifting up a value out of an or == into an in incorrectly

BlibsOP•2y ago

Is this a Ash or AshPostgres issue, do you want me to create a issue for this in one of these repos?

ZachDaniel•2y ago

Its in Ash I'm looking into it at the moment actually (pretty serious bug that should be fixed ASAP) Just released a new version with the fix 2.13.4

BlibsOP•2y ago

Thanks @Zach Daniel I just tested it and it is working great. Sorry to interrupt your well deserved vacation with this 😅

barnabasj•2y ago

I upgradet to main this morning and I'm seeing a really weird query for an update action This is the policy for the action

      %Ash.Policy.Policy{
        condition: [
          {Ash.Policy.Check.Action, [action: :add_viewer]},
          {Ash.Policy.Check.Expression,
           [
             expr: not is_nil({:_actor, :contact_id}) and (customer_id == {:_actor,
              :contact_id} or customer_natural_contact_id == {:_actor,
              :contact_id})
           ]}
        ],
        policies: [
          %Ash.Policy.Check{
            check: {AshRbac.HasRole, [role: [roles: [:user]]]},
            check_module: AshRbac.HasRole,
            check_opts: [role: [roles: [:user]]],
            type: :authorize_if
          }
        ],
        bypass?: nil,
        checks: nil,
        description: nil,
        access_type: nil
      },

      %Ash.Policy.Policy{
        condition: [
          {Ash.Policy.Check.Action, [action: :add_viewer]},
          {Ash.Policy.Check.Expression,
           [
             expr: not is_nil({:_actor, :contact_id}) and (customer_id == {:_actor,
              :contact_id} or customer_natural_contact_id == {:_actor,
              :contact_id})
           ]}
        ],
        policies: [
          %Ash.Policy.Check{
            check: {AshRbac.HasRole, [role: [roles: [:user]]]},
            check_module: AshRbac.HasRole,
            check_opts: [role: [roles: [:user]]],
            type: :authorize_if
          }
        ],
        bypass?: nil,
        checks: nil,
        description: nil,
        access_type: nil
      },

It loggs this select query first:

SELECT t0."id" FROM "travel_plan" AS t0 WHERE ((((t0."customer_id"::uuid = $1::uuid) OR (t0."customer_natural_contact_id"::uuid = $2::uuid)) AND NOT ($3::boolean)) OR (t0."id"::uuid = $4::uuid))

SELECT t0."id" FROM "travel_plan" AS t0 WHERE ((((t0."customer_id"::uuid = $1::uuid) OR (t0."customer_natural_contact_id"::uuid = $2::uuid)) AND NOT ($3::boolean)) OR (t0."id"::uuid = $4::uuid))

afterwards, it starts the transaction for the update

10:40:36.205 [debug] Successful authorization: Demo.ConciergeService.Resources.TravelPlan.add_viewer


Generated Filter:
not is_nil("06dee239-863c-4de0-8579-f74ef05432ca") and (customer_id == "06dee239-863c-4de0-8579-f74ef05432ca" or customer_natural_contact_id == "06dee239-863c-4de0-8579-f74ef05432ca")


10:40:36.207 [debug] QUERY OK source="travel_plan" db=0.2ms queue=0.2ms
SELECT t0."id" FROM "travel_plan" AS t0 WHERE ((((t0."customer_id"::uuid = $1::uuid) OR (t0."customer_natural_contact_id"::uuid = $2::uuid)) AND NOT ($3::boolean)) OR (t0."id"::uuid = $4::uuid)) LIMIT $5 ["06dee239-863c-4de0-8579-f74ef05432ca", "06dee239-863c-4de0-8579-f74ef05432ca", false, "2c464e07-8cd0-4c1c-bd95-251e5e1c9f55", 1]
10:40:36.208 [debug] QUERY OK db=0.0ms
begin []
10:40:36.208 [debug] QUERY OK db=0.2ms
UPDATE "travel_plan" SET "updated_at" = $1, "updated_by" = $2 WHERE "id" = $3 [~U[2023-08-09 08:40:36.208187Z], "customer", "2c464e07-8cd0-4c1c-bd95-251e5e1c9f55"]

10:40:36.205 [debug] Successful authorization: Demo.ConciergeService.Resources.TravelPlan.add_viewer


Generated Filter:
not is_nil("06dee239-863c-4de0-8579-f74ef05432ca") and (customer_id == "06dee239-863c-4de0-8579-f74ef05432ca" or customer_natural_contact_id == "06dee239-863c-4de0-8579-f74ef05432ca")


10:40:36.207 [debug] QUERY OK source="travel_plan" db=0.2ms queue=0.2ms
SELECT t0."id" FROM "travel_plan" AS t0 WHERE ((((t0."customer_id"::uuid = $1::uuid) OR (t0."customer_natural_contact_id"::uuid = $2::uuid)) AND NOT ($3::boolean)) OR (t0."id"::uuid = $4::uuid)) LIMIT $5 ["06dee239-863c-4de0-8579-f74ef05432ca", "06dee239-863c-4de0-8579-f74ef05432ca", false, "2c464e07-8cd0-4c1c-bd95-251e5e1c9f55", 1]
10:40:36.208 [debug] QUERY OK db=0.0ms
begin []
10:40:36.208 [debug] QUERY OK db=0.2ms
UPDATE "travel_plan" SET "updated_at" = $1, "updated_by" = $2 WHERE "id" = $3 [~U[2023-08-09 08:40:36.208187Z], "customer", "2c464e07-8cd0-4c1c-bd95-251e5e1c9f55"]

ZachDaniel•2y ago

Can you show me what it was doing before/highlight the part that is strange?

barnabasj•2y ago

SELECT t0."id" FROM "travel_plan" AS t0 WHERE ((((t0."customer_id"::uuid = $1::uuid) OR (t0."customer_natural_contact_id"::uuid = $2::uuid)) AND NOT ($3::boolean)) OR (t0."id"::uuid = $4::uuid))

SELECT t0."id" FROM "travel_plan" AS t0 WHERE ((((t0."customer_id"::uuid = $1::uuid) OR (t0."customer_natural_contact_id"::uuid = $2::uuid)) AND NOT ($3::boolean)) OR (t0."id"::uuid = $4::uuid))

The last OR circumvents the other where clauses after the update, the action was accessible by users that didn't match the customer id

ZachDaniel•2y ago

🥶 it was basically a typo I'm away from my computer will fix ASAP

barnabasj•2y ago

@Blibs you should probably update again

BlibsOP•2y ago

@barnabasj thanks for the heads up, will do. That actually made me wonder, is there some way to disable that query optimizer from Ash.Query? I mean, correct me if I'm wrong, but AFAIK, when using ecto, the query that I write will be exactly the query that ecto will output, there is no optimization from their part. I kinda like that behavior since it makes the developer responsibility to make the best query possible.

ZachDaniel•2y ago

The query optimizer we have is generally very simple, specifically for turning things like (a == 1 or a == 2) into (a in [1, 2]) Beyond that it doesn’t do much

Gaming

Programming

Generated filter expression is incorrect - BUG

Did you find this page helpful?