Locks are not correctly verified in `can?` function

[sezaru]

Code of Conduct

• I agree to follow this project's Code of Conduct

AI Policy

• I agree to follow this project's AI Policy, or I agree that AI was not used while creating this issue.

Versions

Hex: 2.3.2
Elixir: 1.19.4
OTP: 28.3
ash_postgres 2.6.32

Operating system

NixOS

Current Behavior

Locks like "FOR UPDATE SKIP LOCKED" should be supported. But if you run AshPostgres.DataLayer.can?(Resource, {:lock, "FOR UPDATE SKIP LOCKED"}) you will get false.

Reproduction

Basically add Ash.Query.lock("FOR UPDATE SKIP LOCKED") to a read action in any resource thas uses AshPostgres.

Based on the lock function:

ash_postgres/lib/data_layer.ex

Line 3600 in f15ddb2

lock = "#{lock} SKIP LOCKED"

These types of lock are supported, but they will fail because ash will first call can? and that will return false.

The issue is in this code:

ash_postgres/lib/data_layer.ex

Line 689 in f15ddb2

def can?(_, {:lock, string}) do

  def can?(_, {:lock, string}) do
    string = String.trim_trailing(string, " NOWAIT")

    String.upcase(string) in [
      "FOR UPDATE",
      "FOR NO KEY UPDATE",
      "FOR SHARE",
      "FOR KEY SHARE"
    ]
  end

As you can see, This can logic is incorrect, it should probably be something like this:

def can?(_, {:lock, string}) do
  string |> String.trim_trailing(" NOWAIT") |> String.upcase() |> can_lock?()
end

def can_lock?("FOR UPDATE" <> _), do: true
def can_lock?("FOR NO KEY UPDATE" <> _), do: true
def can_lock?("FOR SHARE" <> _), do: true
def can_lock?("FOR KEY SHARE" <> _), do: true
def can_lock?(_), do: false

Expected Behavior

No response

[zachdaniel]

Would you be interested in making a PR for this? Good find 🙇

[sezaru]

Would that example code I added as a solution be good enough for the PR or should I tackle it another way?

[zachdaniel]

I think we'd want to explicitly matched on the format of SKIP LOCKED, but fixing it in the can? function is correct yes.

[zachdaniel]

We don't want to allow any arbitrary text through due to safety issues as these are unquoted literals.

[sezaru]

What about this? I think it matches the lock functions

# Move @locks to the top of the file
@locks [
   "FOR UPDATE",
   "FOR NO KEY UPDATE",
   "FOR SHARE",
   "FOR KEY SHARE"
 ]

def can?(_, {:lock, string}), do: string |> String.upcase() |> can_lock?()

for lock <- @locks do
  def can_lock?(lock), do: true

  for suffix <- ["NOWAIT", "SKIP LOCKED"] do
    def can_lock?("#{lock} #{suffix}"), do: true
  end
end

def can_lock?(_), do: false

[zachdaniel]

Yes, that looks perfect 👍

[sezaru]

Actually, there is something else that I'm not sure if it is a bug or not.

Here:

  for lock <- @locks do
    frag = "#{lock} OF ?"

    def lock(query, unquote(lock), _) do
      {:ok, Ecto.Query.lock(query, [{^0, a}], fragment(unquote(frag), a))}
    end

    frag = "#{lock} OF ? NOWAIT"
    lock = "#{lock} NOWAIT"

    def lock(query, unquote(lock), _) do
      {:ok, Ecto.Query.lock(query, [{^0, a}], fragment(unquote(frag), a))}
    end

    frag = "#{lock} OF ? SKIP LOCKED"
    lock = "#{lock} SKIP LOCKED"

    def lock(query, unquote(lock), _) do
      {:ok, Ecto.Query.lock(query, [{^0, a}], fragment(unquote(frag), a))}
    end
  end

Assuming that the lock variable in the loop is FOR UPDATE, the line lock = "#{lock} SKIP LOCKED" will generate FOR UPDATE NOWAIT SKIP LOCKED.

Is this correct? In my case, I want to add FOR UPDATE SKIP LOCKED only, without the NOWAIT, which with the current code is not supported

[sezaru]

Looking more into it, it seems as a bug since NOWAIT and SKIP LOCKED are mutually exclusive.

I think the code should be rewritten like this:

  for lock <- @locks do
    frag = "#{lock} OF ?"

    def lock(query, unquote(lock), _) do
      {:ok, Ecto.Query.lock(query, [{^0, a}], fragment(unquote(frag), a))}
    end

    frag = "#{lock} OF ? NOWAIT"
    new_lock = "#{lock} NOWAIT"

    def lock(query, unquote(new_lock), _) do
      {:ok, Ecto.Query.lock(query, [{^0, a}], fragment(unquote(frag), a))}
    end

    frag = "#{lock} OF ? SKIP LOCKED"
    new_lock = "#{lock} SKIP LOCKED"

    def lock(query, unquote(new_lock), _) do
      {:ok, Ecto.Query.lock(query, [{^0, a}], fragment(unquote(frag), a))}
    end
  end

[sezaru]

#704