[JSON-API] Add support for "include" query

[janko]

I would like to get some code review on this, especially if I wrote the tests good. I didn't manage to figure out how to run Muntant for this, I ran PATTERN="Yaks::Behaviour::OptionalIncludes*" bundle exec rake mutant, but the output looked like nothing has been run.

Fixes #83

[danelowe]

typo? Should that fancy gem (ataru) have picked this up?

[janko]

Thanks, forgot about Ataru, fixed :)

[plexus]

I'm aware I'm being a bit cheeky here, but what do you think of this version 😉

require "rack/utils"

module Yaks
  module Behaviour
    module OptionalIncludes
      RACK_KEY = "yaks.optional_includes".freeze

      def associations
        super.select(&method(:include_association?))
      end

      private

      def include_association?(association)
        includes = env.fetch(RACK_KEY) do
          query_string = env.fetch("QUERY_STRING", "")
          query = Rack::Utils.parse_query(query_string)
          env[RACK_KEY] = query.fetch("include", "").split(",").map { |r| r.split(".") }
        end

        includes.any? do |relationship|
          relationship[mapper_stack.size] == association.name.to_s
        end
      end
    end
  end
end

[danelowe]

So what do we think about merging only if the key doesn't already exist in the options. That way we could always or never include a particular sub-resource simply by putting something in the config for :if

Can you see any issues with that approach?

[janko]

With the new implementation proposed by @plexus it's no longer valid to implement it this way. However, I think my original implementation had the downside of overriding the :if, and I think the user would want to keep OptionalInclude's :if and still add their own.

Now we just need to think how to specify these options. @plexus, any ideas?

[danelowe]

Nice. thanks @janko-m!

[danelowe]

Yeah as @groyoh said, I always make that typo myself.

[janko]

@plexus For me it seems dangerous to cache it in Rack env, I think it could cause threading issues. But it would be useful to somehow cache the parsing of include, although it will usually be a very short string, so if we don't find a way I think we wouldn't compromise performance.

[janko]

@plexus But I really like the rest of the changes, I will implement those.

[plexus]

For me it seems dangerous to cache it in Rack env, I think it could cause threading issues.

Each request gets a new Rack env, so even if you run multi-threaded, each request will be handled by a single thread

[janko]

You're right. Since I agree with everything you've written, I simply copy-pasted your implementation :)

[groyoh]

To fix ataru, try adding this in ataru_setup.rb at top level:

User = Struct.new

this in the Setup module:

def user
  User.new
end

and this in the README here:

yaks = Yaks.new

and run ataru locally with ataru check.

[janko]

Thanks, I didn't know how to run Ataru. Instead of adding anything to ataru_setup.rb, I reused the existing objects.

Btw, how do you run RuboCop locally? I tried bundle exec rake rubocop from both yaks/ and top-level directory, and it says that the task is missing.

[plexus]

bundle exec rake rubocop from the top level should work. Does it show up when you do bundle exec rake -T?

[janko]

Actaully, I meant to say that from the top-level it works, but it throws a lot of offenses unrelated to my files, so it doesn't seem like the same Rake task that is run on Travis.

[plexus]

😄

[groyoh]

For mutant, have your tried bundle exec rake mutant PATTERN=Yaks::Behaviour::OptionalIncludes?

[janko]

Thanks, that did work. I only had to change it to

bundle exec rake mutant -r yaks/behaviour/optional_includes PATTERN="Yaks::Behaviour::OptionalIncludes"

Because I'm not requiring that behaviour by default. I will go on fixing those.

[plexus]

With the new version users will still be able to provide their own :if, but it will be checked in addition to the includes, not instead of it. Is that good enough?

The main other case I can see is where an association is not optional, i.e. will always be included regardless of the ?include=

[groyoh]

@janko-m I think there is a require 'yaks/behaviour/optional_include' missing in yaks.rb.

[janko]

@groyoh I left it out on purpose, because I'm requiring rack/utils, and Yaks should be usable without Rack. Another option would be to require it inside a method, but I didn't want this. I don't know, what are your thoughts on that?

[plexus]

I was also thinking not to load any of yaks/behavior/ by default, just because this namespace might become quite large, and most of it will remain unused by the average user. The dependency on Rack is also a good argumnent.

[groyoh]

Ok, it makes sense! 😉 I think it should be written explicitly in the README that we should require it (and not only put it in the code block). Shouldn't we also add -r yaks/behaviour/optional_includes to mutant rake task?

[janko]

@groyoh Good call, I will edit the README. Well, I'm not sure about Mutant, if we add another behaviour then we'll need to change again. Is there a better way?

[groyoh]

@janko-m actually I do not really have a better solution. It does not look like a huge trade off for me. We won't have too many behaviour files hopefully 😄
Maybe at best we could change rakelib/shared.rake like this:

requires = %w(
  -ryaks
  -ryaks/behaviour/optional_include
)
args = %w[-Ilib --use rspec --score 100] + requires + opts + [pattern]

[janko]

I have an existenial problem here. Mutant is replacing the env.fetch(RACK_KEY) with env.fetch(nil), and my tests are still passing. This is because I'm not actually testing the caching. The thing is, I don't want to test that I'm caching :), it's just a speed optimization.

So I'm in a situation where I can introduce a meaningless test just to make Mutant pass. What should I do?

[groyoh]

@janko-m Why do you say that the test is meaningless? If it's there, it should be tested IMO.

[janko]

@groyoh You're right, I added a test. I also added your suggestion for Mutant and killed living mutants.

@plexus @danelowe I implemented it this way: if :if is passed in for an association, then don't check the include query for it. I think that's the most intuitive behaviour.

[groyoh]

Wouldn't that also include it even if we have if: ->{ false } for example?

[plexus]

it would at this stage, map_associations handles evaluating the if and taking the association out

[janko]

@groyoh Yes, but this is exactly what I want. I want that if someone specifies :if, that include_association? checking is ignored. Because I want it to mean that their overriding this behaviour for that association. If someone specifies :if -> { false }, that should mean that they never want this association included (in which case of course they wouldn't even put it there).

[danelowe]

@janko-m The behaviour you described makes sense to me. I would expect that if I supply my own callback for :if, it would override all default behaviour. Would include_association? be available inside our custom callback as well?

[janko]

@danelowe Yes, include_association? will be available as an instance method on your mapper, so you can use it in :if.

[janko]

I think this is ready for merge, is there something more I need to do? 😃

[plexus]

You're lucky the docs don't get Rubocopped. Oh hey there's an idea :D

[plexus]

A few minor nitpicks. You want to fix them? I don't consider them blocking

[janko]

Managed to correct the readme comment :)

[plexus]

I saw that :) one more thing for next time you can immediately add a mention of your cool new feature in the CHANGELOG. I go through the changes before each release but it's nice if it's already mostly filled in.

Thanks for the PR! Very useful feature.

[janko]

You're welcome! Thank you guys for such a detailed code review, it's really great that we merged our opinions together, I think this really helps keep quality of changes at a high level.

[plexus]

I very much agree

[groyoh]

Good job! Keep up the good work 👍