Skip to content

Explicitely load ostruct so it's compatible with latest JSON gem#49

Merged
ilyazub merged 2 commits intoserpapi:masterfrom
lcmen:fix-hash-extensions-for-latest-json
Apr 10, 2024
Merged

Explicitely load ostruct so it's compatible with latest JSON gem#49
ilyazub merged 2 commits intoserpapi:masterfrom
lcmen:fix-hash-extensions-for-latest-json

Conversation

@lcmen
Copy link
Contributor

@lcmen lcmen commented Apr 5, 2024

The latest json gem version (2.7.2 for the time being) no longer requires OpenStruct (PR: https://github.com/flori/json/pull/565/files) making this gem incompatible with it.

This PR modifies the codebase to explicitly require 'ostruct' so JSON gem is aware of it but most importantly it makes OpenStruct available in lib/utils/hash_extension.

deivid-rodriguez
deivid-rodriguez approved these changes Apr 5, 2024
View reviewed changes
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Bundler daily specs run into this too, thanks for fixing it!

javierjulio
javierjulio approved these changes Apr 5, 2024
View reviewed changes
Copy link
Contributor

@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

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

Thanks! We ran into the following error today which this should address.

bundler: failed to load command: turbo_tests (/usr/local/bundle/bin/turbo_tests)
/usr/local/bundle/gems/turbo_tests-2.2.0/lib/utils/hash_extension.rb:4:in `to_struct': uninitialized constant CoreExtensions::OpenStruct (NameError)

@inkstak
Copy link

inkstak commented Apr 8, 2024

I've also had to fixed it in #41 (but an explicit PR might be better).

@lcmen
Copy link
Contributor Author

lcmen commented Apr 8, 2024

@javierjulio @deivid-rodriguez who can I ping to merge it and release a new version?

@klaustopher klaustopher mentioned this pull request Apr 8, 2024
Merged
@javierjulio javierjulio mentioned this pull request Apr 8, 2024
Open
javierjulio added a commit to javierjulio/rspec-rails that referenced this pull request Apr 8, 2024
@javierjulio
Require ostruct to fix test failure
5f03be3 
This resolves the following OpenStruct error. There is only 1 reference to OpenStruct in this file, no other references in the code base.

```
NameError:
uninitialized constant RSpec::Rails::OpenStruct
# ./spec/rspec/rails/example/view_example_group_spec.rb:190:in `controller'
# ./spec/rspec/rails/example/view_example_group_spec.rb:196:in `block (3 levels) in <module:Rails>'
# /home/runner/work/rspec-rails/rspec-core/lib/rspec/core/example.rb:263:in `instance_exec'
```

This could be caused by a recent gem update like `json` as I encountered this with serpapi/turbo_tests#49 as I use turbo_tests to run my rspec test suite.
javierjulio added a commit to javierjulio/rspec-rails that referenced this pull request Apr 8, 2024
@javierjulio
Fix uninitialized constant RSpec::Rails::OpenStruct error
9631355 
This started to occur on recent test suite runs. It may be related to a json gem update that removed the ostruct dependency as I encountered this with turbo_tests and a fix for that is in serpapi/turbo_tests#49 with more details.
@javierjulio javierjulio mentioned this pull request Apr 8, 2024
Merged
@bmulholland bmulholland mentioned this pull request Apr 9, 2024
Closed
@bmulholland
Copy link

bmulholland commented Apr 9, 2024

Closes #52, which I created because such an issue would have saved me 15 minutes (I looked only in Issues, not in PRs).

@ilyazub ilyazub linked an issue Apr 10, 2024 that may be closed by this pull request
json v2.7.2 breaks turbo_tests #52
Closed
ilyazub
ilyazub approved these changes Apr 10, 2024
View reviewed changes
Copy link
Collaborator

@ilyazub ilyazub left a comment

Choose a reason for hiding this comment

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

Thank you @lcmen! Looks good to me.

I added json gem as an explicit dependency in 0767720.

@ilyazub ilyazub merged commit 7031210 into serpapi:master Apr 10, 2024
@lcmen lcmen deleted the fix-hash-extensions-for-latest-json branch April 10, 2024 05:38
@lcmen
Copy link
Contributor Author

lcmen commented Apr 10, 2024

Thank you @ilyazub !

@ilyazub
Copy link
Collaborator

ilyazub commented Apr 10, 2024

Released in v2.2.1: https://rubygems.org/gems/turbo_tests/versions/2.2.1.

JonRowe pushed a commit to javierjulio/rspec-rails that referenced this pull request Apr 10, 2024
@javierjulio @JonRowe
Require ostruct to fix test failure
883d87a 
This resolves the following OpenStruct error. There is only 1 reference to OpenStruct in this file, no other references in the code base.

```
NameError:
uninitialized constant RSpec::Rails::OpenStruct
# ./spec/rspec/rails/example/view_example_group_spec.rb:190:in `controller'
# ./spec/rspec/rails/example/view_example_group_spec.rb:196:in `block (3 levels) in <module:Rails>'
# /home/runner/work/rspec-rails/rspec-core/lib/rspec/core/example.rb:263:in `instance_exec'
```

This could be caused by a recent gem update like `json` as I encountered this with serpapi/turbo_tests#49 as I use turbo_tests to run my rspec test suite.
@ilyazub ilyazub mentioned this pull request Apr 10, 2024
Merged
4 tasks
@hsbt
Copy link
Contributor

hsbt commented Apr 11, 2024

Thanks all ❤️ (I'm maintainer of json gem)

@ilyazub
Copy link
Collaborator

ilyazub commented Apr 15, 2024

This PR introduced a problem in Ruby-core CI: ruby/ruby#10496.

@javierjulio
Copy link
Contributor

javierjulio commented Apr 15, 2024 •
edited
Loading

@ilyazub I encountered the same JSON gem related error in the rspec-rails repo when submitting a PR. I resolved that issue initially by requiring OpenStruct but since it was only one instance, I replaced it later with a Struct instead due to maintainer's suggestion. I wonder if it would help to do that same replacement here too? In general, it's long been advised to not use OpenStruct anyway.

@javierjulio javierjulio mentioned this pull request Apr 15, 2024
Merged
@javierjulio
Copy link
Contributor

javierjulio commented Apr 15, 2024

@ilyazub I have a patch in #53 ready for review that should replace the OpenStruct with a Struct instead so the ostruct dependency can be removed safely.

@javierjulio javierjulio mentioned this pull request Apr 15, 2024
Merged
@ilyazub
Copy link
Collaborator

ilyazub commented Apr 16, 2024

@javierjulio Makes sense. Thank you for debugging it!

I encountered the same JSON gem related error in the rspec-rails repo when submitting a PR.

I understood the consequences (rspec-rails/ruby-core had issues), but not the actual root cause. What error it was in the rspec-rails repo?

@javierjulio
Copy link
Contributor

javierjulio commented Apr 16, 2024

@ilyazub you're welcome! It was the same error as reported here, essentially a "NameError: uninitialized constant OpenStruct" error message. This is a failed build on rspec-rails from my first PR where that error occurred. I'm not sure I understand the root cause but it's tied to the json gem somehow. The understanding I and others came to elsewhere was it would be best to remove the OpenStruct usage rather than continue to rely on it.

@ilyazub
Copy link
Collaborator

ilyazub commented Apr 16, 2024

Thank you @javierjulio! rspec/rspec-rails#2755 makes total sense.

ilyazub added a commit that referenced this pull request Apr 16, 2024
@ilyazub
Revert "Merge pull request #49 from lcmen/fix-hash-extensions-for-lat…
ced6eb7 
…est-json"

This reverts commit 7031210, reversing
changes made to b9e812e.
@ilyazub ilyazub mentioned this pull request Apr 16, 2024
Merged
@lcmen
Copy link
Contributor Author

lcmen commented Apr 17, 2024

Thank you guys for addressing from bigger picture perspective 🙇 . I wasn't aware of the problem in other repos.

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

Reviewers

@ilyazub ilyazub ilyazub approved these changes

+2 more reviewers

@javierjulio javierjulio javierjulio approved these changes

@deivid-rodriguez deivid-rodriguez deivid-rodriguez approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

json v2.7.2 breaks turbo_tests

7 participants

@lcmen @inkstak @bmulholland @ilyazub @hsbt @javierjulio @deivid-rodriguez

Comments