Boolean parser blows up on a Fixnum. #12769

Merged
merged 1 commit into from Feb 1, 2014

Conversation

Projects
None yet
3 participants
@birkirb
Contributor

birkirb commented Nov 5, 2013

PARSING['boolean'].call(1) would fail (no strip on Fixnum).
Converting to string before strip should do it.
Looked for tests, but couldn't find them.
If there are some perhaps someone could point me in the right direction?

@robin850

This comment has been minimized.

Show comment Hide comment
@robin850

robin850 Nov 10, 2013

Member

Hello,

thanks for your patch! Actually, it will fail for all values which aren't strings (as far as I can understand).

>> foo = Proc.new { |boolean| %w(1 true).include?(boolean.strip) }
=> #<Proc:0x00000000fa0b40@(pry):1>
>> foo.call(1)
NoMethodError: undefined method `strip' for 1:Fixnum
from (pry):1:in `block in __pry__'
>> foo.call(true)
NoMethodError: undefined method `strip' for true:TrueClass
from (pry):1:in `block in __pry__'

It would be very nice if you could add an entry at the top of the CHANGELOG. Maybe you could create a test in activesupport/test/xml_mini_test.rb like this :

class ParsingTest < ActiveSupport::TestCase
  def setup
    @parsing = ActiveSupport::XmlMini::PARSING
  end

  def test_boolean_with_fixnum
    assert @parsing["boolean"].call(1)
    assert @parsing["boolean"].call(true)

    assert_not @parsing["boolean"].call(false)
  end
end

What do you think ?

Member

robin850 commented Nov 10, 2013

Hello,

thanks for your patch! Actually, it will fail for all values which aren't strings (as far as I can understand).

>> foo = Proc.new { |boolean| %w(1 true).include?(boolean.strip) }
=> #<Proc:0x00000000fa0b40@(pry):1>
>> foo.call(1)
NoMethodError: undefined method `strip' for 1:Fixnum
from (pry):1:in `block in __pry__'
>> foo.call(true)
NoMethodError: undefined method `strip' for true:TrueClass
from (pry):1:in `block in __pry__'

It would be very nice if you could add an entry at the top of the CHANGELOG. Maybe you could create a test in activesupport/test/xml_mini_test.rb like this :

class ParsingTest < ActiveSupport::TestCase
  def setup
    @parsing = ActiveSupport::XmlMini::PARSING
  end

  def test_boolean_with_fixnum
    assert @parsing["boolean"].call(1)
    assert @parsing["boolean"].call(true)

    assert_not @parsing["boolean"].call(false)
  end
end

What do you think ?

@birkirb

This comment has been minimized.

Show comment Hide comment
@birkirb

birkirb Nov 11, 2013

Contributor

Hi @robin850

Sounds good. Added tests for most of the parsers.
Found a couple of extra bugs and fixed those (symbol would also fail for all non strings).
Not sure if all the behavior is as it should be but at least it's an improvement.
Added a line to the changelog.
Hope that's all good!

Contributor

birkirb commented Nov 11, 2013

Hi @robin850

Sounds good. Added tests for most of the parsers.
Found a couple of extra bugs and fixed those (symbol would also fail for all non strings).
Not sure if all the behavior is as it should be but at least it's an improvement.
Added a line to the changelog.
Hope that's all good!

@robin850

View changes

activesupport/CHANGELOG.md
@@ -1,3 +1,7 @@
+* Fix parsing bugs in `XmlMini`. Add tests for parsers.

This comment has been minimized.

Show comment Hide comment
@robin850

robin850 Nov 12, 2013

Member

Can you improve this a bit please ? I think that we should mention that errors were raised when passing for instance fixnums or symbols for instance. Actually, the test part is not needed ; this is common to add tests when fixing a bug or adding a feature.

@robin850

robin850 Nov 12, 2013

Member

Can you improve this a bit please ? I think that we should mention that errors were raised when passing for instance fixnums or symbols for instance. Actually, the test part is not needed ; this is common to add tests when fixing a bug or adding a feature.

This comment has been minimized.

Show comment Hide comment
@birkirb

birkirb Nov 13, 2013

Contributor

Will do.

@birkirb

birkirb Nov 13, 2013

Contributor

Will do.

@robin850

View changes

activesupport/lib/active_support/xml_mini.rb
"date" => Proc.new { |date| ::Date.parse(date) },
- "datetime" => Proc.new { |time| Time.xmlschema(time).utc rescue ::DateTime.parse(time).utc },

This comment has been minimized.

Show comment Hide comment
@robin850

robin850 Nov 12, 2013

Member

Could you elaborate on this change please ?

@robin850

robin850 Nov 12, 2013

Member

Could you elaborate on this change please ?

This comment has been minimized.

Show comment Hide comment
@birkirb

birkirb Nov 13, 2013

Contributor

No .utc method in DateTime. (at least on 1.9.3 it blew up)

  • Although now I can't replicate. All add another test for the utc bit.
  • Added a test, ultimately it seems to be irrelevant.
@birkirb

birkirb Nov 13, 2013

Contributor

No .utc method in DateTime. (at least on 1.9.3 it blew up)

  • Although now I can't replicate. All add another test for the utc bit.
  • Added a test, ultimately it seems to be irrelevant.

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Dec 19, 2013

Member

I think we should bring back the utc. If it doesn't have the method it is because it is missing the require for activesupport/lib/active_support/core_ext/date_time/calculations.rb

@rafaelfranca

rafaelfranca Dec 19, 2013

Member

I think we should bring back the utc. If it doesn't have the method it is because it is missing the require for activesupport/lib/active_support/core_ext/date_time/calculations.rb

This comment has been minimized.

Show comment Hide comment
@birkirb

birkirb Dec 20, 2013

Contributor

Is there a test I could write that would fail without the utc?

@birkirb

birkirb Dec 20, 2013

Contributor

Is there a test I could write that would fail without the utc?

This comment has been minimized.

Show comment Hide comment
@robin850

robin850 Dec 20, 2013

Member

As you mentioned, on 1.9.3 it blew up so the test will be marked as errored. In my opinion, it's not needed to test such behavior. What do you guys think ?

@robin850

robin850 Dec 20, 2013

Member

As you mentioned, on 1.9.3 it blew up so the test will be marked as errored. In my opinion, it's not needed to test such behavior. What do you guys think ?

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Dec 20, 2013

Member

That is my point. It only blew up because the require is missing.

@rafaelfranca

rafaelfranca Dec 20, 2013

Member

That is my point. It only blew up because the require is missing.

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Dec 20, 2013

Member

@birkirb what we can try to do is use some input that raise an exception when Time.xmlschema(time) is called and assert if the right datetime was created.

@rafaelfranca

rafaelfranca Dec 20, 2013

Member

@birkirb what we can try to do is use some input that raise an exception when Time.xmlschema(time) is called and assert if the right datetime was created.

This comment has been minimized.

Show comment Hide comment
@birkirb

birkirb Dec 20, 2013

Contributor

Any ideas for input?

I though I had it covered with this (they are malformed and will hit the rescue):

assert_equal DateTime.new(2013,11,12,02,11), parser.call("2013-11-12T02:11Z")
assert_equal DateTime.new(2013,11,12,02,11), parser.call("2013-11-12T11:11+9")

And as such I think the code works as it should.

@birkirb

birkirb Dec 20, 2013

Contributor

Any ideas for input?

I though I had it covered with this (they are malformed and will hit the rescue):

assert_equal DateTime.new(2013,11,12,02,11), parser.call("2013-11-12T02:11Z")
assert_equal DateTime.new(2013,11,12,02,11), parser.call("2013-11-12T11:11+9")

And as such I think the code works as it should.

This comment has been minimized.

Show comment Hide comment
@birkirb

birkirb Dec 23, 2013

Contributor

Added back the utc... are we ok then?

@birkirb

birkirb Dec 23, 2013

Contributor

Added back the utc... are we ok then?

@robin850

This comment has been minimized.

Show comment Hide comment
@robin850

robin850 Nov 12, 2013

Member

The Base 64 related test seems to be broken on Ruby 2.0, could you investigate please ? Also please squash your commits. Thank you!

Member

robin850 commented Nov 12, 2013

The Base 64 related test seems to be broken on Ruby 2.0, could you investigate please ? Also please squash your commits. Thank you!

@birkirb

This comment has been minimized.

Show comment Hide comment
@birkirb

birkirb Nov 13, 2013

Contributor

@robin850 Updated.
I squashed the commit to another branch, reset my master to that and force pushed. Looks fine on my local git (and here: https://github.com/birkirb/rails/commits/master) but github seems to still have the other commits in this pull request. Not sure what to do about that. Took a while to update.

Contributor

birkirb commented Nov 13, 2013

@robin850 Updated.
I squashed the commit to another branch, reset my master to that and force pushed. Looks fine on my local git (and here: https://github.com/birkirb/rails/commits/master) but github seems to still have the other commits in this pull request. Not sure what to do about that. Took a while to update.

@birkirb

This comment has been minimized.

Show comment Hide comment
@birkirb

birkirb Nov 15, 2013

Contributor

@robin850
Current test fail is from the master branch, should I base off something else?

Contributor

birkirb commented Nov 15, 2013

@robin850
Current test fail is from the master branch, should I base off something else?

@robin850

View changes

activesupport/CHANGELOG.md
@@ -1,3 +1,9 @@
+* Fix parsing bugs in `XmlMini`: symbol parsing would fail for non strings,
+ decimal parsing would fail due to missing require and boolean would fail
+ for non strings (i.e. integers 1|0).

This comment has been minimized.

Show comment Hide comment
@robin850

robin850 Nov 16, 2013

Member

What do you think about rewording it to :

Fix parsing bugs in `XmlMini`

Symbols or boolean parsing would raise an error for non string values (e.g. 
integers). Decimal parsing would fail due to a missing requirement.
@robin850

robin850 Nov 16, 2013

Member

What do you think about rewording it to :

Fix parsing bugs in `XmlMini`

Symbols or boolean parsing would raise an error for non string values (e.g. 
integers). Decimal parsing would fail due to a missing requirement.
@robin850

This comment has been minimized.

Show comment Hide comment
@robin850

robin850 Nov 16, 2013

Member

@birkirb : This was due to an error on master but now your build is green. Could you just please look at the comment I added about the changelog ? Also, could you rebase ? I think then it will be good ; I will ask a core team member to review this. Thank you! ❤️

Member

robin850 commented Nov 16, 2013

@birkirb : This was due to an error on master but now your build is green. Could you just please look at the comment I added about the changelog ? Also, could you rebase ? I think then it will be good ; I will ask a core team member to review this. Thank you! ❤️

@robin850

View changes

activesupport/test/xml_mini_test.rb
+
+ parser = @parsing['base64Binary']
+ assert_equal expected_base64.gsub(/\n/," ").strip, parser.call(base64)
+ assert_nothing_raised { parser.call("NON BASE64 INPUT") }

This comment has been minimized.

Show comment Hide comment
@robin850

robin850 Nov 16, 2013

Member

Ugh, I forgot this but IIRC we are deprecating the use of assert_nothing_raised, the body of the passed block is enough since the test would be marked as errored if any error is raised.

@robin850

robin850 Nov 16, 2013

Member

Ugh, I forgot this but IIRC we are deprecating the use of assert_nothing_raised, the body of the passed block is enough since the test would be marked as errored if any error is raised.

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Nov 16, 2013

Member

👍

This comment has been minimized.

Show comment Hide comment
@birkirb

birkirb Nov 17, 2013

Contributor

I'll change it. While I understand the point about it not making a difference in the test I do feel that having it there makes the intent more clear.

@birkirb

birkirb Nov 17, 2013

Contributor

I'll change it. While I understand the point about it not making a difference in the test I do feel that having it there makes the intent more clear.

@birkirb

This comment has been minimized.

Show comment Hide comment
@birkirb

birkirb Nov 17, 2013

Contributor

@robin850 Updated.

Contributor

birkirb commented Nov 17, 2013

@robin850 Updated.

@birkirb

This comment has been minimized.

Show comment Hide comment
@birkirb

birkirb Nov 23, 2013

Contributor

Build now passing.
@robin850 Is this done, or does someone else have to look at it as well?

Contributor

birkirb commented Nov 23, 2013

Build now passing.
@robin850 Is this done, or does someone else have to look at it as well?

@robin850

This comment has been minimized.

Show comment Hide comment
@robin850

robin850 Nov 23, 2013

Member

Thank you @birkirb! ❤️

@rafaelfranca : Could you have a look please ? This looks good to me!

Member

robin850 commented Nov 23, 2013

Thank you @birkirb! ❤️

@rafaelfranca : Could you have a look please ? This looks good to me!

Fix breakage in XmlMini
- Boolean parsing breaks on non strings (i.e. integer 1|0)
- Symbol parsing breaks on non strings.
- BigDecimal parsing breaks due to missing require.
- Update changelog.
@birkirb

This comment has been minimized.

Show comment Hide comment
@birkirb

birkirb Jan 31, 2014

Contributor

@rafaelfranca I still have to patch this bug in my code, anyway to get this in somewhere?

Contributor

birkirb commented Jan 31, 2014

@rafaelfranca I still have to patch this bug in my code, anyway to get this in somewhere?

rafaelfranca added a commit that referenced this pull request Feb 1, 2014

Merge pull request #12769 from birkirb/master
Boolean parser blows up on a Fixnum.

Conflicts:
	activesupport/CHANGELOG.md

@rafaelfranca rafaelfranca merged commit 8dd4aca into rails:master Feb 1, 2014

rafaelfranca added a commit that referenced this pull request Feb 1, 2014

Merge pull request #12769 from birkirb/master
Boolean parser blows up on a Fixnum.

Conflicts:
	activesupport/CHANGELOG.md

Conflicts:
	activesupport/CHANGELOG.md
@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Feb 1, 2014

Member

Not anymore. Thank you for the ping

Member

rafaelfranca commented Feb 1, 2014

Not anymore. Thank you for the ping

@birkirb

This comment has been minimized.

Show comment Hide comment
@birkirb

birkirb Feb 1, 2014

Contributor

No worries, glad to see it in there.

Contributor

birkirb commented Feb 1, 2014

No worries, glad to see it in there.

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