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

values proc validator runs at startup #801

Closed
bradrobertson opened this issue Nov 3, 2014 · 6 comments · Fixed by #854
Closed

values proc validator runs at startup #801

bradrobertson opened this issue Nov 3, 2014 · 6 comments · Fixed by #854

Comments

@bradrobertson
Copy link

We've got a param validator that looks like this:

param :x, type: String, values: ->(){ SomeModel.pluck(:code) }

Where SomeModel is an ActiveRecord model. For some reason, this values proc is run at startup of our application (ie the moment the values declaration is made). In our test suite when we run a rake db:schema:load, this proc runs, but since the schema obviously doesn't exist yet at this point, the startup fails with ERROR: relation "some_models" does not exist.

At this point I assumed that proc was running the one time and storing the value, which would make it useless for us, but I've verified that it does indeed get called at runtime during a request as we're able to get the validation values to change with new rows in the db.

So I have to ask then, is there a reason this block runs at the moment that it's defined? It appears as if the code is verifying that the validator params are themselves valid, but I don't know the Grape source all that well.

Is this a bug? An intentional decision?

@dblock
Copy link
Member

dblock commented Nov 3, 2014

I would say this is a bug, the whole point of a lambda here is that it executes at runtime. Would love a pull request to fix or at least a spec that reproduces the problem.

@dblock dblock closed this as completed in 152fcc5 Dec 16, 2014
dblock added a commit that referenced this issue Dec 16, 2014
Fix #801: Evaluate values proc lazily.
@dblock
Copy link
Member

dblock commented Dec 16, 2014

Fixed in #851.

@bradrobertson
Copy link
Author

I must be missing something here. All I can see is a variable name change and a test that ensures the block is called at runtime.

The original bug isn't that the block wasn't called at Runtime, it IS. It's just that the block is ALSO called on startup, and if we're referencing a model whose table didn't exist yet, we'd get an error.

It's more of a test issue than anything. Running rake db:test:prepare would throw that error for us as mentioned above.

@dblock
Copy link
Member

dblock commented Dec 16, 2014

Ok, I get it now. I thought my spec reproduced a problem, but I guess that was something between the computer and the chair. I re-fixed this in https://github.com/intridea/grape/pull/854/files. We lose the ability to determine the coerce type for example and to double check for impossible scenarios (like values is :x :y, but default is :z), but I think this is right especially that you can just remove the lambda and get the original behavior.

Please check my PR for me?

dblock added a commit to dblock/grape that referenced this issue Dec 16, 2014
dblock added a commit to dblock/grape that referenced this issue Dec 16, 2014
dblock added a commit to dblock/grape that referenced this issue Dec 16, 2014
dblock added a commit to dblock/grape that referenced this issue Dec 16, 2014
@dblock
Copy link
Member

dblock commented Dec 16, 2014

Also fixed the same problem with default: -> { }.

dblock added a commit that referenced this issue Dec 16, 2014
Fix #801: Only evaluate values proc lazily.
@bradrobertson
Copy link
Author

Cool thx!

I don't know the source that well so it's hard for me to comment, but I think it makes sense.

Will probably have a chance to test it out in the next week or 2

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

Successfully merging a pull request may close this issue.

2 participants