Allow jsonapi-rspec to match Symbol or String based source documents

[sa73917]

What is the current behavior?

The current version of the jsonapi-rspec matcher requires the source jsonapi document to be a string keyed hash and requires all hash parameters to use the same key style as the source jsonapi document.

What is the new behavior?

All matchers now support:

• a string or symbol keyed jsonapi document
• (where applicable) a string or symbol keyed parameter hash (or array of hashes)

Checklist

Please make sure the following requirements are complete:

• Tests for the changes have been added (for bug fixes / features)
  All Matchers have a spec that tests the matcher against Symbol and String keyed jsonapi documents. The Relationships matcher did not have an spec so I have created one based on the sample matchers in the README and the code.
  Where applicable the tests include Symbol and String keyed hash parameters

• Docs have been reviewed and added / updated if needed (for bug fixes / features)
  README.md document has been updated to provide an example spec helper, and examples of String and Symbol based keys.

• All automated checks pass (CI/CD)
  rubocop and rspec tests pass.
  NB1. I have added the new rubocop tests and a few cop defaults to rubocop.yml in 633b06.
  NB2. I have added spec_helper into .rspec to save including it in all specs in 33eb64.

[stas]

@sa73917 thank you for the contribution and the nice PR!!!

Before we proceed with the full review, I'd like to raise some questions and try to understand your approach a bit on a higher level...

Particularly, I believe you took the ActiveSupport approach here, and ended up monkey-patching tested data (based on the IndifferentAccess module, we're overwriting core Hash#dig and other accessor methods). I generally don't find this a good idea and try to encourage everyone to stay away from such technics.

Would you reconsider the approach and instead use the core JSON#parse method to take care of the symbol/string names instead?

Consider this...

module JSONAPI
  module RSpec
    mattr_accessor :use_indifferent_access

    def as_indifferent_hash(doc)
      return doc unless self.use_indifferent_access
      return doc.with_indifferent_access if doc.respond_to?(:with_indifferent_access) && self.use_indifferent_access
      JSON.parse(JSON.generate(doc))
    end

    module Meta
      ::RSpec::Matchers.define :have_meta do |val|
        match do |actual|
          actual = JSONAPI::RSpec.as_indifferent_hash(actual)
          actual.key?('meta') &&
            (!val || actual['meta'] == JSONAPI::RSpec.as_indifferent_hash(val))
        end
      end
    end
  end
end

An in a similar manner just convert the names to string if the indifferent access is set.

Another important thing to mention is that I believe this should be an absolutely optional feature. JSONAPI spec clearly communicates that the format is JSON, it's just the Ruby that blurries the lines between how JSON can be parsed/handled. As in, a parsed JSON response would never have symbols or mixed key names in it...

Let me know what you think and thanks again for the contribution!

[sa73917]

Hi @stas , thanks for the quick reply :). I'm more than happy to consider any approach as I'm pretty new to ruby (and this being a first PR here, open to feedback)..

Using JSON.parse(JSON.generate(doc)) and doing String key'd hash comparisons slotted in pretty easily - definitely simpler than how I was doing it.

Reading through the code you've provided above to add a toggle for indifferent access (assuming I'm reading things right), you're relying on various rails methods/variables (mattr_accessor, with_indifferent_access). Was this the intent? I was trying to achieve the 'indifferent' result without requiring any additional gems to save on bloating this one. Of couse there's every possibility there's a way that I don't know of to use these rails features without bringing in all of ActiveSupport - see previous comment about being new to all this! :-)

I agree with you that the jsonapi payload should be JSON rather than a Symbol key'd hash but symbolizing the payload makes other (non jsonapi-rspec) tests far more readable and having two versions of the payload, one for jsonapi-rspec tests and one for all the others seems wonky - hence this PR.

Anyway, I was playing around with your example this morning (though with a manually created setter/getter for the toggle in the module) and as written Meta was just returning undefined method 'as_indifferent_hash' for JSONAPI::RSpec:Module. Probably just me doing something stupid but to work around that I made it a static method and it at least ran, but then was getting some fairly random results around what the use_indifferent_access toggle was set to when I got into the matcher (even if I set it inside the it directly before the is expected!?). I wasn't getting very far so decided to try a chain to toggle indifferent on - just to see how that would work. eg:

      ::RSpec::Matchers.define :have_type do |expected|
        match do |actual|
          actual = JSONAPI::RSpec.as_indifferent_hash(actual) if @use_indifferent_access

          (actual || {})['type'] == expected
        end

        chain :with_indifferent_access do
          @use_indifferent_access = true
        end
      end

which makes it obvious in the test

it { is_expected.to have_type('bar').with_indifferent_access }

and makes the output pretty clear:

JSONAPI::RSpec::Type
  #have_type
    with empty document
      is expected not to have type "bar"
    with stringifyed doc
      is expected to have type "bar" with indifferent access
      is expected not to have type "foo" with indifferent access
      without indifferent access
        is expected to have type "bar"
    with symbolized doc
      is expected not to have type "foo" with indifferent access
      is expected to have type "bar" with indifferent access
      without indifferent access
        is expected not to have type "bar"

But made me wonder whether adding .with_indifferent_access to all the tests would be a pain in the proverbial if someone's doing a lot of tests on the payload. Anyway - I'm supposed to be doing my day job so I'll have another look later and see if I can get your idea working.

Post edit - understand now that it parses the entire spec file before running the individual tests in it - so setting the toggle on and off between tests doesn't work :) - hence my somewhat random results earlier

[stas]

Reading through the code you've provided above to add a toggle for indifferent access (assuming I'm reading things right), you're relying on various rails methods/variables (mattr_accessor, with_indifferent_access). Was this the intent? I was trying to achieve the 'indifferent' result without requiring any additional gems to save on bloating this one. Of couse there's every possibility there's a way that I don't know of to use these rails features without bringing in all of ActiveSupport - see previous comment about being new to all this! :-)

Yup, you're right, in fact, attr_accessor is more than enough for the configuration, alternatively we could use the RSpec#add_setting, either way works. As for with_indifferent_access, I just assume a lot of folks might be using this gem along with the rails, so, no extra legwork is needed if the method is available...

Anyway, I was playing around with your example this morning (though with a manually created setter/getter for the toggle in the module) and as written Meta was just returning undefined method 'as_indifferent_hash' for JSONAPI::RSpec:Module.

Apologies for that, I was in a rush, please look at the code I pasted as more of a pseudocode. It's definitely not working 🙈

it { is_expected.to have_type('bar').with_indifferent_access }

Indeed, this is actually a nice idea, but I believe it would be a lot to type for some folks, so a more global option would be desired...

Thanks again for working on this! 🙇

[sa73917]

@stas - I think it's in a pretty good state to review now.

I've removed the monkey-patching and used the JSON parsing logic (though I do wonder if this will have a performance impact - hopefully not given most of these payloads are unlikely to be huge?)

I've refactored all the rspec tests so they all loop through:

• the various source document types (empty, HashWithIndifferentAccess, symbolized hash and JSON)
• the two settings for the allow_symbolized_jsonapi configuration setting
• where applicable, the various formats the parameter value(s) can be in (symbolized, stringifyed)
  This does make them a little more complicated but at least they should cover all the different combinations now (I think).

I've made the allow_symbolized_jsonapi setting impact how the source document is handled but it doesn't affect the parameter values. ie. { version: '1.0' } and { "version" => "1.0" } are both accepted by the jsonapi_object matcher regardless of the setting.

Let me know if there's anything more you think I should change.

[stas]

@sa73917 I did a quick review, aside from some cleanups, I'd like us to reduce the changeset to minimum and do not alter existing tests. There's no need for all the tests to be run twice, just add a new test where the new optional configuration is enabled, or consider adding a unit test for the new module method.

Also, please try to touch as little the existing code, overall, we're adding a new functionality and not rewriting the whole gem. Let's just keep things simple.

I'm sorry for having to ask you to reconsider some of the work you've done and thanks again for working on this 🙈 🙇

[sa73917]

@stas - finally had a chance to go through and refactor everything to split out the two changes that were in this PR.

PR #19 has the code to allow jsonapi-rspec to accept Symbol/String Parameters, and this one has the code to allow jsonapi-rspec to match against Symbol/String source documents. (obviously a rebase in one of them's future)

Hopefully a little easier to review - thanks for your patience :-)

[sa73917]

@stas - just checking in to see if there's anything you're waiting on me for? Would love to finalise this PR so I can rebase PR #19 and get that one ready for review :)

[stas]

@sa73917 this looks great. I suggested a change related to how the tests are organized. Let me know if you have time to take a look and quickly update those. Thank you! 🙇

[sa73917]

@stas I've run through and refactored all the specs as requested. Makes sense to have test of as_indifferent_hash and keep the noise out of the other tests so I've put together one that tests the various combinations. All of the existing tests are now in the same format, all include a present/not present test and where applicable include symbol and string parameters.