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 invalid date error message #5250

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@ahorek
Copy link
Contributor

ahorek commented Jul 15, 2018

master NoMethodError (undefined method `gsub!' for 4:Integer)
->
patch TypeError: no implicit conversion of Integer into String

ruby/spec#610

@kares

This comment has been minimized.

Copy link
Member

kares commented Jul 16, 2018

shouldnt this go into parse instead of the low level _parse? did not check MRI ...

@ahorek ahorek force-pushed the ahorek:fix_invalid_date branch 2 times, most recently from 2766bd5 to 2de21bb Jul 16, 2018

@ahorek ahorek changed the title fix invalid date error message WIP: fix invalid date error message Jul 16, 2018

@ahorek ahorek force-pushed the ahorek:fix_invalid_date branch 2 times, most recently from e59607e to ecdd3cc Jul 16, 2018

@ahorek

This comment has been minimized.

Copy link
Contributor Author

ahorek commented Jul 16, 2018

I think we should always call .to_str because for instance ActiveSupport::SafeBuffer doesn't work right with regex see rails/rails#1555

@ahorek ahorek changed the title WIP: fix invalid date error message fix invalid date error message Jul 16, 2018

@kares

This comment has been minimized.

Copy link
Member

kares commented Jul 16, 2018

right, makes sense but we strive for MRI compat first, than dealing with whatever (Rails) monkey-patches.
Date._parse seems to do the to_str conversion and that is what I was after here to check :

2.5.1 :010 > str = ActiveSupport::SafeBuffer.new '2011-01-01'
 => "2011-01-01" 
2.5.1 :011 > Date.parse '2011-01-01'
 => Sat, 01 Jan 2011 
2.5.1 :012 > Date.parse str
 => Sat, 01 Jan 2011 
2.5.1 :013 > Date._parse str
 => {:year=>2011, :mon=>1, :mday=>1}
@kares

This comment has been minimized.

Copy link
Member

kares commented Jul 16, 2018

oh, actually to_str was already there in _parse this adds it to other places
but introduces yet another ugly internal method on Date ;( maybe it can be avoided?
esp. since this now "broke" _parse from behaving (doing the to_str coercion) as in MRI

@ahorek

This comment has been minimized.

Copy link
Contributor Author

ahorek commented Jul 16, 2018

right, I will fix it and add more specs

@ahorek ahorek changed the title fix invalid date error message WIP: fix invalid date error message Jul 16, 2018

@ahorek ahorek force-pushed the ahorek:fix_invalid_date branch 2 times, most recently from cb1127e to b432c91 Jul 16, 2018

@ahorek

This comment has been minimized.

Copy link
Contributor Author

ahorek commented Jul 16, 2018

Date._parse ActiveSupport::SafeBuffer.new '2011-01-01'

doesn't work without the explicit conversion into a String.

is it safe to convert it? MRI uses StringValue()
https://github.com/ruby/ruby/blob/576b245ffae913aec11ae321fe7ddc9c2b688f67/ext/date/date_core.c#L4297

@ahorek ahorek changed the title WIP: fix invalid date error message fix invalid date error message Jul 16, 2018

@@ -482,6 +482,9 @@ def self.strptime(str='-4712-01-01', fmt='%F', sg=ITALY)
#
# +sg+ specifies the Day of Calendar Reform.
def self.parse(str='-4712-01-01', comp=true, sg=ITALY)
unless str.kind_of?(::String) || str.respond_to?(:to_str)

This comment has been minimized.

Copy link
@kares

kares Jul 16, 2018

Member

could we just move this to _parse ?
and than it wouldn't need to exist on 3 places + MRI seems to validate there as well:

2.5.1 :003 > Date._parse 111
Traceback (most recent call last):
        3: from /opt/local/rvm/rubies/ruby-2.5.1/bin/irb:11:in `<main>'
        2: from (irb):3
        1: from (irb):3:in `_parse'
TypeError (no implicit conversion of Integer into String)

... and since this would eventually be moved to native like they did it will be simpler for anyone ending up doing it.

pavel

@ahorek ahorek force-pushed the ahorek:fix_invalid_date branch from b432c91 to 7bc62d5 Jul 16, 2018

@@ -577,7 +577,11 @@ def self._parse_ddd(str, e) # :nodoc:
def self._parse(str, comp=true)
# Newer MRI version (written in C converts non-strings to strings
# and also has other checks like all ascii.
str = str.to_str if !str.kind_of?(::String) && str.respond_to?(:to_str)
if str.kind_of?(::String) || str.respond_to?(:to_str)
str = str.to_str if str.respond_to?(:to_str)

This comment has been minimized.

Copy link
@ahorek

ahorek Jul 16, 2018

Author Contributor

the second check is for this case which seems very unlikely to me. Do you think it's worth to write a test for it?

class Sub < String
  undef_method :to_str
end
Date._parse Sub.new('20160505')

This comment has been minimized.

Copy link
@kares

kares Jul 16, 2018

Member

not really - there wasn't a second check.

@ahorek

This comment has been minimized.

Copy link
Contributor Author

ahorek commented Jul 16, 2018

_parse is used a lot internally on places where we know that the data type is already ok, but the real impact is probably not measurable

@kares

This comment has been minimized.

Copy link
Member

kares commented Jul 16, 2018

_parse is used a lot internally on places where we know that the data type is already ok,

ok.
still, makes me wonder str.to_str should raise proper TypeError, maybe _parse is worth a C read
... feels slightly "wrong" that we have to mess around String checks - to_str should do the work for us

@ahorek

This comment has been minimized.

Copy link
Contributor Author

ahorek commented Jul 16, 2018

but #to_str isn't defined on Integer

other methods like #_iso8601 fails on "=~" with the same error (TypeError) which is fine, but #_parse calls #gsub! first (NoMethodError) and it should be also TypeError

jruby-9.2.0.0 :004 > Date.iso8601 1
Traceback (most recent call last):
        9: from /home/ahorek/.rvm/rubies/jruby-9.2.0.0/bin/irb:13:in `<main>'
        8: from org/jruby/RubyKernel.java:1180:in `catch'
        7: from org/jruby/RubyKernel.java:1180:in `catch'
        6: from org/jruby/RubyKernel.java:1418:in `loop'
        5: from org/jruby/RubyKernel.java:1037:in `eval'
        4: from (irb):4:in `<eval>'
        3: from /home/ahorek/.rvm/rubies/jruby-9.2.0.0/lib/ruby/stdlib/date.rb:492:in `iso8601'
        2: from /home/ahorek/.rvm/rubies/jruby-9.2.0.0/lib/ruby/stdlib/date/format.rb:660:in `_iso8601'
        1: from org/jruby/RubyRegexp.java:1093:in `=~'
TypeError (no implicit conversion of Integer into String)
jruby-9.2.0.0 :002 > Date.parse 1
Traceback (most recent call last):
        8: from /home/ahorek/.rvm/rubies/jruby-9.2.0.0/bin/irb:13:in `<main>'
        7: from org/jruby/RubyKernel.java:1180:in `catch'
        6: from org/jruby/RubyKernel.java:1180:in `catch'
        5: from org/jruby/RubyKernel.java:1418:in `loop'
        4: from org/jruby/RubyKernel.java:1037:in `eval'
        3: from (irb):2:in `<eval>'
        2: from /home/ahorek/.rvm/rubies/jruby-9.2.0.0/lib/ruby/stdlib/date.rb:487:in `parse'
        1: from /home/ahorek/.rvm/rubies/jruby-9.2.0.0/lib/ruby/stdlib/date/format.rb:587:in `_parse'
NoMethodError (undefined method `gsub!' for 1:Integer)

@kares kares added this to the JRuby 9.2.1.0 milestone Jul 18, 2018

kares added a commit to kares/jruby that referenced this pull request Jul 18, 2018

[refactor] respond checking + dry-out constant definition
also added some in house tests ... a follow up on jrubyGH-5250

kares added a commit that referenced this pull request Jul 18, 2018

[refactor] respond checking + dry-out constant definition
also added some in house tests ... a follow up on GH-5250
@kares

This comment has been minimized.

Copy link
Member

kares commented Jul 18, 2018

pushed to master as a6dad4b

@kares kares closed this Jul 18, 2018

@ahorek

This comment has been minimized.

Copy link
Contributor Author

ahorek commented Jul 18, 2018

thanks @kares
but after a6dad4b this case doesn't work anymore

require 'active_support'
require 'date'

str = ActiveSupport::SafeBuffer.new('2011-05-07')

Date.parse str

mri
=> #<Date: 2011-05-07 ((2455689j,0s,0n),+0s,2299161j)>
jruby
        7: from org/jruby/RubyKernel.java:1178:in `catch'
        6: from org/jruby/RubyKernel.java:1178:in `catch'
        5: from org/jruby/RubyKernel.java:1412:in `loop'
        4: from org/jruby/RubyKernel.java:1040:in `eval'
        3: from (irb):10:in `evaluate'
        2: from /jruby/lib/ruby/stdlib/date.rb:486:in `parse'
        1: from /jruby/lib/ruby/stdlib/date.rb:444:in `new_by_frags'
[1mArgumentError ([4minvalid date[0;1m)[m

Date._parse str

mri
=> {:year=>2011, :mon=>5, :mday=>7}
jruby
=> {:mon=>0}

I'm not sure if it should work or not, see rails/rails#1555 . According to the rails devs we should never use regex directly on a SafeBuffer... but MRI handles this case, could you recheck?

@kares

This comment has been minimized.

Copy link
Member

kares commented Jul 18, 2018

guess, they're pushing it to the limits with their SafeBuffer ... anyway seems like $1 globals are to blame.
have actually wondered if we can avoid those or really need to fix it to work for sub-strings properly ...

@ahorek

This comment has been minimized.

Copy link
Contributor Author

ahorek commented Jul 18, 2018

There's no need, I don't think it could cause a real problem and even if so, it's easily fixable by calling .to_str explicitly. Thanks for your comments.

@kares

This comment has been minimized.

Copy link
Member

kares commented Jul 18, 2018

okay, I can confirm its actually smt that should "not" have worked - JRuby stores $ 'globals' in current frame.
with the String sub-class there's going to be 2 frames for SafeBuffer#sub due how its setup thus $1 won't reflect a sub operation on a frame before. altough we could play a trick here to bypass the sub-class easily.
... its likely legit since the format.rb should be native -> refactored not relying on $ local state anyways

kares added a commit to kares/jruby that referenced this pull request Jul 18, 2018

kares added a commit to kares/jruby that referenced this pull request Jul 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.