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

Raising an error when query methods have blank arguments. #9258

Merged
merged 1 commit into from Feb 19, 2013

Conversation

Projects
None yet
5 participants
@wangjohn
Contributor

wangjohn commented Feb 12, 2013

Currently, whenever an activerecord query is included with a nil or nil-like object, no argument error is given. For instance:

Posts.limit().all = Posts.all

That relation is completely useless when there are blank-like arguments. For methods which this doesn't make sense for, I'm adding checks which will raise an ArgumentError.

@tadast

This comment has been minimized.

Show comment
Hide comment
@tadast

tadast Feb 12, 2013

Contributor

I would disagree. Often times you want this 'null pattern' behaviour when generating parameters dynamically and this change would force you to write unnecessary conditional statements.

Contributor

tadast commented Feb 12, 2013

I would disagree. Often times you want this 'null pattern' behaviour when generating parameters dynamically and this change would force you to write unnecessary conditional statements.

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Feb 19, 2013

Member

@tadast Could you show some code that's affected, before & after? Note that we're checking that any arguments are passed, not the argument isn't null or an empty string.

>> [''].blank?
=> false
Member

jeremy commented Feb 19, 2013

@tadast Could you show some code that's affected, before & after? Note that we're checking that any arguments are passed, not the argument isn't null or an empty string.

>> [''].blank?
=> false
@tadast

This comment has been minimized.

Show comment
Hide comment
@tadast

tadast Feb 19, 2013

Contributor

Right, I got confused by splat operator again 😊 Sorry for the FUD @wangjohn, @jeremy

Contributor

tadast commented Feb 19, 2013

Right, I got confused by splat operator again 😊 Sorry for the FUD @wangjohn, @jeremy

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 19, 2013

Member

Seems good. Could you add a changelog entry?

Member

rafaelfranca commented Feb 19, 2013

Seems good. Could you add a changelog entry?

@jeremy

This comment has been minimized.

Show comment
Hide comment
Member

jeremy commented Feb 19, 2013

@tadast 👍

@wangjohn

This comment has been minimized.

Show comment
Hide comment
@wangjohn

wangjohn Feb 19, 2013

Contributor

Changelog entry has been added.

Contributor

wangjohn commented Feb 19, 2013

Changelog entry has been added.

rafaelfranca added a commit that referenced this pull request Feb 19, 2013

Merge pull request #9258 from wangjohn/blank_argument_errors_in_arel
Raising an error when query methods have blank arguments.

@rafaelfranca rafaelfranca merged commit 8991083 into rails:master Feb 19, 2013

@tadast

This comment has been minimized.

Show comment
Hide comment
@tadast

tadast Feb 19, 2013

Contributor

Sorry, I don't want to cause any more trouble, but I'm thinking about it again... What if someone has a method like this:

def fancy_filter(options = {})
  related = []
  query = {}
  if options[:author_name]
    query[:author] = { name: options[:author_name] }
    related << :author
  end
  where(query).references(related)
end

would it not break things? In cases like this people would have to make sure to omit the #references call unless options[:author_name] is present.

Contributor

tadast commented Feb 19, 2013

Sorry, I don't want to cause any more trouble, but I'm thinking about it again... What if someone has a method like this:

def fancy_filter(options = {})
  related = []
  query = {}
  if options[:author_name]
    query[:author] = { name: options[:author_name] }
    related << :author
  end
  where(query).references(related)
end

would it not break things? In cases like this people would have to make sure to omit the #references call unless options[:author_name] is present.

@wangjohn

This comment has been minimized.

Show comment
Hide comment
@wangjohn

wangjohn Feb 19, 2013

Contributor

So where is handled separately. I only changed the functionality where blank arguments make strictly no sense. Where still returns self.

Contributor

wangjohn commented Feb 19, 2013

So where is handled separately. I only changed the functionality where blank arguments make strictly no sense. Where still returns self.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 19, 2013

Member

@wangjohn the @tadast's problem still occurs with references

Member

rafaelfranca commented Feb 19, 2013

@wangjohn the @tadast's problem still occurs with references

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Feb 19, 2013

Member

Again:

>> [[]].blank?
=> false

It'll only break if you do .references(*related), which would splat an empty array.

Member

jeremy commented Feb 19, 2013

Again:

>> [[]].blank?
=> false

It'll only break if you do .references(*related), which would splat an empty array.

@tadast

This comment has been minimized.

Show comment
Hide comment
@tadast

tadast Feb 19, 2013

Contributor

🙊

Contributor

tadast commented Feb 19, 2013

🙊

@wangjohn wangjohn deleted the wangjohn:blank_argument_errors_in_arel branch Feb 19, 2013

@richardkmichael

This comment has been minimized.

Show comment
Hide comment
@richardkmichael

richardkmichael Feb 20, 2013

I'm confused too; sorry!

Given the discussion above, the CHANGELOG.md description is misleading, I think. It reads: "are the arguments blank", not "are there arguments". I'm confused by the examples:

Post.limit()     # => raises error  
Post.include([]) # => raises error

The limit method has not been changed to call check_empty_arguments, so how will .limit() raise?
As mentioned in the discussion above, with .include([]), args.blank? == false, so how will it raise?

Are there tests for this I can read?

I'm confused too; sorry!

Given the discussion above, the CHANGELOG.md description is misleading, I think. It reads: "are the arguments blank", not "are there arguments". I'm confused by the examples:

Post.limit()     # => raises error  
Post.include([]) # => raises error

The limit method has not been changed to call check_empty_arguments, so how will .limit() raise?
As mentioned in the discussion above, with .include([]), args.blank? == false, so how will it raise?

Are there tests for this I can read?

@wangjohn

This comment has been minimized.

Show comment
Hide comment
@wangjohn

wangjohn Feb 20, 2013

Contributor

Sorry, the CHANGELOG is misleading and there is an error in it. I'm sending in a PR to fix this and add some more documentation and tests #9332.

Contributor

wangjohn commented Feb 20, 2013

Sorry, the CHANGELOG is misleading and there is an error in it. I'm sending in a PR to fix this and add some more documentation and tests #9332.

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