Skip to content
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

Non-existent filter arguments #585

Open
smcabrera opened this issue Feb 15, 2025 · 0 comments · May be fixed by #586
Open

Non-existent filter arguments #585

smcabrera opened this issue Feb 15, 2025 · 0 comments · May be fixed by #586

Comments

@smcabrera
Copy link

I'm using ActiveInteraction 5.5.0

Filters will allow you to include keyword arguments that don't exist:

class MyInteraction < ActiveInteraction::Base
  string :beverage, type: :sparkling_wine, bubbles: true

  def execute
    puts "Enjoy your #{beverage}!"
  end
end

MyInteraction.run!(beverage: 'Cava')

While these vestigial arguments are mostly harmless it could confuse people if they misunderstand the docs or misspell something and then wonder why it doesn't work as they expect

class SquareIds < ActiveInteraction::Base
  array :ids, type: :integer, descl: 'Documentation of this filter'

  def execute
    puts ids.map { _1 ** 2 } 
  end
end

SquareIds.run!(ids: [1, 2, 'BAD'])
# bin/script:15:in `block in execute': undefined method `**' for an instance of String (NoMethodError)

A user might think "Why didn't my type checking protect me?" when really it's because type doesn't exist. They should have done:

array :ids do
  integer
end

But instead they misremembered (or perhaps an LLM hallucinated 😅) there being a type kwarg.

Why isn't the documentation they wrote present? Well they just misspelled desc as descl...but we didn't yell at them.

I haven't dug into the source yet but I'm guessing if we were to add an allowlist to check keywords against we could ensure that only kwargs that actually do something are let through. Assuming maintainers think that's desirable? It could be a breaking change but hopefully not a hard one for users to address (remove your keyword arguments that weren't doing anything in the first place?)

@smcabrera smcabrera linked a pull request Feb 17, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant