Skip to content

feat!: redesign required option, add validates option#7

Open
bharget wants to merge 4 commits intomainfrom
ben/fix-required-validation
Open

feat!: redesign required option, add validates option#7
bharget wants to merge 4 commits intomainfrom
ben/fix-required-validation

Conversation

@bharget
Copy link
Copy Markdown
Owner

@bharget bharget commented Dec 18, 2025

What changed

required: true now raises an ArgumentError immediately if the argument isn't provided.

Before this change, required: true added an ActiveModel presence validation. This was problematic because:

  1. Presence validation calls blank? on the value, which triggers database queries on ActiveRecord relations
  2. The error only surfaced after calling valid?, not at instantiation
  3. It conflated "argument must be provided" with "value must not be blank"

Now, required: true simply checks if the key was included in the arguments hash. If not, it raises ArgumentError right away.

# Raises ArgumentError: missing required argument: user
MyService.call

# These all work fine - the key is provided
MyService.call(user: some_user)
MyService.call(user: nil)    # nil is allowed
MyService.call(user: false)  # false is allowed

New validates: option

If you want ActiveModel validations, there's now an explicit validates: option:

argument :email, validates: { format: { with: /@/ } }
argument :name, required: true, validates: { length: { minimum: 2 } }

Migration

A rake task is included to automatically update your services:

rake telephone:migrate                    # defaults to app/services
rake telephone:migrate[path/to/services]  # custom path

This adds validates: { presence: true } to any argument with required: true, preserving the old behavior.

Manual migration

If you were relying on required: true to validate that values aren't blank:

# Before
argument :user, required: true

# After - same validation behavior as before
argument :user, required: true, validates: { presence: true }

If you just wanted to ensure the argument was passed, no changes needed.

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 this pull request may close these issues.

2 participants