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

Fix - `to_s(:db)` in numeric range #22622

Merged
merged 1 commit into from Dec 29, 2015
Merged

Conversation

@akshay-vishnoi
Copy link
Contributor

akshay-vishnoi commented Dec 17, 2015

When to_s(:db) is applied on numeric range

Previously,
to_s(:db) was called on both the limits but Fixnum#to_s expects base in the argument and tries to convert :db into integer and raises exception TypeError: no implicit conversion of Symbol into Integer.

Solution - Using to_s after rescue if to_s(:db) is not available on limits of range.

@rails-bot

This comment has been minimized.

Copy link

rails-bot commented Dec 17, 2015

r? @eileencodes

(@rails-bot has picked a reviewer for you, use r? to override)

@akshay-vishnoi

This comment has been minimized.

Copy link
Contributor Author

akshay-vishnoi commented Dec 18, 2015

r? @kaspth

@rails-bot rails-bot assigned kaspth and unassigned eileencodes Dec 18, 2015
@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Dec 18, 2015

Is this caused by the change that removed alias_method from to_s?

@akshay-vishnoi

This comment has been minimized.

Copy link
Contributor Author

akshay-vishnoi commented Dec 18, 2015

Ohh... Got it. Here is the source - f61dd4f

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Dec 18, 2015

Ok, so we should use that code as inspiration to fix this. rescue is not an option.

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Dec 18, 2015
@akshay-vishnoi

This comment has been minimized.

Copy link
Contributor Author

akshay-vishnoi commented Dec 18, 2015

Yup, you are right. I will dig into this direction.

@akshay-vishnoi akshay-vishnoi force-pushed the akshay-vishnoi:range_format_fix branch Dec 19, 2015
@akshay-vishnoi akshay-vishnoi changed the title Fix - rescue `to_s(:db)` in numeric range Fix - `to_s(:db)` in numeric range Dec 19, 2015
@akshay-vishnoi

This comment has been minimized.

Copy link
Contributor Author

akshay-vishnoi commented Dec 19, 2015

PR updated. Fixed the original to_s.

@kaspth
kaspth reviewed Dec 20, 2015
View changes
activesupport/lib/active_support/core_ext/numeric/conversions.rb Outdated
end)
else
super
end

This comment has been minimized.

Copy link
@kaspth

kaspth Dec 20, 2015

Member

This module is only prepended into Fixnum, Bignum, Float and BigDecimal, so I think we can simplify the code to:

if is_a?(Float) || format.is_a?(Symbol)
  super()
else
  super
end

Though to be honest, the is_a? checks suggests that this might not be the right abstraction level. In other words this module tries to be independent of it's includers, but can't. @rafaelfranca (since you commented on this), do you have thoughts on what to do?

@akshay-vishnoi akshay-vishnoi force-pushed the akshay-vishnoi:range_format_fix branch Dec 21, 2015
@akshay-vishnoi

This comment has been minimized.

Copy link
Contributor Author

akshay-vishnoi commented Dec 21, 2015

@kaspth - PR updated.

@akshay-vishnoi akshay-vishnoi force-pushed the akshay-vishnoi:range_format_fix branch Dec 21, 2015
@sgrif

This comment has been minimized.

Copy link
Contributor

sgrif commented Dec 21, 2015

I'd rather we just remove to_s(:db). It's usefulness is extremely marginal since you can pass a range to where, and this function will always be unsafe as you cannot properly escape a string without a database connection. Improper range quoting has been the cause of SQL injection vulnerabilities in the past, and anyone using Range#to_s(:db) is likely exposing themselves to vulnerabilities as well.

@kaspth

This comment has been minimized.

Copy link
Member

kaspth commented Dec 21, 2015

I don't have a strong opinion about keeping it. Could go with a deprecation just as well.

@akshay-vishnoi

This comment has been minimized.

Copy link
Contributor Author

akshay-vishnoi commented Dec 21, 2015

I agree with @sgrif and we can go for deprecation, but we need to fix it as currently it is not behaving how it is expected to be.

@rafaelfranca rafaelfranca modified the milestones: 5.0.0, 5.0.0 [temp] Dec 22, 2015
@akshay-vishnoi akshay-vishnoi force-pushed the akshay-vishnoi:range_format_fix branch Dec 22, 2015
Akshay Vishnoi
@akshay-vishnoi akshay-vishnoi force-pushed the akshay-vishnoi:range_format_fix branch to 007bb11 Dec 29, 2015
@akshay-vishnoi

This comment has been minimized.

Copy link
Contributor Author

akshay-vishnoi commented Dec 29, 2015

PR rebased. Ready for review.

kaspth added a commit that referenced this pull request Dec 29, 2015
Fix - `to_s(:db)` in numeric range
@kaspth kaspth merged commit 929c615 into rails:master Dec 29, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@akshay-vishnoi

This comment has been minimized.

Copy link
Contributor Author

akshay-vishnoi commented Dec 30, 2015

Should we go for deprecation as well?

@kaspth

This comment has been minimized.

Copy link
Member

kaspth commented Dec 30, 2015

I'll leave that up to @sgrif

@sgrif

This comment has been minimized.

Copy link
Contributor

sgrif commented Dec 31, 2015

Yes, let's go ahead and deprecate it before 5.0 goes out. Care to open a PR?

On Wed, Dec 30, 2015 at 1:01 AM Kasper Timm Hansen notifications@github.com
wrote:

I'll leave that up to @sgrif https://github.com/sgrif


Reply to this email directly or view it on GitHub
#22622 (comment).

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

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.