Align have_relationship matcher with readme and add support for Symbol or String parameters

[sa73917]

What is the current behavior?

The current README file provides an example usage for the matcher of

• expect(document['data']).to have_relationships(:posts, :comments) With the current implementation of have_relationships, this fails as it does not to_s the relationship array values provided.

PR #15 was rejected as it only added Symbol parameters to the relationships matcher without updating all matchers.

This PR includes support for symbol parameters in all matchers

What is the new behavior?

All matchers now accept strings or symbols (and string or symbol keyed hashes where applicable) Relationships matcher now has a spec to test it.

Checklist

Please make sure the following requirements are complete:

• [X] Tests for the changes have been added (for bug fixes / features)
• [X] Docs have been reviewed and added / updated if needed (for bug fixes / features)
• [X] All automated checks pass (CI/CD)

[stas]

Superseded by #18

[stas]

@sa73917 I went ahead and cherry-picked your work. Unfortunately I had to revert the changes you did on the tests, please review the relevant commit and let me know if you have questions why the rest of the work was not included b6085bc24ee0dd0d0db83387be4002f917c6973b

Thanks again for helping with this.

[sa73917]

@stas - just looking at what eventually went into master - the aim of this PR was to allow all the matchers to accept parameters that are strings or symbols. (as in it's current state, some accept symbols (have_attribute, have_relationship) and some don't (have_type). In my code I covered all the matchers but my main driver was allowing the have_type matcher to accept symbols. It looks like after your changes before the merge this logic was removed. Was this by design or an oversight?

[stas]

@sa73917 I'm taking a second look and I don't think I follow.

Mainly, it does look like have_attributeand have_relationship are allowing strings/symbols and are not strict, which I agree should not be allowed. But I don't see how this PR solves this.

I'll be happy to merge the fix if you can open another PR that does not allow symbols and strings to match attributes and relationships in the same time though.

Sorry for missing out on the initial issue that was raised. :see_no_evil:

[sa73917]

Sounds like your change to the code in my PR was intentional then (which is unfortunate IMO).

Whilst I understand a true JSON document can't have symbols in it, at the end of the day this is a Ruby gem that helps test JSON documents so (at least from my point of view) it should allow ruby developers to use it much like they would expect to interact with the vast majority of ruby gems (and in fact most standard ruby methods and objects)

Clearly personal opinion but I'd much prefer to read:

expect(document[:data]).to have_type(:users)

rather than:

expect(document['data']).to have_type('users')

My expectation when passing symbols into the matcher is (obviously) not that the type attribute will have a symbol as its value (as that would be impossible). I'm just expecting the matcher will include a "to_s" on their expected type. Eg.

module JSONAPI
  module RSpec
    module Type
      ::RSpec::Matchers.define :have_type do |expected|
        match do |actual|
          JSONAPI::RSpec.as_indifferent_hash(actual)['type'] == expected.to_s
        end
      end
    end
  end
end

Oh well, if you change your mind and would like a PR that ensures all the matchers will accept symbols, I'm more than happy to do so. I'll respectfully decline the reverse however, as I'm loath to remove the symbol parameter functionality from the matchers that already accept them - I'd have to change a bunch of my specs into a format I much less prefer to read. :-)

[stas]

I think we should make it clear that without jsonapi_indifferent_hash the matchers will be dumb and match documents only based on the user implementation. For example, if you are testing hashes with symbols as keys, and trying to match against a string key, tests won't pass.

And only if you set up jsonapi_indifferent_hash = true the above will ignored, such that the hashes can match either with a string or a symbol.

I think the default behaviour is valid only for folks who really want to encourage a specific way of formatting the tests/jsonapi-documents, but for the most of the users, jsonapi_indifferent_hash = true should be more than enough.

Let me know if that makes sense. And I would definitely not mind updating those two matchers that force the string-based matching.

Thank you for clarifying this @sa73917!

[sa73917]

No worries - always hard to understand intent in text isn't it! :-)

If we are to go down the path of the the jsonapi_indifferent_hash configuration flag being the driver for this, then we should update the existing matchers to reflect this change too.. So with it true - it's whatever combination of strings and symbols you feel like (in the source document and matcher parameters).. with it false, it's strings only everywhere..

Is that what you're thinking?

[stas]

Is that what you're thinking?

Nope. What I'm trying to say is that... If you're trying to use the matchers on a document like this with strings:

{
  data: {
    attributes: {
      id: "1",
      email: "my@domain.tld"
    }
  }
}

it should fail, and succeed on symbols (since obviously the document uses symbols (likewise with the opposite: string keys, and trying to match with symbol names).

If you have enabled the jsonapi_indifferent_hash it will pass since we don't care what keys are being used.

Basically, I don't want us to alter the document/matcher arguments, unless we are explicitly told jsonapi_indifferent_hash = true.

[sa73917]

I think we're in violent agreement on the true setting

with jsonapi_indifferent_hash set to true - it should work with whatever combination of symbols and strings you feel like throwing at it. Symbolized or string source document matching happily with symbolized or string based parameters.

On the false setting though - my thought was assume the source must be a valid JSON document and hence parameters must be strings. Sounds like you'd like to leave source and parameters alone and just compare directly. So if it's a symbolized source document, it should only match symbolized parameters. If it's a string source document it should only match string parameters.

Do I have it now? :)

[stas]

On the false setting though - my thought was assume the source must be a valid JSON document and hence parameters must be strings. Sounds like you'd like to leave source and parameters alone and just compare directly. So if it's a symbolized source document, it should only match symbolized parameters. If it's a string source document it should only match string parameters.

I'm afraid this can still be confusing since we'd have to convert to strings for have_meta as well (see #17 for example). So I really believe we should not touch/alter the original document or matcher arguments or folks won't like us at all! :see_no_evil:

[sa73917]

Rather than go back and forth trying to describe this - here's a branch with my suggested change. At a high level, it basically means each matcher calls as_indifferent_hash on the user supplied keys and (for the value matchers), on the user supplied values.

(I did think it might be worth having a jsonapi_indifferent_parameters configuration option (and related as_indifferent_param) method so the users can have control of whether source, parameters or both are processed to be indifferent but figured it's a minor change from where this branch is if you think its worth it..

[stas]

@sa73917 I went ahead and made it clear that we will always convert matcher arguments where JSON:API spec dictates the format.

I'm considering this conversation resolved for now. Thank you for helping with it!