phoet / asin

:books: :package: Amazon Simple INterface - Support for ItemLookup, SimilarityLookup, Search, BrowseNode and Cart Operations
http://asin.herokuapp.com/
167 stars 59 forks source link

Latest Ruby/Rails issue... #22

Closed RichIsOnRails closed 11 years ago

RichIsOnRails commented 11 years ago

Ruby 1.9.3p392, Rails 3.2.13, I get the following error on some products:

uninitialized constant REXML::Text::Document

The line that causes it is this: @items = client.lookup @item.asin

client is client = ASIN::Client.instance

RichIsOnRails commented 11 years ago

I was able to replicate this without Rails by copy pasting the code from the readme and changing to my credentials, see a partial stacktrace below:

C:/Ruby193/lib/ruby/1.9.1/rexml/text.rb:386:in `block in unnormalize': uninitialized constant REXML::Text::Document (NameError)
        from C:/Ruby193/lib/ruby/1.9.1/rexml/text.rb:384:in `gsub'
        from C:/Ruby193/lib/ruby/1.9.1/rexml/text.rb:384:in `unnormalize'
        from C:/Ruby193/lib/ruby/gems/1.9.1/gems/crack-0.3.2/lib/crack/xml.rb:185:in `unnormalize_xml_entities'
        from C:/Ruby193/lib/ruby/gems/1.9.1/gems/crack-0.3.2/lib/crack/xml.rb:82:in `to_hash'
        from C:/Ruby193/lib/ruby/gems/1.9.1/gems/crack-0.3.2/lib/crack/xml.rb:110:in `merge!'
        from C:/Ruby193/lib/ruby/gems/1.9.1/gems/crack-0.3.2/lib/crack/xml.rb:110:in `block in to_hash'
        from C:/Ruby193/lib/ruby/gems/1.9.1/gems/crack-0.3.2/lib/crack/xml.rb:108:in `each'
        from C:/Ruby193/lib/ruby/gems/1.9.1/gems/crack-0.3.2/lib/crack/xml.rb:108:in `to_hash'
        from C:/Ruby193/lib/ruby/gems/1.9.1/gems/crack-0.3.2/lib/crack/xml.rb:110:in `merge!'
        from C:/Ruby193/lib/ruby/gems/1.9.1/gems/crack-0.3.2/lib/crack/xml.rb:110:in `block in to_hash'
        from C:/Ruby193/lib/ruby/gems/1.9.1/gems/crack-0.3.2/lib/crack/xml.rb:108:in `each'
phoet commented 11 years ago

this is an issue with crack https://github.com/jnunemaker/crack/pull/42

welike commented 11 years ago

How about instead of just closing the issue, you drop back to a version of crack that does not introduce this regression. This seems to be a common response in open source software these days where the finger is simply pointed elsewhere.

The facts are, asin is broken for people. You have control over what dependencies to use, and which versions of them to use. Could you please reopen and fix this?

kevinelliott commented 11 years ago

I agree here too.

phoet commented 11 years ago

the isse seems to be related to changes in ruby, so what should i do about that? feel free to provide a patch to fix this issue or the dependency to crack.

kurtfunai commented 16 days ago Seems related to the changes in ruby-1.9.3-p392 release: http://www.ruby-lang.org/en/news/2013/02/22/ruby-1-9-3-p392-is-released/ "Entity expansion DoS vulnerability in REXML (XML bomb)"

Changing my local version back to 1.9.3-p385 fixed the issues I was experiencing.

RichIsOnRails commented 11 years ago

WeLike, while i do agree with your general assessment of certain gem developers finger pointing instead of fixing, phoet is right about ruby breaking stuff. The latest release of Ruby had a security fix that made crack incompatible. The crack team has been unresponsive in fixing this (it's a 1 line code change.)

If you want to fix ASIN on the latest version of ruby, you can use my fork of crack which has the fix for crack.

https://github.com/RichIsOnRails/crack

That's how I ultimately got things running again.

phoet commented 11 years ago

guys, this software is opensource and i dedicate my spare time to maintain it.

unless you know me in person, please do not post such stuff in the issues.

it is not an issue with the library, its a thrd party dependency and i am not maintaining windows at all.

so if you want to fix this i am happy to integrate your pull requests as fast as i can.

kevinelliott commented 11 years ago

I think you are missing our point. Most of us are involved in open source, and can appreciate your time and efforts here. The point we are making is that as a gem maintainer, you have the opportunity to define your dependencies and lock them to known working versions. You have chosen to lock the third-party dependency to a version that is broken, and thus indirectly you are also held accountable for it breaking. It seems strange to me that you feel it's ok that there is a broken dependency that only works in some environments under very strict conditions (a particular ruby version).

You should be working with the third-party dependency's team to resolve this (since as users, we can only complain, or submit a fork's pull request with a gem dependency change), but also in the mean time mitigating the issue by locking to a previous version of the third-party dependency that does not have this regression. As a consumer of the third-party gem, you have more clout than we do as complaining users of your gem. Their lack of attention to the issue is proof of this :(

Again, I'm not being critical of you as a person (despite how I may be presenting myself here), and do appreciate your efforts and time that you have spent to make this project available to all of us.

phoet commented 11 years ago

instead of compaining you could just submit a patch with a less strict dependency to the broken library like it was done before https://github.com/phoet/asin/pull/13

kurtfunai commented 11 years ago

I'm going to jump onto @phoet's side here. The version of crack used by asin was stable and working. As far as I know, there has not been any recent releases that cause this dependency to break. The last commit to crack was 4 months ago [ https://github.com/jnunemaker/crack/commits/ ].

It was the newest release of ruby that caused an issue with the crack gem, and developers have been discussing this change (and submitted a pull request) to fix the issue. So far that change has not been accepted. The upgrade from 1.9.3-p385 to 1.9.3-p392 (or to ruby-2.0.0) causes this error.

If you'd like to see this fixed, please make a comment in the pull request on the crack gem. https://github.com/jnunemaker/crack/pull/42

rubiii commented 11 years ago

@phoet savonrb/nori#37. why not require rexml/document yourself? that's an easy fix.

rubiii commented 11 years ago

@phoet i could submit a pull request right here from github :kissing_closed_eyes:

phoet commented 11 years ago

@rubiii hey, its nike calling "just do it"

phoet commented 11 years ago

there is a new version here: https://rubygems.org/gems/asin/versions/1.1.2

phoet commented 11 years ago

:tada:

kurtfunai commented 11 years ago

Awesome!