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

Backport OpenStruct fix to version shipped in Ruby 3.1 #3

Merged
merged 3 commits into from
Jul 6, 2022

Conversation

dobrite
Copy link
Contributor

@dobrite dobrite commented Jun 30, 2022

Problem Statement

The OpenStruct implementation differs between Ruby 2.7 and 3.1. The
latter comes with OpenStruct 0.5.2. That version has a bug that was
fixed in version 0.5.4.

The TL;DR of the impact of that bug in this repo is that brakeman
includes JSON with an attribute named class. This shadows the normal
class method and causes self.class.ancestors to blow up. The fix
uses the aliased class! method to do the Right Thing™.

Here is a failing Publishing run. We're on Ruby 3.1.

Here is a snippet illustrating the error:

[13] pry(main)> JSON.parse("{ \"class\": \"s\", \"method\": null }", { object_class: OpenStruct })

NoMethodError: undefined method `ancestors' for "s":String

        self.class.ancestors.any? do |mod|
                  ^^^^^^^^^^
from /Users/dave/.rbenv/versions/3.1.2/lib/ruby/3.1.0/ostruct.rb:249:in `is_method_protected!'

Fix

Backport the fix to broken versions of ostruct. Tested in publishing SAT.

The OpenStruct implementation differs between Ruby 2.7 and 3.1. The
latter comes with OpenStruct 0.5.2. That version has a bug that was
fixed [here](ruby/ostruct#37).

The TL;DR of the impact of that bug in this repo is that brakeman
includes JSON with an attribute named `class`. This shadows the normal
`class` method and causes `self.class.ancestors` to blow up. The fix
uses the aliased `class!` method to do the Right Thing™.
@dobrite dobrite marked this pull request as ready for review June 30, 2022 21:58
@dobrite dobrite requested a review from wassimk June 30, 2022 21:58
Copy link
Member

@wassimk wassimk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pioneering Ruby 3.1 🎉 !

This looks like a great solution to me. We can remove it when everyone is on Ruby 3.1.

@dobrite
Copy link
Contributor Author

dobrite commented Jun 30, 2022

@wassimk This isn't working as-is. I'll rework and then ping you again. Sorry for the bother!

@dobrite dobrite marked this pull request as draft June 30, 2022 22:08
@dobrite dobrite marked this pull request as ready for review July 5, 2022 15:50
@dobrite
Copy link
Contributor Author

dobrite commented Jul 5, 2022

@wassimk I reworked this to back port the patch to the broken version.

The prior attempt of installing a later version of ostruct did not work because ostruct is a "default" gem and so one cannot "activate" a newer version.

This fix can get reverted once all repos that use balto-brakeman are updated to a version of Ruby with a fixed version of ostruct. Best guess is that will require Ruby 3.2 and above. So, this might need to be in here for a while.

@dobrite
Copy link
Contributor Author

dobrite commented Jul 5, 2022

@dobrite dobrite changed the title Install latest ostruct to fix JSON parsing into an OpenStruct Backport OpenStruct fix to version shipped in Ruby 3.1 Jul 5, 2022
@wassimk
Copy link
Member

wassimk commented Jul 5, 2022

Thank you for fixing this for everyone else too. 👏🏼

@wassimk wassimk self-requested a review July 5, 2022 23:41
@dobrite dobrite requested a review from molawson July 6, 2022 14:10
@dobrite
Copy link
Contributor Author

dobrite commented Jul 6, 2022

@molawson I tagged you for a second review because I saw you in the commit history too 😅

@dobrite dobrite merged commit 5270dad into main Jul 6, 2022
@dobrite dobrite deleted the do/fix-ruby-3.1 branch July 6, 2022 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants