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

Numeric is not a dry-types built-in type, fix coercion/validation #2271

Merged
merged 1 commit into from
Jul 22, 2022

Conversation

Vasfed
Copy link
Contributor

@Vasfed Vasfed commented Jul 21, 2022

In 1.3+ if someone had a Numeric type parameter which worked in <1.3 - any valid non-blank value will silently fail validation

params { optional :price, type: Numeric }

This happens because dry-types does not contain built-in type for DryTypes::SomeScope::Numeric and const_get by default searches ancestors, thus finding ::Numeric.
Runtime error undefined method `[]' for Numeric:Class is treated as a validation error and leads to unexpected behavior.

This PR adds mapping for Numeric that fixes it, also fallback to const_get(.., false) will make similar issues easier to debug.

@dblock
Copy link
Member

dblock commented Jul 21, 2022

Right on. The change to not lookup inherited types in const_get seems to break a bunch of other stuff, though.

 30) Grape::Endpoint#declared when the param is missing and include_missing=false sets nested objects to be nil
      Failure/Error: MAPPING.fetch(type) { scope.const_get(type.name, false) }
      NameError:
        uninitialized constant #<Module:0x00005598ebeb08a0>::Set

@Vasfed
Copy link
Contributor Author

Vasfed commented Jul 21, 2022

Yep, reverted the const_get change, it's not that important for the fix

}.freeze

def initialize(type, strict = false)
super

@type = type

# TODO: use const_get(.., false) to prevent vague errors on unknown types, but it breaks a lot of other stuff
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just revert this part of the change and open a new issue on GH please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@dblock dblock merged commit 413861f into ruby-grape:master Jul 22, 2022
@dblock
Copy link
Member

dblock commented Jul 22, 2022

Thanks! Merged.

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.

None yet

2 participants