Skip to content

Remove incorrect documentation about combining values and allow_blank - #2631

Closed
eriklovmo wants to merge 1 commit into
ruby-grape:masterfrom
eriklovmo:erik/remove-faulty-documentation-about-blank-values
Closed

Remove incorrect documentation about combining values and allow_blank#2631
eriklovmo wants to merge 1 commit into
ruby-grape:masterfrom
eriklovmo:erik/remove-faulty-documentation-about-blank-values

Conversation

@eriklovmo

@eriklovmo eriklovmo commented Dec 4, 2025

Copy link
Copy Markdown
Contributor

The deleted example was incorrect: you can, in fact, not supply a blank value to the state parameter with such a params block. I don't know the background, maybe it was correct at one point in time.

The example is incorrect: you can, in fact, not supply a blank
value to `state` with that `params` block.
@eriklovmo

Copy link
Copy Markdown
Contributor Author

I did not update the changelog because it seemed like an insignificant patch. Is that OK?

@eriklovmo
eriklovmo marked this pull request as ready for review December 4, 2025 13:11
@ericproulx

Copy link
Copy Markdown
Contributor

@eriklovmo I do confirm the behavior

# frozen_string_literal: true

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'
  gem 'grape'
  gem 'rack'
  gem 'minitest'
  gem 'rack-test'
end

require 'minitest/autorun'
require 'rack/test'
require 'grape'

class GrapeAPIBugTest < Minitest::Test
  include Rack::Test::Methods

  def setup
    @api = Class.new(Grape::API) do
      prefix :api
      version 'v1', using: :path
  
      params do
        requires :state, type: Symbol, values: [:active, :inactive]
      end
  
      post '/' do
        'hello'
      end
    end.new
  end

  def test_v1_users_via_api
    options = {
      method: Rack::POST,
      params: {
        state: nil
      }
    }
    env = Rack::MockRequest.env_for('/api/v1', options)
    response = Rack::MockResponse[*@api.call(env)]

    assert_equal 'hello', response.body
    assert_equal 201, response.status
  end
end

It fails

@ericproulx

Copy link
Copy Markdown
Contributor

But #2185, is mentionning something about optional vs requires. Test is passing when updating the test with optional.

@eriklovmo

Copy link
Copy Markdown
Contributor Author

@eriklovmo I do confirm the behavior

Thanks!

But #2185, is mentionning something about optional vs requires. Test is passing when updating the test with optional.

OK, this might warrant a clarifying note in the docs. When using optional, validation also passes when given an empty string "", which appears to be coerced into nil. I haven’t looked into the code mechanics yet, but do you think the underlying issue is the ambiguity around "blankness", or is it that we’re conflating optional keys with optional values?

@ericproulx

@ericproulx

ericproulx commented May 8, 2026

Copy link
Copy Markdown
Contributor

@eriklovmo Verified — your removal is correct. Behavior matrix from a runnable repro:

params block omitted nil "" invalid valid
requires :state, type: Symbol, values: [...] 400 missing 400 invalid 400 invalid 400 invalid 201
optional :state, type: Symbol, values: [...] 201 absent 201 nil 201 nil 400 invalid 201
optional :state, type: Symbol, values: [...], allow_blank: false 201 absent 400 empty 400 empty 400 invalid 201

To your question — "ambiguity around blankness, or conflating optional keys with optional values?" — it's the latter. Grape::Validations::Validators::ValuesValidator#validate_param! (lib/grape/validations/validators/values_validator.rb:30) short-circuits on nil whenever the param is optional at the root scope:

return if val.nil? && !required_for_root_scope?

This runs before @allow_blank is consulted (line 31), so optional + values: accepts nil regardless of allow_blank setting. @allow_blank itself defaults to nil (lib/grape/validations/validators/base.rb:62), not true — that's the second factual error in the deleted note.

The ""-passes-too observation is the Symbol coercer turning empty strings into nil, which then takes the same line-30 path.

To reject blank values from an optional :values param, the user has to add allow_blank: false. That's handled by a separate AllowBlankValidator (lib/grape/validations/validators/allow_blank_validator.rb) that runs ahead of values validation and rejects nil/empty when the key is present; absent keys still pass, which is the right semantics for optional.

I rolled this into #2700 (the README/docs cleanup PR) with the deletion you proposed plus a clarifying replacement note covering optional + values and the allow_blank: false opt-out. Closing this in favor of that — thanks for catching the bug in the original docs!

ericproulx added a commit that referenced this pull request May 8, 2026
…l + values

Spelling and grammar:
- "given on an `configuration`" → "based on a `configuration`"
- "Accept` head" → "Accept` header"
- "won't affects" → "won't affect"
- "with' blocks" → "with` blocks" (mismatched quote)
- `required :beer` / `:wine` / `:blah` → `requires` (the DSL method
  is `requires`; the samples as written would raise NoMethodError)
- "the the [grape-entity]" → "the [grape-entity]"

Accuracy:
- §"Accept-Version Header": the strict-mode sentence said the 406 was
  raised when no correct `Accept` header was supplied; the middleware
  checks `HTTP_ACCEPT_VERSION`, so corrected to "Accept-Version".
- "Rack::Etag" → "Rack::ETag" (correct class name).
- Removed the "Integer/Fixnum and Coercions" section comparing Ruby
  2.4 vs. earlier — `grape.gemspec` requires Ruby >= 3.2, where
  `Fixnum` no longer exists.

Replaces an incorrect note about `requires + values + allow_blank`
(closes #2631) with a correct note covering `optional + values`. The
deleted note claimed `:allow_blank` defaults to `true` and that
absence of `:allow_blank` does not prevent `:state` from receiving
blank values — both wrong: `@allow_blank` defaults to `nil`, and
`requires` already rejects `nil` via the values validator. The real
quirk is `optional + values`: `ValuesValidator#validate_param!`
short-circuits on `nil` whenever the param is optional at the root
scope, so missing/`nil`/`""`-coerced-to-`nil` all pass. The new note
documents that and shows `allow_blank: false` as the way to opt out
while still allowing the key to be absent.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ericproulx

Copy link
Copy Markdown
Contributor

Superseded by #2700, which incorporates this deletion plus a corrected clarifying note about optional + values semantics. Thanks for catching the bug in the original docs!

@ericproulx ericproulx closed this May 8, 2026
ericproulx added a commit that referenced this pull request May 8, 2026
…l + values (#2700)

Spelling and grammar:
- "given on an `configuration`" → "based on a `configuration`"
- "Accept` head" → "Accept` header"
- "won't affects" → "won't affect"
- "with' blocks" → "with` blocks" (mismatched quote)
- `required :beer` / `:wine` / `:blah` → `requires` (the DSL method
  is `requires`; the samples as written would raise NoMethodError)
- "the the [grape-entity]" → "the [grape-entity]"

Accuracy:
- §"Accept-Version Header": the strict-mode sentence said the 406 was
  raised when no correct `Accept` header was supplied; the middleware
  checks `HTTP_ACCEPT_VERSION`, so corrected to "Accept-Version".
- "Rack::Etag" → "Rack::ETag" (correct class name).
- Removed the "Integer/Fixnum and Coercions" section comparing Ruby
  2.4 vs. earlier — `grape.gemspec` requires Ruby >= 3.2, where
  `Fixnum` no longer exists.

Replaces an incorrect note about `requires + values + allow_blank`
(closes #2631) with a correct note covering `optional + values`. The
deleted note claimed `:allow_blank` defaults to `true` and that
absence of `:allow_blank` does not prevent `:state` from receiving
blank values — both wrong: `@allow_blank` defaults to `nil`, and
`requires` already rejects `nil` via the values validator. The real
quirk is `optional + values`: `ValuesValidator#validate_param!`
short-circuits on `nil` whenever the param is optional at the root
scope, so missing/`nil`/`""`-coerced-to-`nil` all pass. The new note
documents that and shows `allow_blank: false` as the way to opt out
while still allowing the key to be absent.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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