fix invalid date error message by ahorek · Pull Request #5250 · jruby/jruby

ahorek · 2018-07-15T12:04:50Z

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

ruby/spec#610

kares · 2018-07-16T06:20:12Z

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

ahorek · 2018-07-16T11:08:05Z

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

kares · 2018-07-16T11:24:37Z

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 · 2018-07-16T11:28:26Z

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 · 2018-07-16T11:30:22Z

right, I will fix it and add more specs

ahorek · 2018-07-16T14:59:55Z

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

kares · 2018-07-16T15:08:17Z

lib/ruby/stdlib/date.rb

  #
  # +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)


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.

ahorek · 2018-07-16T15:38:55Z

lib/ruby/stdlib/date/format.rb

    # 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)


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')

not really - there wasn't a second check.

ahorek · 2018-07-16T15:40:20Z

_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 · 2018-07-16T16:12:40Z

_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 · 2018-07-16T16:23:56Z

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)

also added some in house tests ... a follow up on jrubyGH-5250

also added some in house tests ... a follow up on GH-5250

kares · 2018-07-18T06:58:26Z

pushed to master as a6dad4b

ahorek · 2018-07-18T08:53:47Z

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 · 2018-07-18T10:50:16Z

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 · 2018-07-18T11:12:52Z

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 · 2018-07-18T12:26:58Z

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

... relates to the work performed at jrubyGH-5250

ahorek force-pushed the fix_invalid_date branch 2 times, most recently from 2766bd5 to 2de21bb Compare July 16, 2018 08:49

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

ahorek force-pushed the fix_invalid_date branch 2 times, most recently from e59607e to ecdd3cc Compare July 16, 2018 10:55

ahorek mentioned this pull request Jul 16, 2018

can't use Date.iso8601 with a string subclass #5252

Closed

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

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

ahorek force-pushed the fix_invalid_date branch 2 times, most recently from cb1127e to b432c91 Compare July 16, 2018 12:38

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

kares reviewed Jul 16, 2018

View reviewed changes

fix invalid date error message

7bc62d5

ahorek force-pushed the fix_invalid_date branch from b432c91 to 7bc62d5 Compare July 16, 2018 15:30

ahorek commented Jul 16, 2018

View reviewed changes

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

94ea3b5

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

1572660

also added some in house tests ... a follow up on GH-5250

kares closed this Jul 18, 2018

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

[fix] improve compat for Date._parse to work with AS::SafeBuffer

0af3e14

... relates to the work performed at jrubyGH-5250

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

[fix] improve compat for Date._parse to work with AS::SafeBuffer

a943b01

... relates to the work performed at jrubyGH-5250

Uh oh!

Conversation

ahorek commented Jul 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kares commented Jul 16, 2018

Uh oh!

ahorek commented Jul 16, 2018

Uh oh!

kares commented Jul 16, 2018

Uh oh!

kares commented Jul 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahorek commented Jul 16, 2018

Uh oh!

ahorek commented Jul 16, 2018

Uh oh!

kares Jul 16, 2018

Choose a reason for hiding this comment

Uh oh!

ahorek Jul 16, 2018

Choose a reason for hiding this comment

Uh oh!

kares Jul 16, 2018

Choose a reason for hiding this comment

Uh oh!

ahorek commented Jul 16, 2018

Uh oh!

kares commented Jul 16, 2018

Uh oh!

ahorek commented Jul 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kares commented Jul 18, 2018

Uh oh!

ahorek commented Jul 18, 2018

Uh oh!

kares commented Jul 18, 2018

Uh oh!

ahorek commented Jul 18, 2018

Uh oh!

kares commented Jul 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ahorek commented Jul 15, 2018 •

edited

Loading

kares commented Jul 16, 2018 •

edited

Loading

ahorek commented Jul 16, 2018 •

edited

Loading