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

validators and instance variables: weird behaviour #1592

Closed
loicginoux opened this issue Mar 10, 2017 · 4 comments
Closed

validators and instance variables: weird behaviour #1592

loicginoux opened this issue Mar 10, 2017 · 4 comments
Labels

Comments

@loicginoux
Copy link

Hi, I am posting this here in case someone already had this case. I think there is a bug
I am using:

  • grape (0.16.2)
  • ruby 1.9.3
  • rails 3.2.22.1

With the following code

class AlertIdValidation < Grape::Validations::Base
  include NewApi::Alerti::V3::Helpers::Authentication

  def validate_param!(attr_name, params)
    if !current_user.alerts.where(id: params[attr_name]).exists?
      raise Api::Exceptions::AlertNotExist, "alert #{params[attr_name]} does not exist"
    end
  end
end

class NewApi::Alerti::V3::Endpoints::Alerts < Grape::API
  helpers do
    def authenticate!
      raise NewApi::Alerti::V3::Exceptions::InvalidOrExpiredToken unless current_user
    end

    def current_user(req_params = nil)
      return @current_user unless @current_user.nil?
      @current_user = get_user(req_params) #not detailed, it will get user from the request access token 
    end
  end

  before do
    authenticate!
  end

  params do
    requires :alert_id, type: Integer, alert_id_validation: true
  end
  route_param :alert_id do
    # GET /alerts/:alert_id/
    get "/", jbuilder: 'alerts/show' do
      @alert = current_user.alerts.where(id: params[:alert_id]).first
    end
  end
end

I have a request GET /alerts/:alert_id

I call it once for a user A having an alert with id 1
curl -X GET "http://api.my_site.dev/v3/alerts/1?token=d1071245ed8dc94dfa336f629e1e23cf2671b21f82d5e41714c514401027"

It brings back the alert 1 correctly

I call the same endpoind for a user B having an alert with id 2
curl -X GET "http://api.my_site.dev/v3/alerts/2?token=6aa880cbc0413aaac411d260e158ec598102f6b3d0da6ce511570bcdd9e7"

the AlertIdValidation raises an error wrongly, the token is fine and the user B owns the alert 2.

After investigating, I have noticed that inside the validator, when calling the method current_user:
if !current_user.alerts.where(id: params[attr_name]).exists?

the current_user method will have a reference @current_user which is pointing to user A. everywhere else where I call current_user in the second call, I have the correct @current_user pointing to user B.

also implementing without using instance variable fix the issue when doing the second call, so I guess there is something that does not play well between instance variable and validators

def current_user(req_params = nil)
  get_user(req_params) #not detailed, it will get user from the request access token 
end

If this is a known bug or someone has experienced the same issue, let me know. this bug starts driving me crazy.
If you think it's worth investigating, I'll try to build a branch of the gem that isolate the issue.
Thank you

@dblock
Copy link
Member

dblock commented Mar 10, 2017

Good debugging! I feel that validator instance variables should definitely not carry across requests, so I would think this is a bug. It would be helpful to start with a spec, probably something that remembers @valid and fails on a second request.

@anakinj
Copy link
Contributor

anakinj commented Jun 22, 2017

I think this was fixed in the PR #1649 . Maybe you @loicginoux could verify with the latest master?

@loicginoux
Copy link
Author

ok, I'll try and let you know

@dblock
Copy link
Member

dblock commented Jun 22, 2017

I am going to call this fixed and close it, reopen if that's not the case.

@dblock dblock closed this as completed Jun 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants