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

aws-sdk-core implicitly depends on Oj gem #2247

Closed
leosurwiki opened this issue Mar 4, 2020 · 12 comments · Fixed by #2254
Closed

aws-sdk-core implicitly depends on Oj gem #2247

leosurwiki opened this issue Mar 4, 2020 · 12 comments · Fixed by #2254
Labels
dependencies This issue is a problem in a dependency. investigating Issue is being investigated

Comments

@leosurwiki
Copy link

leosurwiki commented Mar 4, 2020

According to

aws-core-sdk will try to use OJ if its installed, and the version of this dependency is not controlled at all. When error raised from Oj, it also tries to convert Oj::ParseError to Aws::Json::ParseError

ohler55/oj#584
However, according to the Author of Oj, Oj is supposed to raise Json::ParserError, and raising Oj::ParserError is the behavior caused by a bug in a previous version.

Context:
Same issue from #2085 but still unfixed for us since we use a different Oj version.

Suggested changes could be:

  • also rescue JSON::ParserError even if Oj is installed
  • declare the Oj dependency explicitly
    ...
@mullermp
Copy link
Contributor

mullermp commented Mar 4, 2020

FYI - We control the version of OJ depending on the Ruby version: https://github.com/aws/aws-sdk-ruby/blob/master/Gemfile#L47-L56

@mullermp
Copy link
Contributor

mullermp commented Mar 4, 2020

We will investigate this.

@mullermp mullermp added the investigating Issue is being investigated label Mar 4, 2020
@leosurwiki
Copy link
Author

leosurwiki commented Mar 5, 2020

Gemfile cannot serve for transitive dependency version control, you might need to declare it in the gemspec https://github.com/aws/aws-sdk-ruby/blob/master/gems/aws-sdk-core/aws-sdk-core.gemspec

@mullermp
Copy link
Contributor

mullermp commented Mar 5, 2020

That's true. We try not to add dependencies in core. If Oj is not installed, we fall back to JSON (https://github.com/aws/aws-sdk-ruby/blob/master/gems/aws-sdk-core/lib/aws-sdk-core/json.rb#L63) and we do similar things such as for XML (https://github.com/aws/aws-sdk-ruby/blob/master/gems/aws-sdk-core/lib/aws-sdk-core/xml/parser.rb#L68).

Edit - to clarify, we do it this way so that customers don't NEED oj or other XML parsing libraries, for example.

@leosurwiki
Copy link
Author

Totally understandable, but it's kinda risky to have a dependency without version control unless it's very stable. Since this has been discovered as an issue, I wish it's possible to reevaluate this decision 🙏

@mullermp mullermp added the dependencies This issue is a problem in a dependency. label Mar 5, 2020
@mullermp
Copy link
Contributor

mullermp commented Mar 5, 2020

Unfortunately I don't think I can add it as a spec dependency anyway because it's a native C extension gem, and then JRuby customers would be broken. The implementation frees up what parser you may be using.

I'm thinking I can just add JSON::ParserError to the list here:

[Oj::ParseError, EncodingError]

I'm open to other ideas.

@leosurwiki
Copy link
Author

👍 that also sounds good to me

@mullermp
Copy link
Contributor

mullermp commented Mar 5, 2020

Before I make this change, I'm trying to track down what version that was introduced in. I'm using the latest oj version 3.10.5 and trying to run Oj.load("<ServiceUnavailableException/>") but it raises an Oj::ParseError as expected..

@mullermp
Copy link
Contributor

mullermp commented Mar 5, 2020

From that other issue, I tried using version 3.3.6 as well:

irb(main):001:0> require 'oj'
=> true
irb(main):002:0> Oj::VERSION
=> "3.3.6"
irb(main):003:0> Oj.load("<ServiceUnavailableException/>")
Traceback (most recent call last):
        5: from /Users/mamuller/.rbenv/versions/2.6.5/bin/irb:23:in `<main>'
        4: from /Users/mamuller/.rbenv/versions/2.6.5/bin/irb:23:in `load'
        3: from /Users/mamuller/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/irb-1.0.0/exe/irb:11:in `<top (required)>'
        2: from (irb):3
        1: from (irb):3:in `load'
Oj::ParseError (unexpected character at line 1, column 1 [parse.c:690])

@leosurwiki
Copy link
Author

This behavior might be related to another gem.
Could you also try again with:
oj_mimic_json (1.0.1)

@mullermp
Copy link
Contributor

mullermp commented Mar 9, 2020

Recap from other thread - key component is mimic_JSON even when passed explicit options.

Aws> Oj.mimic_JSON
=> JSON
Aws> Oj.load("<ServiceUnavailableException/>", { mode: :compat, symbol_keys: false })
JSON::ParserError: Empty input () at line 1, column 1 [parse.c:995] in '<ServiceUnavailableException/>
from (pry):3:in `load'

@mullermp
Copy link
Contributor

mullermp commented Mar 9, 2020

mimic_JSON will certainly complicate testing. It appears to rewrite the JSON namespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies This issue is a problem in a dependency. investigating Issue is being investigated
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants