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

Ensure response.parsed_body support for pattern matching #49003

Merged

Conversation

seanpdoyle
Copy link
Contributor

Motivation / Background

Both Nokogiri and Minitest have merged the PRs mentioned to integrate support for Ruby's Pattern matching
(sparklemotion/nokogiri#2523 and minitest/minitest#936, respectively).

Detail

This commit adds coverage for those new assertions, and incorporates examples into the documentation for the response.parsed_body method.

Additional information

In order to incorporate pattern-matching support for JSON responses, this commit changes the response parser to call JSON.parse with object_class: ActiveSupport::HashWithIndifferentAccess, since String instances for Hash keys are incompatible with Ruby's syntactically pattern matching.

For example:

irb(main):001:0> json = {"key" => "value"}
=> {"key"=>"value"}
irb(main):002:0> json in {key: /value/}
=> false

irb(main):001:0> json = {"key" => "value"}
=> {"key"=>"value"}
irb(main):002:0> json in {"key" => /value/}
.../3.2.0/lib/ruby/gems/3.2.0/gems/irb-1.7.4/lib/irb/workspace.rb:113:in `eval': (irb):2: syntax error, unexpected terminator, expecting literal content or tSTRING_DBEG or tSTRING_DVAR or tLABEL_END (SyntaxError)
json in {"key" => /value/}
             ^

        .../ruby/3.2.0/lib/ruby/gems/3.2.0/gems/irb-1.7.4/exe/irb:9:in `<top (required)>'
        .../ruby/3.2.0/bin/irb:25:in `load'
        .../ruby/3.2.0/bin/irb:25:in `<main>'

When the Hash maps String keys to Symbol keys, it's able to be pattern matched:

irb(main):005:0> json = {"key" => "value"}.with_indifferent_access
=> {"key"=>"value"}
irb(main):006:0> json in {key: /value/}
=> true

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the actionpack label Aug 22, 2023
@@ -53,6 +53,6 @@ def self.register_encoder(mime_name, param_encoder: nil, response_parser: nil)
end

register_encoder :html, response_parser: -> body { Rails::Dom::Testing.html_document.parse(body) }
register_encoder :json, response_parser: -> body { JSON.parse(body) }
register_encoder :json, response_parser: -> body { JSON.parse(body, object_class: ActiveSupport::HashWithIndifferentAccess) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is potentially breaking and not backwards compatible.

Since it's a test harness change, and doesn't affect host application's implementations, I'm uncertain of the impact.

The risk is that it makes callers of response.parsed_body more flexible than the authors originally intended.

While that's true, tests that are currently passing would continue to pass, since String and Symbol keys are interchangeable.

The trouble is that subsequent changes to those suites that used symbols would pass when the did not before.

Some alternatives to this change:

  • Wrap the Hash or Array constructed by JSON.parse in a new Class that only uses an indifferent access Hash when calling #deconstruct_keys to limit the change to only affect pattern matching
  • Add a new framework default for 7.1 that uses object_class: ActiveSupport::HashWithIndifferentAccess, then fall back to using object_class: Hash)
  • revert any change to the implementation, and encourage callers to chain .with_indifferent_access or .deep_symbolize_keys when using Ruby's pattern matching

@seanpdoyle seanpdoyle force-pushed the test-response-parsed-body-pattern-matching branch from 4acc596 to b965da0 Compare August 22, 2023 20:12
@seanpdoyle seanpdoyle force-pushed the test-response-parsed-body-pattern-matching branch from b965da0 to c99088d Compare August 22, 2023 20:30
@rafaelfranca
Copy link
Member

One test failed because of this change

@seanpdoyle seanpdoyle force-pushed the test-response-parsed-body-pattern-matching branch from c99088d to 6d575ca Compare August 22, 2023 21:12
@seanpdoyle seanpdoyle force-pushed the test-response-parsed-body-pattern-matching branch from 6d575ca to 22ee27c Compare August 23, 2023 17:25
Both `Nokogiri` and `Minitest` have merged the PRs mentioned to
integrate support for Ruby's Pattern matching
(sparklemotion/nokogiri#2523 and
minitest/minitest#936, respectively).

This commit adds coverage for those new assertions, and incorporates
examples into the documentation for the `response.parsed_body` method.

In order to incorporate pattern-matching support for JSON responses,
this commit changes the response parser to call `JSON.parse` with
[object_class: ActiveSupport::HashWithIndifferentAccess][object_class],
since String instances for `Hash` keys are incompatible with Ruby's
syntactically pattern matching.

For example:

```ruby
irb(main):001:0> json = {"key" => "value"}
=> {"key"=>"value"}
irb(main):002:0> json in {key: /value/}
=> false

irb(main):001:0> json = {"key" => "value"}
=> {"key"=>"value"}
irb(main):002:0> json in {"key" => /value/}
.../3.2.0/lib/ruby/gems/3.2.0/gems/irb-1.7.4/lib/irb/workspace.rb:113:in `eval': (irb):2: syntax error, unexpected terminator, expecting literal content or tSTRING_DBEG or tSTRING_DVAR or tLABEL_END (SyntaxError)
json in {"key" => /value/}
             ^

        .../ruby/3.2.0/lib/ruby/gems/3.2.0/gems/irb-1.7.4/exe/irb:9:in `<top (required)>'
        .../ruby/3.2.0/bin/irb:25:in `load'
        .../ruby/3.2.0/bin/irb:25:in `<main>'
```

When the Hash maps String keys to Symbol keys, it's able to be pattern
matched:

```ruby
irb(main):005:0> json = {"key" => "value"}.with_indifferent_access
=> {"key"=>"value"}
irb(main):006:0> json in {key: /value/}
=> true
```

[object_class]: https://docs.ruby-lang.org/en/3.2/JSON.html#module-JSON-label-Parsing+Options
@seanpdoyle seanpdoyle force-pushed the test-response-parsed-body-pattern-matching branch from 22ee27c to 0f4ab82 Compare August 23, 2023 17:28
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we support versions of Ruby that pre-date pattern matching, the test cases in this file need to be declared separate from the _test.rb file, then included conditionally based on RUBY_VERSION.

If there is a way to do this with Ruby compiler directives that instruct <= 3.0 versions to ignore them, I'm happy to revert this split and unify the files.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, what @zenspider asked me to do in the tests for pattern matching minitest is to conditionally eval the code. But what you did above is what I did in the Nokogiri test suite.

@seanpdoyle
Copy link
Contributor Author

One test failed because of this change

@rafaelfranca I've made the necessary changes to pass the suite in CI.

@rafaelfranca rafaelfranca merged commit a7a1217 into rails:main Aug 23, 2023
4 checks passed
@seanpdoyle seanpdoyle deleted the test-response-parsed-body-pattern-matching branch August 23, 2023 18:03
@flavorjones
Copy link
Member

@seanpdoyle Thank you for this! It's the first feedback I've heard that the pattern matching work in nokogiri and minitest was useful. ❤️ ❤️ ❤️

flavorjones added a commit to sparklemotion/nokogiri that referenced this pull request Aug 26, 2023
**What problem is this PR intended to solve?**

Pattern matching was introduced as an experimental feature in v1.14.0
(see #2360 and #2523 for historical context).

Seeing downstream libraries adopt pattern matching for testing, as in
rails/rails#49003, means this is apparently
useful enough to start using.

Let's end experimental support and make it a fully-supported feature in
the next minor release, v1.16.0.
zzak pushed a commit to zzak/rails that referenced this pull request Feb 5, 2024
This change is an up-port of rails#50787 which applied a similar fix to 7-1-stable.

Since these tests weren't running before, we didn't notice when the DirectUploads controller tests were broken. My theory is that it has something to do with changing `response.parsed_body` to return a HWIA (in rails#49003).

The differences in the 7-1-stable was to remove both `deep_stringify_keys` in stead a single `Hash#with_indifferent_access` on the metadata.

I'm open to suggestions on the best way to make that assertion look right, and we can backport that to 7-1-stable as well.
zzak added a commit to zzak/rails that referenced this pull request Feb 5, 2024
This change is an up-port of rails#50787 which applied a similar fix to 7-1-stable.

Since these tests weren't running before, we didn't notice when the DirectUploads controller tests were broken. My theory is that it has something to do with changing `response.parsed_body` to return a HWIA (in rails#49003).

The differences in the 7-1-stable was to remove both `deep_stringify_keys` in stead a single `Hash#with_indifferent_access` on the metadata.

I'm open to suggestions on the best way to make that assertion look right, and we can backport that to 7-1-stable as well.

Co-authored-by: Sean Doyle <seanpdoyle@users.noreply.github.com>
Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com>
zzak added a commit to zzak/rails that referenced this pull request Feb 5, 2024
This change is an up-port of rails#50787 which applied a similar fix to 7-1-stable.

Since these tests weren't running before, we didn't notice when the DirectUploads controller tests were broken. My theory is that it has something to do with changing `response.parsed_body` to return a HWIA (in rails#49003).

The differences in the 7-1-stable was to remove both `deep_stringify_keys` in stead a single `Hash#with_indifferent_access` on the metadata.

I'm open to suggestions on the best way to make that assertion look right, and we can backport that to 7-1-stable as well.

Co-authored-by: Sean Doyle <seanpdoyle@users.noreply.github.com>
Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com>
zzak added a commit to zzak/rails that referenced this pull request Feb 5, 2024
This change is an up-port of rails#50787 which applied a similar fix to 7-1-stable.

Since these tests weren't running before, we didn't notice when the DirectUploads controller tests were broken. My theory is that it has something to do with changing `response.parsed_body` to return a HWIA (in rails#49003).

The differences in the 7-1-stable was to remove both `deep_stringify_keys` in stead a single `Hash#with_indifferent_access` on the metadata.

I'm open to suggestions on the best way to make that assertion look right, and we can backport that to 7-1-stable as well.

Co-authored-by: Sean Doyle <seanpdoyle@users.noreply.github.com>
Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com>
zzak added a commit to zzak/rails that referenced this pull request Feb 6, 2024
This change is an up-port of rails#50787 which applied a similar fix to 7-1-stable.

Since these tests weren't running before, we didn't notice when the DirectUploads controller tests were broken. My theory is that it has something to do with changing `response.parsed_body` to return a HWIA (in rails#49003).

This change is different from the 7-1-stable PR in that it removes the need to stringify or symbolize any keys, since we are comparing the metadata with string keys. This is a follow up to rails#43705.

Co-authored-by: Sean Doyle <seanpdoyle@users.noreply.github.com>
Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com>
zzak added a commit to zzak/rails that referenced this pull request Feb 6, 2024
This change is an up-port of rails#50787 which applied a similar fix to 7-1-stable.

Since these tests weren't running before, we didn't notice when the DirectUploads controller tests were broken. My theory is that it has something to do with changing `response.parsed_body` to return a HWIA (in rails#49003).

This change is different from the 7-1-stable PR in that it removes the need to stringify or symbolize any keys, since we are comparing the metadata with string keys. This is a follow up to rails#43705.

Co-authored-by: Sean Doyle <seanpdoyle@users.noreply.github.com>
Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com>
zzak added a commit to zzak/rails that referenced this pull request Feb 6, 2024
This change is an up-port of rails#50787 which applied a similar fix to 7-1-stable.

Since these tests weren't running before, we didn't notice when the DirectUploads controller tests were broken. My theory is that it has something to do with changing `response.parsed_body` to return a HWIA (in rails#49003).

This change is different from the 7-1-stable PR in that it removes the need to stringify or symbolize any keys, since we are comparing the metadata with string keys. This is a follow up to rails#43705.

Co-authored-by: Sean Doyle <seanpdoyle@users.noreply.github.com>
Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com>
Ridhwana pushed a commit to Ridhwana/rails that referenced this pull request Mar 7, 2024
This change is an up-port of rails#50787 which applied a similar fix to 7-1-stable.

Since these tests weren't running before, we didn't notice when the DirectUploads controller tests were broken. My theory is that it has something to do with changing `response.parsed_body` to return a HWIA (in rails#49003).

This change is different from the 7-1-stable PR in that it removes the need to stringify or symbolize any keys, since we are comparing the metadata with string keys. This is a follow up to rails#43705.

Co-authored-by: Sean Doyle <seanpdoyle@users.noreply.github.com>
Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com>
viralpraxis pushed a commit to viralpraxis/rails that referenced this pull request Mar 24, 2024
This change is an up-port of rails#50787 which applied a similar fix to 7-1-stable.

Since these tests weren't running before, we didn't notice when the DirectUploads controller tests were broken. My theory is that it has something to do with changing `response.parsed_body` to return a HWIA (in rails#49003).

This change is different from the 7-1-stable PR in that it removes the need to stringify or symbolize any keys, since we are comparing the metadata with string keys. This is a follow up to rails#43705.

Co-authored-by: Sean Doyle <seanpdoyle@users.noreply.github.com>
Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com>
fractaledmind pushed a commit to fractaledmind/rails that referenced this pull request May 13, 2024
This change is an up-port of rails#50787 which applied a similar fix to 7-1-stable.

Since these tests weren't running before, we didn't notice when the DirectUploads controller tests were broken. My theory is that it has something to do with changing `response.parsed_body` to return a HWIA (in rails#49003).

This change is different from the 7-1-stable PR in that it removes the need to stringify or symbolize any keys, since we are comparing the metadata with string keys. This is a follow up to rails#43705.

Co-authored-by: Sean Doyle <seanpdoyle@users.noreply.github.com>
Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com>
noahgibbs pushed a commit to noahgibbs/rails that referenced this pull request May 24, 2024
This change is an up-port of rails#50787 which applied a similar fix to 7-1-stable.

Since these tests weren't running before, we didn't notice when the DirectUploads controller tests were broken. My theory is that it has something to do with changing `response.parsed_body` to return a HWIA (in rails#49003).

This change is different from the 7-1-stable PR in that it removes the need to stringify or symbolize any keys, since we are comparing the metadata with string keys. This is a follow up to rails#43705.

Co-authored-by: Sean Doyle <seanpdoyle@users.noreply.github.com>
Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants