From bcb98289cc83fc12c0029d02b4c2ff8381ce32e7 Mon Sep 17 00:00:00 2001 From: Lee Richmond Date: Sat, 30 Mar 2019 11:52:58 -0400 Subject: [PATCH] Improve remote resource behavior * Previously, we'd disallow all writes. Now we allow associating to an existing remote entity (only applies to `belongs_to). * Previously, we'd avoid links for remote resources. Now, we avoid link validation (because we can't validate a remote API), but still generate the links (you can of course always turn this off). * As part of the last point, improved foreign key inferrence. We usually infer FK via the model name, but for remote resources the model is OpenStruct so we should use the resource name instead. --- lib/graphiti/resource/configuration.rb | 6 ++ lib/graphiti/resource/remote.rb | 8 ++- lib/graphiti/schema.rb | 2 + lib/graphiti/sideload.rb | 1 - lib/graphiti/sideload/belongs_to.rb | 13 ++++- lib/graphiti/util/serializer_relationships.rb | 2 + spec/filtering_spec.rb | 4 +- spec/remote_resource_spec.rb | 30 ++++++++-- spec/schema_diff_spec.rb | 5 +- spec/schema_spec.rb | 12 +++- spec/serialization_spec.rb | 57 +++++++++++++++++-- spec/sideload_spec.rb | 53 +++++++++++++++++ 12 files changed, 175 insertions(+), 18 deletions(-) diff --git a/lib/graphiti/resource/configuration.rb b/lib/graphiti/resource/configuration.rb index 47f8928c..472c56e2 100644 --- a/lib/graphiti/resource/configuration.rb +++ b/lib/graphiti/resource/configuration.rb @@ -38,6 +38,12 @@ def adapter=(val) def remote=(val) super include ::Graphiti::Resource::Remote + self.endpoint = { + path: val, + full_path: val, + url: val, + actions: [:index, :show], + } end def model diff --git a/lib/graphiti/resource/remote.rb b/lib/graphiti/resource/remote.rb index a3d1c084..0939340b 100644 --- a/lib/graphiti/resource/remote.rb +++ b/lib/graphiti/resource/remote.rb @@ -18,8 +18,12 @@ def remote_url end end - def save(*args) - raise Errors::RemoteWrite.new(self.class) + def save(model, meta) + if meta[:attributes] == {} && meta[:method] == :update + model + else + raise Errors::RemoteWrite.new(self.class) + end end def destroy(*args) diff --git a/lib/graphiti/schema.rb b/lib/graphiti/schema.rb index f19cd20f..d0d0ff47 100644 --- a/lib/graphiti/schema.rb +++ b/lib/graphiti/schema.rb @@ -49,6 +49,8 @@ def generate_types def generate_endpoints {}.tap do |endpoints| @resources.each do |r| + next if r.remote? + r.endpoints.each do |e| actions = {} e[:actions].each do |a| diff --git a/lib/graphiti/sideload.rb b/lib/graphiti/sideload.rb index 1b7800d6..2bff857b 100644 --- a/lib/graphiti/sideload.rb +++ b/lib/graphiti/sideload.rb @@ -47,7 +47,6 @@ def initialize(name, opts) end if remote? - @link = false @resource_class = create_remote_resource end end diff --git a/lib/graphiti/sideload/belongs_to.rb b/lib/graphiti/sideload/belongs_to.rb index 12aa1960..3566e935 100644 --- a/lib/graphiti/sideload/belongs_to.rb +++ b/lib/graphiti/sideload/belongs_to.rb @@ -23,8 +23,17 @@ def ids_for_parents(parents) end def infer_foreign_key - if polymorphic_child? - parent.foreign_key + return parent.foreign_key if polymorphic_child? + + if resource.remote? + namespace = namespace_for(resource.class) + resource_name = resource.class.name + .gsub("#{namespace}::", "") + .gsub("Resource", "") + if resource_name.include?(".remote") + resource_name = resource_name.split(".remote")[0].split(".")[1] + end + :"#{resource_name.singularize.underscore}_id" else model = resource.model namespace = namespace_for(model) diff --git a/lib/graphiti/util/serializer_relationships.rb b/lib/graphiti/util/serializer_relationships.rb index 74e7bffa..9d8433b1 100644 --- a/lib/graphiti/util/serializer_relationships.rb +++ b/lib/graphiti/util/serializer_relationships.rb @@ -109,6 +109,8 @@ def validate_link! end def validate_link_for_sideload!(sideload) + return if sideload.resource.remote? + action = sideload.type == :belongs_to ? :show : :index cache_key = :"#{@sideload.object_id}-#{action}" return if self.class.validated_link_cache.include?(cache_key) diff --git a/spec/filtering_spec.rb b/spec/filtering_spec.rb index 23094415..e86983e1 100644 --- a/spec/filtering_spec.rb +++ b/spec/filtering_spec.rb @@ -395,7 +395,7 @@ def self.name end context "when allow list is omitted" do - context 'when using a string_enum field' do + context "when using a string_enum field" do it "raises an error at load time" do expect { resource.filter :enum_first_name, :string_enum do @@ -408,7 +408,7 @@ def self.name end end - context 'when using an integer_enum field' do + context "when using an integer_enum field" do it "raises an error at load time" do expect { resource.filter :enum_age, :integer_enum do diff --git a/spec/remote_resource_spec.rb b/spec/remote_resource_spec.rb index 9dbdc802..78cab5bf 100644 --- a/spec/remote_resource_spec.rb +++ b/spec/remote_resource_spec.rb @@ -777,10 +777,32 @@ def self.name end context "when updating" do - it "raises error" do - expect { - klass.find(id: 1, data: {type: "employees"}).update_attributes - }.to raise_error(Graphiti::Errors::RemoteWrite, /not supported/) + let(:payload) do + { + data: { + type: "employees", + id: "123", + attributes: {last_name: "Jane"}, + }, + } + end + + context "and only associating to a remote parent" do + before do + payload[:data].delete(:attributes) + end + + it "works" do + klass.find(payload).update_attributes + end + end + + context "and passing more attributes than simple association" do + it "raises error" do + expect { + klass.find(payload).update_attributes + }.to raise_error(Graphiti::Errors::RemoteWrite, /not supported/) + end end end diff --git a/spec/schema_diff_spec.rb b/spec/schema_diff_spec.rb index 3dfb7964..7945f7d0 100644 --- a/spec/schema_diff_spec.rb +++ b/spec/schema_diff_spec.rb @@ -126,8 +126,9 @@ def self.name end end - it "does not diff" do - expect(diff).to eq([]) + it "notes only the newly-missing missing endpoint" do + expect(diff) + .to eq(["Endpoint \"/schema_diff/employees\" was removed."]) end end diff --git a/spec/schema_spec.rb b/spec/schema_spec.rb index f9516fc5..191657d5 100644 --- a/spec/schema_spec.rb +++ b/spec/schema_spec.rb @@ -91,7 +91,7 @@ }, integer_enum: { description: "Integer enum type. Like a normal integer, but only eq/!eq filters. Limited to only the allowed values.", - kind: "scalar" + kind: "scalar", }, integer: { kind: "scalar", @@ -590,6 +590,16 @@ def self.name end end + context "when a resource is remote" do + before do + employee_resource.remote = "http://foo.com/employees" + end + + it "does not add endpoints to the schema" do + expect(schema[:endpoints]).to eq({}) + end + end + context "when 2 resources, same path" do let(:employee_search_resource) do Class.new(application_resource) do diff --git a/spec/serialization_spec.rb b/spec/serialization_spec.rb index 7af5ee85..162e354f 100644 --- a/spec/serialization_spec.rb +++ b/spec/serialization_spec.rb @@ -1029,6 +1029,18 @@ def positions .to eq("/poro/positions?filter[employee_id]=1") end + context "that is remote" do + before do + resource.has_many :positions, remote: "http://foo.com/positions" + end + + it "links correctly" do + render + expect(positions["links"]["related"]) + .to eq("http://foo.com/positions?filter[employee_id]=1") + end + end + context "opting-out of linking" do before do resource.has_many :positions, link: false @@ -1159,6 +1171,24 @@ def define_relationship .to eq("/poro/mastercards/789") end + context "that is remote" do + before do + resource.polymorphic_belongs_to :credit_card do + group_by(:credit_card_type) do + on(:Mastercard).belongs_to :mastercard, + remote: "http://foo.com/mastercards" + end + end + end + + it "links correctly" do + render + credit_card = json["data"][0]["relationships"]["credit_card"] + expect(credit_card["links"]["related"]) + .to eq("http://foo.com/mastercards/789") + end + end + context "but one child does not have an endpoint" do before do mastercard_resource.endpoint = nil @@ -1212,15 +1242,24 @@ def define_relationship end context "and a has_one relationship" do - before do - resource.has_one :bio - end - it "links to index endpoint" do + resource.has_one :bio render expect(json["data"][0]["relationships"]["bio"]["links"]["related"]) .to eq("/poro/bios?filter[employee_id]=#{employee.id}") end + + context "that is remote" do + before do + resource.has_one :bio, remote: "http://foo.com/bios" + end + + it "links correctly" do + render + expect(json["data"][0]["relationships"]["bio"]["links"]["related"]) + .to eq("http://foo.com/bios?filter[employee_id]=#{employee.id}") + end + end end context "and a belongs_to relationship" do @@ -1305,6 +1344,16 @@ def classification xit "links correctly" do end end + + context "when remote" do + it "links correctly" do + resource.belongs_to :classification, + remote: "http://foo.com/classifications" + render + expect(classification["links"]["related"]) + .to eq("http://foo.com/classifications/789") + end + end end end end diff --git a/spec/sideload_spec.rb b/spec/sideload_spec.rb index 3a5e6baf..684e8b5d 100644 --- a/spec/sideload_spec.rb +++ b/spec/sideload_spec.rb @@ -208,6 +208,59 @@ class SideloadSpecEmployee expect(instance.infer_foreign_key).to eq(:sideload_spec_employee_id) end end + + context "when the resource is remote" do + let(:name) { "positions" } + + context "via the sideload :remote option" do + it "is inferred correctly from the parent resource" do + opts.delete(:resource) + opts[:remote] = "http://foo.com/positions" + expect(instance.infer_foreign_key).to eq(:employee_id) + end + + context "and belongs_to" do + let(:instance) { Class.new(Graphiti::Sideload::BelongsTo).new(name, opts) } + + before do + opts[:type] = :belongs_to + end + + it "works" do + opts.delete(:resource) + opts[:remote] = "http://foo.com/positions" + expect(instance.infer_foreign_key).to eq(:position_id) + end + end + end + + context "via resource class" do + before do + opts[:resource] = Class.new(Graphiti::Resource) do + self.remote = "http://foo.com" + def self.name + "PORO::PositionResource" + end + end + end + + it "is inferred correctly from the parent resource" do + expect(instance.infer_foreign_key).to eq(:employee_id) + end + + context "and belongs_to" do + let(:instance) { Class.new(Graphiti::Sideload::BelongsTo).new(name, opts) } + + before do + opts[:type] = :belongs_to + end + + it "works" do + expect(instance.infer_foreign_key).to eq(:position_id) + end + end + end + end end describe "#ids_for_parents" do