Proposal: Create assert_response_schema test helper

[maurogeorge]

It is a common pattern to use JSON Schema to validate a API response 1, 2
and 3.

This patch creates the assert_response_schema test helper that helps people do
this kind of validation easily on the controller tests.

This makes sense to you guys? If make sense I can continue with CHANGELOG, Docs and so on.

[bf4]

Wow, this is awesome! I think I made a issue to add this.. specifically for json-api, but in general is also great!

I have a few comments on implementation, but am generally wow!

[maurogeorge]

Hi @bf4 great to hear that 😃
I added some docs, now waiting for your review.

[bf4]

should be under a test namespace if it's for tests, e.g. ActiveModelSerializers::Test::Schema

And would be good for the method not to be dependent on where it's included

e.g.

module Schema
  module_function

  def validate!(schema_full_path, json, strict = true)
    JSON::Validator.validate!(schema_full_path, json, strict: strict) # is this keyword args? we can't support that yet
  end

  def validate_response_schema!(response, schema_path = nil)
    validate!(schema_full_path_for(response.body, schema_path))
  end

  def schema_full_path_for(response, schema_path = nil)
        controller_path = response.request.filtered_parameters[:controller]
        action = response.request.filtered_parameters[:action]
        schema_directory = ActiveModel::Serializer.config.schema_path
        schema_path ||= "#{controller_path}/#{action}.json"
        "#{schema_directory}/#{schema_path}"
  end

[bf4]

Sorry for delay in review this. There'll be a good rebase ...

[bf4]

ref: #1162

[maurogeorge]

Thanks for the review, I think it is better hold the working on this until the #1291 be merged, since both define instrumentation and the last one is more mature.

First I will work on the #1291 until the merge and later I back to this one.

[bf4]

wanna take another look at this? I like this PR

[maurogeorge]

I think it is better hold the working on this until the #1322 be merged, since both define instrumentation and the last one is more mature.

[bf4]

instrumentation PR is merged!

[malandrina]

This is a cool idea!

I wanted to chime in and share some thoughts about compatibility with other JSON Schema-based tools. Something we discovered recently with json_matchers at thoughtbot is that json_matchers currently isn’t compatible with Heroku’s JSON Schema-based tools, e.g. prmd and committee, because it makes different assumptions about the organization and structure of the schemata.

It’s likely you’ll run into the same issue with the implementation you have here. If you’re interested in ensuring compatibility with Heroku’s tooling you should consider using json_schema to parse and validate schemata and organize the schemata according to Heroku’s conventions.

[maurogeorge]

@malandrina thanks a lot for you thoughts here ❤️

I missed this on README of json-schema is said that this does not handle any JSON Hyper-Schema and prmd uses JSON Hyper-Schema.
The json_schema you recommended handle this. So yes I will change this to use json_schema.

When you said it makes different assumptions about the organization and structure of the schemata, the recommended way to organize the individual resource schema is under docs/schema/schemata. I created a option to define the path here so if we change this to ActiveModel::Serializer.config.schema_path = 'docs/schema/schemata' we can use the same json files to the prmd and comittee right?

Thanks

[malandrina]

@maurogeorge no problem!

Based on my experience with this json_matchers issue (apologies for not having referenced it directly in my previous comment), I'm not confident that making the root path for the schemata configurable is enough to achieve compatibility with Heroku's tools. I believe users of ActiveModelSerializers will have to structure their schemata according to Heroku's conventions (e.g. including required metadata and links) in addition to storing the schema files in docs/schema/schemata.

[maurogeorge]

@malandrina I updated to use the json_schema. To test the heroku integration I create a hyper-schema using the prmd and test against it. So now it is possible to do some simple validation using a simple json or using the hyper-schema format.

With this I think the users can structure their schemata according to Heroku's conventions and storing the schema files in docs/schema/schemata. You think this is ok or I miss something.

Thanks

[malandrina]

@maurogeorge your process seems solid. 👍 The only other I thing I would suggest thinking about is use of the $ref keyword and JSON Pointers in the simple (i.e. non-hyper) schemata.

If you would like for users of the matcher to be able to use JSON Pointers in non-hyper schemata, I'd suggesting writing a test or two to validate your assumptions about how JSON Pointers should be formatted and how json_schema dereferences them. This could be as simple as adding a couple $refs to test/support/schemas/custom/show.json.

If you don't expect users to include JSON Pointers in non-hyper schemata, I'd recommend including that information in the documentation.

[bf4]

I noticed that the hana gem includes a json pointer implementation (which is pretty simple) https://github.com/tenderlove/hana/blob/5da3b986cd529c680c96aacf20fc7f3c223d8102/lib/hana.rb#L4-L37 compared to https://github.com/tent/json-pointer-ruby/blob/ade5fea30acd9c85c3886db62e83d68e3b5919bb/lib/json-pointer.rb#L3-L220

[bf4]

This is an end-user's guide to testing, right? (vs. in CONTRIBUTING.md how to develop on AMS).

In any case, this is 💯

[maurogeorge]

@bf4 thanks! Yes, this is the idea.

[maurogeorge]

@malandrina thanks for the tip, I updated the code maurogeorge@0852b49 do you think this is enough?

Thanks

[bf4]

@maurogeorge Looks like CI wants you to rebase off of master and force push

This PR is so awesome. I love looking at it!

[maurogeorge]

@bf4 Thanks for the review.

I updated the code following your comments. We are back to green after drop Ruby 1.9 on Travis.

The AppVeyor is broken because we are running Ruby 1.9.3 on it, I can't found a dot file to change this, I imagine this can be managed via some interface, so we need to change this.

Thanks

[maurogeorge]

@bf4 I sent a PR to fix AppVeyor the #1395 I found for a dot file, but it is not a dot file is appveyor.yml only 😄

[maurogeorge]

@bf4 I updated the code following your last comments and rebased against the master.

We are good to ?

Thanks

[bf4]

Looks good but I'm tired right now and want to review more carefully. Did I miss why json_schema is a runtime dependency instead of development?

@malandrina thoughts?

cc @rails-api/ams this is a good one to look at

[maurogeorge]

@bf4 thanks, no problem take a rest 😃

I think if we keep the json_schema as a development dependency we will need to ask for people to install this, before use the test helper.

[bf4]

Oh right. We could warn inside a method like rails attr_encrypted so rails doesn't need to specify bcrypt

B mobile phone

On Jan 6, 2016, at 3:09 PM, Mauro George [email protected] wrote:

@bf4 thanks, no problem take a rest

I think if we keep the json_schema as a development dependency we will need to ask for people to install this, before use the test helper.

—
Reply to this email directly or view it on GitHub.

[malandrina]

Grammar: "a example" => "an example"

[malandrina]

I left some comments with suggested improvements to the documentation but overall this seems reasonable to me. It's great that this will be compatible with Heroku's tooling. 👍

[maurogeorge]

@bf4 Liked the idea, I update the code following the same idea Rails does.

@malandrina thanks for the review, I updated the docs accordingly.

[bf4]

@maurogeorge rebase and good to merge, I think. Wanna join the commiters team?

[maurogeorge]

@bf4 rebased,

😊 Yeah, I like the idea to join the commiters team. Probably I will leave the open source world for a while, but later I can continue working on the project.

[bf4]

@maurogeorge awesome. I'll have @joaomdmoura add you. (I thought I could, but I guess not.)

[joaomdmoura]

@maurogeorge You are invited! ❤️ would be nice to have you on slack so that I can add you on the maintainer channel 😄

[maurogeorge]

@bf4 @joaomdmoura thanks ❤️

João as I said in the #1270 (comment), I will leave the open source world for a while, but later I can continue working on the project and help more the project.

@bf4 ?

[bf4]

Merged by ac13053

[maurogeorge]

Thanks for the merge 👍