Skip to content

Commit 6bf00fa

Browse files
author
Lee Richmond
committed
Remove Sideload#to_hash
This hash of all possible sideloads could get huge when dealing with recursive tree structures. Instead, we'll validate the requested sideloads at runtime instead of comparing against all possible sideloads. This moves "what are the possible sideloads" logic to jsonapi_swagger_helpers, and "prevent non-whitelisted sideloads" to the Query object, both of which simplify the internals of this project and improve performance. There is one breaking change - instead of ```ruby jsonapi resource: MyResource do sideload_whitelist({ index: [:foo] }) end ``` This now (correctly) moves to the controller: ```ruby jsonapi resource: MyResource sideload_whitelist({ index: [:foo] }) ```
1 parent f7053e4 commit 6bf00fa

File tree

13 files changed

+156
-341
lines changed

13 files changed

+156
-341
lines changed

lib/jsonapi_compliable/base.rb

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,13 @@ module Base
77

88
included do
99
class << self
10-
attr_accessor :_jsonapi_compliable
10+
attr_accessor :_jsonapi_compliable, :_sideload_whitelist
1111
end
1212

1313
def self.inherited(klass)
1414
super
1515
klass._jsonapi_compliable = Class.new(_jsonapi_compliable)
16+
klass._sideload_whitelist = _sideload_whitelist.dup if _sideload_whitelist
1617
end
1718
end
1819

@@ -52,6 +53,47 @@ def jsonapi(foo = 'bar', resource: nil, &blk)
5253

5354
self._jsonapi_compliable.class_eval(&blk) if blk
5455
end
56+
57+
# Set the sideload whitelist. You may want to omit sideloads for
58+
# security or performance reasons.
59+
#
60+
# Uses JSONAPI::IncludeDirective from {{http://jsonapi-rb.org jsonapi-rb}}
61+
#
62+
# @example Whitelisting Relationships
63+
# # Given the following whitelist
64+
# class PostsController < ApplicationResource
65+
# jsonapi resource: MyResource
66+
#
67+
# sideload_whitelist({
68+
# index: [:blog],
69+
# show: [:blog, { comments: :author }]
70+
# })
71+
#
72+
# # ... code ...
73+
# end
74+
#
75+
# # A request to sideload 'tags'
76+
# #
77+
# # GET /posts/1?include=tags
78+
# #
79+
# # ...will silently fail.
80+
# #
81+
# # A request for comments and tags:
82+
# #
83+
# # GET /posts/1?include=tags,comments
84+
# #
85+
# # ...will only sideload comments
86+
#
87+
# @param [Hash, Array, Symbol] whitelist
88+
# @see Query#include_hash
89+
def sideload_whitelist(hash)
90+
self._sideload_whitelist = JSONAPI::IncludeDirective.new(hash).to_hash
91+
end
92+
end
93+
94+
# @api private
95+
def sideload_whitelist
96+
self.class._sideload_whitelist || {}
5597
end
5698

5799
# Returns an instance of the associated Resource

lib/jsonapi_compliable/query.rb

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -49,12 +49,15 @@ def include_directive
4949
# @return [Hash] the scrubbed include directive as a hash
5050
def include_hash
5151
@include_hash ||= begin
52-
requested = include_directive.to_hash
53-
all_allowed = resource.sideloading.to_hash[:base]
54-
whitelist = resource.sideload_whitelist.values.reduce(:merge)
55-
allowed = whitelist ? Util::IncludeParams.scrub(all_allowed, whitelist) : all_allowed
52+
requested = include_directive.to_hash
5653

57-
Util::IncludeParams.scrub(requested, allowed)
54+
whitelist = nil
55+
if resource.context
56+
whitelist = resource.context.sideload_whitelist
57+
whitelist = whitelist[resource.context_namespace] if whitelist
58+
end
59+
60+
whitelist ? Util::IncludeParams.scrub(requested, whitelist) : requested
5861
end
5962
end
6063

lib/jsonapi_compliable/resource.rb

Lines changed: 2 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -132,36 +132,6 @@ def self.sideloading
132132
@sideloading ||= Sideload.new(:base, resource: self)
133133
end
134134

135-
# Set the sideload whitelist. You may want to omit sideloads for
136-
# security or performance reasons.
137-
#
138-
# Uses JSONAPI::IncludeDirective from {{http://jsonapi-rb.org jsonapi-rb}}
139-
#
140-
# @example Whitelisting Relationships
141-
# # Given the following whitelist
142-
# class PostResource < ApplicationResource
143-
# # ... code ...
144-
# sideload_whitelist([:blog, { comments: :author }])
145-
# end
146-
#
147-
# # A request to sideload 'tags'
148-
# #
149-
# # GET /posts?include=tags
150-
# #
151-
# # ...will silently fail.
152-
# #
153-
# # A request for comments and tags:
154-
# #
155-
# # GET /posts?include=tags,comments
156-
# #
157-
# # ...will only sideload comments
158-
#
159-
# @param [Hash, Array, Symbol] whitelist
160-
# @see Query#include_hash
161-
def self.sideload_whitelist(whitelist)
162-
config[:sideload_whitelist] = JSONAPI::IncludeDirective.new(whitelist).to_hash
163-
end
164-
165135
# Whitelist a filter
166136
#
167137
# @example Basic Filtering
@@ -413,7 +383,6 @@ def self.default_page_size(val)
413383
def self.config
414384
@config ||= begin
415385
{
416-
sideload_whitelist: {},
417386
filters: {},
418387
default_filters: {},
419388
extra_fields: {},
@@ -568,60 +537,9 @@ def persist_with_relationships(meta, attributes, relationships, caller_model = n
568537
persistence.run
569538
end
570539

571-
# All possible sideload names, including nested names
572-
#
573-
# { comments: { author: {} } }
574-
#
575-
# Becomes
576-
#
577-
# [:comments, :author]
578-
#
579-
# @see Sideload#to_hash
580-
# @return [Array<Symbol>] the list of association names
540+
# @see Sideload#association_names
581541
def association_names
582-
@association_names ||= begin
583-
if sideloading
584-
Util::Hash.keys(sideloading.to_hash[:base])
585-
else
586-
[]
587-
end
588-
end
589-
end
590-
591-
# An Include Directive Hash of all possible sideloads for the current
592-
# context namespace, taking into account the sideload whitelist.
593-
#
594-
# In other words, say we have this resource:
595-
#
596-
# class PostResource < ApplicationResource
597-
# sideload_whitelist({
598-
# index: :comments,
599-
# show: { comments: :author }
600-
# })
601-
# end
602-
#
603-
# Expected behavior:
604-
#
605-
# allowed_sideloads(:index) # => { comments: {} }
606-
# allowed_sideloads(:show) # => { comments: { author: {} }
607-
#
608-
# instance.with_context({}, :index) do
609-
# instance.allowed_sideloads # => { comments: {} }
610-
# end
611-
#
612-
# @see Util::IncludeParams.scrub
613-
# @see #with_context
614-
# @param [Symbol] namespace Can be :index/:show/etc - The current context namespace will be used by default.
615-
# @return [Hash] the scrubbed include directive
616-
def allowed_sideloads(namespace = nil)
617-
return {} unless sideloading
618-
619-
namespace ||= context_namespace
620-
sideloads = sideloading.to_hash[:base]
621-
if !sideload_whitelist.empty? && namespace
622-
sideloads = Util::IncludeParams.scrub(sideloads, sideload_whitelist[namespace])
623-
end
624-
sideloads
542+
sideloading.association_names
625543
end
626544

627545
# The relevant proc for the given attribute and calculation.
@@ -710,12 +628,6 @@ def extra_fields
710628
self.class.config[:extra_fields]
711629
end
712630

713-
# @see .sideload_whitelist
714-
# @api private
715-
def sideload_whitelist
716-
self.class.config[:sideload_whitelist]
717-
end
718-
719631
# @see .default_filter
720632
# @api private
721633
def default_filters

lib/jsonapi_compliable/scope.rb

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,10 +80,8 @@ def sideload(results, includes)
8080
return if results == []
8181

8282
includes.each_pair do |name, nested|
83-
if @resource.allowed_sideloads.has_key?(name)
84-
sideload = @resource.sideload(name)
85-
sideload.resolve(results, @query)
86-
end
83+
sideload = @resource.sideload(name)
84+
sideload.resolve(results, @query)
8785
end
8886
end
8987

lib/jsonapi_compliable/sideload.rb

Lines changed: 7 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -359,44 +359,15 @@ def all_sideloads
359359
end
360360
end
361361

362-
# Looks at all nested sideload, and all nested sideloads for the
363-
# corresponding Resources, and returns an Include Directive hash
364-
#
365-
# For instance, this configuration:
366-
#
367-
# class BarResource < ApplicationResource
368-
# allow_sideload :baz do
369-
# end
370-
# end
371-
#
372-
# class PostResource < ApplicationResource
373-
# allow_sideload :foo do
374-
# allow_sideload :bar, resource: BarResource do
375-
# end
376-
# end
377-
# end
378-
#
379-
# +post_resource.sideloading.to_hash+ would return
380-
#
381-
# { base: { foo: { bar: { baz: {} } } } }
382-
#
383-
# @return [Hash] The nested include hash
384-
# @api private
385-
def to_hash(recursion_chain = [], parent = nil)
386-
recursing = ->(arr) { arr == [parent.object_id, self.object_id] }
387-
recursions = recursion_chain.select(&recursing).length
388-
return {} if recursions >= self.class.max_recursion
389-
390-
unless (parent && parent.name == :base) || name == :base
391-
recursion_chain += [[parent.object_id, self.object_id]]
392-
end
393-
394-
{ name => {} }.tap do |hash|
395-
all_sideloads.each_pair do |key, sl|
396-
sideload_hash = sl.to_hash(recursion_chain, self)
397-
hash[name].merge!(sideload_hash)
362+
def association_names(memo = [])
363+
all_sideloads.each_pair do |name, sl|
364+
unless memo.include?(sl.name)
365+
memo << sl.name
366+
memo |= sl.association_names(memo)
398367
end
399368
end
369+
370+
memo
400371
end
401372

402373
# @api private
@@ -417,14 +388,6 @@ def fire_hooks!(parent, objects, method)
417388

418389
private
419390

420-
def nested_sideload_hash(sideload, processed)
421-
{}.tap do |hash|
422-
if sideloading = sideload.resource_class.sideloading
423-
hash.merge!(sideloading.to_hash(processed)[:base])
424-
end
425-
end
426-
end
427-
428391
def polymorphic_grouper(grouping_field)
429392
lambda do |record|
430393
if record.is_a?(Hash)

spec/extra_fields_spec.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ def include_foo!
5959
end
6060

6161
it 'does not include the extra field in the response' do
62-
ctx = double(allow_net_worth?: false)
62+
ctx = double(allow_net_worth?: false).as_null_object
6363
resource.with_context ctx do
6464
expect(json['data'][0]['attributes'].keys).to match_array(%w(first_name last_name))
6565
end

spec/fields_spec.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ def json
3737

3838
it 'disallows fields guarded by :if, even if specified' do
3939
params[:fields] = { authors: 'first_name,salary' }
40-
ctx = double(current_user: 'non-admin')
40+
ctx = double(current_user: 'non-admin').as_null_object
4141
resource.with_context ctx do
4242
expect(json['data'][0]['attributes'].keys).to_not include('salary')
4343
end

spec/filtering_spec.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@
9292

9393
context 'when the filter is guarded' do
9494
let(:can_filter) { true }
95-
let(:ctx) { double(can_filter_first_name?: can_filter) }
95+
let(:ctx) { double(can_filter_first_name?: can_filter).as_null_object }
9696

9797
before do
9898
params[:filter] = { first_name_guarded: 'Stephen' }
Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
if ENV["APPRAISAL_INITIALIZED"]
2+
require 'rails_spec_helper'
3+
4+
RSpec.describe 'integrated resources and adapters', type: :controller do
5+
let(:genre_resource) do
6+
Class.new(JsonapiCompliable::Resource) do
7+
type :genres
8+
use_adapter JsonapiCompliable::Adapters::ActiveRecord
9+
end
10+
end
11+
12+
let(:book_resource) do
13+
Class.new(JsonapiCompliable::Resource) do
14+
type :books
15+
use_adapter JsonapiCompliable::Adapters::ActiveRecord
16+
allow_filter :id
17+
18+
belongs_to :genre,
19+
scope: -> { Genre.all },
20+
foreign_key: :genre_id,
21+
resource: GenreResource
22+
end
23+
end
24+
25+
let(:author_resource) do
26+
Class.new(JsonapiCompliable::Resource) do
27+
type :authors
28+
use_adapter JsonapiCompliable::Adapters::ActiveRecord
29+
30+
has_many :books,
31+
scope: -> { Book.all },
32+
foreign_key: :author_id,
33+
resource: BookResource
34+
end
35+
end
36+
37+
before do
38+
stub_const('GenreResource', genre_resource)
39+
stub_const('BookResource', book_resource)
40+
stub_const('AuthorResource', author_resource)
41+
42+
controller.class.jsonapi resource: AuthorResource
43+
end
44+
45+
controller(ApplicationController) do
46+
def index
47+
render_jsonapi(Author.all)
48+
end
49+
end
50+
51+
def json
52+
JSON.parse(response.body)
53+
end
54+
55+
def json_includes(type)
56+
json['included'].select { |i| i['type'] == type }
57+
end
58+
59+
let!(:author) { Author.create!(first_name: 'Stephen') }
60+
let!(:book) { Book.create!(title: 'The Shining', author: author, genre: genre) }
61+
let!(:genre) { Genre.create!(name: 'Horror') }
62+
63+
context 'when no sideload whitelist' do
64+
it 'allows loading all relationships' do
65+
get :index, params: { include: 'books.genre' }
66+
expect(json_includes('books')).to_not be_blank
67+
expect(json_includes('genres')).to_not be_blank
68+
end
69+
end
70+
71+
context 'when a sideload whitelist' do
72+
before do
73+
controller.class.sideload_whitelist({
74+
index: [:books],
75+
show: { books: :genre }
76+
})
77+
end
78+
79+
it 'restricts what sideloads can be loaded' do
80+
get :index, params: { include: 'books.genre' }
81+
expect(json_includes('books')).to_not be_blank
82+
expect(json_includes('genres')).to be_blank
83+
end
84+
end
85+
end
86+
end

0 commit comments

Comments
 (0)