[JSON-API] Add support for "include" query #95
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
require "rack/utils" | ||
|
||
module Yaks | ||
module Behaviour | ||
module OptionalIncludes | ||
RACK_KEY = "yaks.optional_includes".freeze | ||
|
||
def associations | ||
super.select do |association| | ||
association.if != Undefined || include_association?(association) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't that also include it even if we have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it would at this stage, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @groyoh Yes, but this is exactly what I want. I want that if someone specifies |
||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Can you see any issues with that approach? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Now we just need to think how to specify these options. @plexus, any ideas? |
||
end | ||
|
||
private | ||
|
||
def include_association?(association) | ||
includes = env.fetch(RACK_KEY) do | ||
query_string = env.fetch("QUERY_STRING", nil) | ||
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].eql?(association.name.to_s) | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
require "yaks/behaviour/optional_includes" | ||
|
||
RSpec.describe Yaks::Behaviour::OptionalIncludes do | ||
include_context 'yaks context' | ||
|
||
subject(:mapper) { mapper_class.new(yaks_context) } | ||
let(:resource) { mapper.call(instance) } | ||
|
||
let(:mapper_class) do | ||
Class.new(Yaks::Mapper).tap do |mapper_class| | ||
mapper_class.send :include, Yaks::Behaviour::OptionalIncludes | ||
mapper_class.type "user" | ||
mapper_class.has_many :posts, mapper: post_mapper_class | ||
mapper_class.has_one :account, mapper: account_mapper_class | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should also work with just passing the class body to Class.new(Yaks::Mapper) do
include Yaks::Behaviour::OptionalIncludes
type "user"
has_many :posts, mapper: post_mapper_class
has_one :account, mapper: account_mapper_class
end There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I almost would, but then There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah ok, then it's fine. I was wondering if there was a reason |
||
end | ||
let(:post_mapper_class) do | ||
Class.new(Yaks::Mapper).tap do |mapper_class| | ||
mapper_class.type "post" | ||
mapper_class.has_many :comments, mapper: comment_mapper_class | ||
end | ||
end | ||
let(:account_mapper_class) { Class.new(Yaks::Mapper) { type "account" } } | ||
let(:comment_mapper_class) { Class.new(Yaks::Mapper) { type "comment" } } | ||
|
||
let(:instance) { fake(posts: [fake(comments: [fake])], account: fake) } | ||
|
||
it "includes the associations" do | ||
rack_env["QUERY_STRING"] = "include=posts.comments,account" | ||
|
||
expect(resource.type).to eq "user" | ||
expect(resource.subresources[0].type).to eq "post" | ||
expect(resource.subresources[0].members[0].type).to eq "post" | ||
expect(resource.subresources[0].members[0].subresources[0].type).to eq "comment" | ||
expect(resource.subresources[0].members[0].subresources[0].members[0].type).to eq "comment" | ||
expect(resource.subresources[1].type).to eq "account" | ||
end | ||
|
||
it "excludes associations not specified in the QUERY_STRING" do | ||
rack_env["QUERY_STRING"] = "include=posts" | ||
|
||
expect(resource.subresources.count).to eq 1 | ||
end | ||
|
||
it "doesn't include the associations when QUERY_STRING is empty" do | ||
expect(resource.type).to eq "user" | ||
expect(resource.subresources).to be_empty | ||
end | ||
|
||
it "allows :if to override the query parameter checking" do | ||
mapper_class.has_one :account, mapper: account_mapper_class, if: true | ||
|
||
expect(resource.subresources.count).to eq 1 | ||
end | ||
|
||
it "caches parsing of the query parameter" do | ||
rack_env["QUERY_STRING"] = "include=posts" | ||
expect(mapper.call(instance).subresources.count).to eq 1 | ||
|
||
rack_env["QUERY_STRING"] = nil | ||
expect(mapper.call(instance).subresources.count).to eq 1 | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To fix ataru, try adding this in
ataru_setup.rb
at top level:this in the
Setup
module:and this in the README here:
and run ataru locally with
ataru check
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 bothyaks/
and top-level directory, and it says that the task is missing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bundle exec rake rubocop
from the top level should work. Does it show up when you dobundle exec rake -T
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.