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

Grape 1.6.1 throughs an error on Grape::Validations::Types.group?() #2214

Closed
mobilutz opened this issue Dec 29, 2021 · 15 comments
Closed

Grape 1.6.1 throughs an error on Grape::Validations::Types.group?() #2214

mobilutz opened this issue Dec 29, 2021 · 15 comments

Comments

@mobilutz
Copy link

mobilutz commented Dec 29, 2021

The patch upgrade from 1.6.0 to 1.6.1 is breaking on the following line for me:
https://github.com/ruby-grape/grape/blob/v1.6.1/lib/grape/dsl/parameters.rb#L154

The full error is:

An error occurred while loading ./spec/riskmethods/jsonapi/validations_spec.rb.
Failure/Error:
  optional :hash, type: ::Hash, strict_hash: true do
    optional :nested
  end

NoMethodError:
  undefined method `group?' for Grape::Validations::Types:Module
# ./vendor/bundle/ruby/2.7.0/gems/grape-1.6.1/lib/grape/dsl/parameters.rb:154:in `optional'

Even though, def self.group?() is defined on Types here, I cannot call it through IRB or any other other means:
https://github.com/ruby-grape/grape/blob/v1.6.1/lib/grape/validations/types.rb#L101-L108

Looking through our specs, I also find this error:

NoMethodError:
  undefined method `multiple?' for Grape::Validations::Types:Module
# ./vendor/bundle/ruby/2.7.0/gems/grape-1.6.1/lib/grape/validations/params_scope.rb:359:in `infer_coercion'

The test setup is as follows:

  1. create a RSpec support app with some endpoints and optional params e.g.
       params do
         optional :hash, type: ::Hash, strict_hash: true do
           optional :nested
         end
       end
    
       get :strict_hash do
         ...
       end
    
  2. call that endpoint in specs
  3. see error from grape 😦

PS: In my opinion, the patch update has way to many changes in it to be honest. But I also know how hard maintaining an open-source product is, so please don't take this as a complaint!

@dm1try
Copy link
Member

dm1try commented Dec 29, 2021

@mobilutz thanks for the report!
@ericproulx could you take a look please? it might be related to the latest changes was made for autoloading.

@dblock dblock added the bug? label Dec 29, 2021
@dblock
Copy link
Member

dblock commented Dec 29, 2021

@mobilutz appreciate if you could please turn your repro into a failing spec PR

@dm1try
Copy link
Member

dm1try commented Dec 29, 2021

@dblock I've discovered that it's easy enough, just remove eager_load from spec_helper to reproduce the problem

@dm1try
Copy link
Member

dm1try commented Dec 29, 2021

it might be worth removing it at all if we have an integration spec for eager_load => https://github.com/ruby-grape/grape/blob/master/spec/integration/eager_load/eager_load_spec.rb

@dm1try
Copy link
Member

dm1try commented Dec 29, 2021

@mobilutz meanwhile, as a workaround, you could put

require 'grape/eager_load'
Grape::Validations.eager_load!
Grape::Validations::Validators.eager_load!

in the spec helper

but I'm hoping for a quick fix release :)

@seb-sykio
Copy link

seb-sykio commented Dec 29, 2021

I am not sure if the problem is similar : I have this error after update from 1.6 to 1.6.1

undefined method multiple?' for Grape::Validations::Types:Module .rvm/gems/ruby-3.0.3/gems/grape-1.6.1/lib/grape/validations/params_scope.rb:359:in infer_coercion'
.rvm/gems/ruby-3.0.3/gems/grape-1.6.1/lib/grape/validations/params_scope.rb:274:in validates' .rvm/gems/ruby-3.0.3/gems/grape-1.6.1/lib/grape/validations/params_scope.rb:195:in validate_attributes'
.rvm/gems/ruby-3.0.3/gems/grape-1.6.1/lib/grape/dsl/parameters.rb:160:in optional' ./app/controllers/api/v1/addresses.rb:12:in block (2 levels) in class:Addresses'

the code is
` resource :addresses do

    desc "List all addresses"
    params do
      optional :market_id, type: String, desc: "ID of the market"
    end`

@dm1try
Copy link
Member

dm1try commented Dec 29, 2021

@seb-sykio yeah, it looks like the same problem.

@dm1try
Copy link
Member

dm1try commented Dec 29, 2021

I propose to revert the latest changes that were made to the autoloading in #2207 #2209 and release again.

At least, the changes for validators will not work as expected because the grape validators are loaded differently(we do not use a class name but use some :symbol in DSL) so we cannot use a standard autoload interface.
not sure about the part with Types autoloading but it looks like it's a problem too^

@dm1try dm1try added confirmed bug and removed bug? labels Dec 29, 2021
@dblock
Copy link
Member

dblock commented Dec 29, 2021

I propose to revert the latest changes that were made to the autoloading in #2207 #2209 and release again.

At least, the changes for validators will not work as expected because the grape validators are loaded differently(we do not use a class name but use some :symbol in DSL) so we cannot use a standard autoload interface. not sure about the part with Types autoloading but it looks like it's a problem too^

That works for me.

@mobilutz
Copy link
Author

Thanks for working on this so fast.
I will test out, if the proposed idea helps us. But I also think it is better to not have a workaround.

If you need any help with a test setup, I can see to create an app for you guys.

Cheers

@ericproulx
Copy link
Contributor

ericproulx commented Dec 29, 2021 via email

@dm1try
Copy link
Member

dm1try commented Dec 30, 2021

@ericproulx it's a holiday time and I think we don't need any rush here, so I'm going to release the fix with reverted #2207 #2209. If you come with some solution then it will be released the next time :)

@dm1try
Copy link
Member

dm1try commented Dec 30, 2021

1.6.2 has been released, try updating 🤞🏻

@dm1try dm1try closed this as completed Dec 30, 2021
@sharq1
Copy link

sharq1 commented Dec 30, 2021

Upgrading to 1.6.2 fixed the related issue #2218 - thank you!

@mobilutz
Copy link
Author

Thanks @dm1try

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants