From 02f97ca6bba9cb5bbefefa7b197ab08d0cb538f5 Mon Sep 17 00:00:00 2001 From: Justin Cypret Date: Sat, 4 Nov 2017 00:00:45 -0500 Subject: [PATCH 1/2] Fix fallback for find_by_hashid methods This existing implementation was falling back to an ID lookup if unable to decode hashid. It should instead return nil or raise an not found exception. --- lib/hashid/rails.rb | 21 ++++++++++------ spec/hashid/rails_spec.rb | 51 +++++++++++++++++++++++++++++++++------ 2 files changed, 56 insertions(+), 16 deletions(-) diff --git a/lib/hashid/rails.rb b/lib/hashid/rails.rb index dbf1901..b327d65 100644 --- a/lib/hashid/rails.rb +++ b/lib/hashid/rails.rb @@ -52,11 +52,14 @@ def encode_id(ids) end end - def decode_id(ids) + # @param ids [String, Integer, Array] id(s) to decode. + # @param fallback [Boolean] indicates whether to return the passed in + # id(s) if unable to decode or if already decoded. + def decode_id(ids, fallback: false) if ids.is_a?(Array) - ids.map { |id| hashid_decode(id) } + ids.map { |id| hashid_decode(id, fallback: fallback) } else - hashid_decode(ids) + hashid_decode(ids, fallback: fallback) end end @@ -67,18 +70,18 @@ def find(*ids) uniq_ids = uniq_ids.first unless expects_array || uniq_ids.size > 1 if Hashid::Rails.configuration.override_find - super(decode_id(uniq_ids)) + super(decode_id(uniq_ids, fallback: true)) else super end end def find_by_hashid(hashid) - find_by(id: decode_id(hashid)) + find_by(id: decode_id(hashid, fallback: false)) end def find_by_hashid!(hashid) - find_by!(id: decode_id(hashid)) + find_by!(id: decode_id(hashid, fallback: false)) end private @@ -95,10 +98,12 @@ def hashid_encode(id) end end - def hashid_decode(id) + def hashid_decode(id, fallback:) decoded_hashid = hashids.decode(id.to_s) + fallback = fallback ? id : nil + if Hashid::Rails.configuration.sign_hashids - valid_hashid?(decoded_hashid) ? decoded_hashid.last : id + valid_hashid?(decoded_hashid) ? decoded_hashid.last : fallback else decoded_hashid.first end diff --git a/spec/hashid/rails_spec.rb b/spec/hashid/rails_spec.rb index 4e83911..eda40a5 100644 --- a/spec/hashid/rails_spec.rb +++ b/spec/hashid/rails_spec.rb @@ -106,8 +106,13 @@ expect(decoded_id).to eq(100_117) end - it "returns already decoded id" do - decoded_id = FakeModel.decode_id(100_117) + it "returns nil when already decoded id" do + decoded_id = FakeModel.decode_id(100_117, fallback: false) + expect(decoded_id).to eq(nil) + end + + it "returns already decoded id when fallback" do + decoded_id = FakeModel.decode_id(100_117, fallback: true) expect(decoded_id).to eq(100_117) end end @@ -118,8 +123,13 @@ expect(decoded_ids).to eq([1]) end - it "returns array with already decoded id" do - decoded_ids = FakeModel.decode_id([1]) + it "returns array with nil when already decoded id" do + decoded_ids = FakeModel.decode_id([1], fallback: false) + expect(decoded_ids).to eq([nil]) + end + + it "returns array with already decoded id when fallback" do + decoded_ids = FakeModel.decode_id([1], fallback: true) expect(decoded_ids).to eq([1]) end end @@ -130,8 +140,13 @@ expect(decoded_ids).to eq([1, 2, 3]) end - it "returns array of already decoded ids" do - decoded_ids = FakeModel.decode_id([1, 2, 3]) + it "returns array of nil when already decoded ids" do + decoded_ids = FakeModel.decode_id([1, 2, 3], fallback: false) + expect(decoded_ids).to eq([nil, nil, nil]) + end + + it "returns array of already decoded ids when fallback" do + decoded_ids = FakeModel.decode_id([1, 2, 3], fallback: true) expect(decoded_ids).to eq([1, 2, 3]) end end @@ -244,7 +259,12 @@ end it "does not try and decode regular ids" do - decoded_id = FakeModel.decode_id(100_117) + decoded_id = FakeModel.decode_id(100_117, fallback: false) + expect(decoded_id).to eq(nil) + end + + it "does not try and decode regular ids when fallback" do + decoded_id = FakeModel.decode_id(100_117, fallback: true) expect(decoded_id).to eq(100_117) end @@ -294,6 +314,14 @@ expect(result).to eq(nil) end + + it "returns nil for non-hashid" do + model = FakeModel.create! + + result = FakeModel.find_by_hashid(model.id) + + expect(result).to eq(nil) + end end describe ".find_by_hashid!" do @@ -305,10 +333,17 @@ expect(result).to eq(model) end - it "returns nil when unable to find model" do + it "raises record not found when unable to find model" do expect { FakeModel.find_by_hashid!("ABC") } .to raise_error(ActiveRecord::RecordNotFound) end + + it "raises record not found for non-hashid" do + model = FakeModel.create! + + expect { FakeModel.find_by_hashid!(model.id) } + .to raise_error(ActiveRecord::RecordNotFound) + end end describe ".configure" do From 4388f020539d14894eb59f5e70ce97ecff2949c7 Mon Sep 17 00:00:00 2001 From: Justin Cypret Date: Fri, 17 Nov 2017 20:51:17 -0600 Subject: [PATCH 2/2] Set fallback value to new var rather than overriding --- lib/hashid/rails.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/hashid/rails.rb b/lib/hashid/rails.rb index b327d65..cc549c5 100644 --- a/lib/hashid/rails.rb +++ b/lib/hashid/rails.rb @@ -100,10 +100,10 @@ def hashid_encode(id) def hashid_decode(id, fallback:) decoded_hashid = hashids.decode(id.to_s) - fallback = fallback ? id : nil + fallback_value = fallback ? id : nil if Hashid::Rails.configuration.sign_hashids - valid_hashid?(decoded_hashid) ? decoded_hashid.last : fallback + valid_hashid?(decoded_hashid) ? decoded_hashid.last : fallback_value else decoded_hashid.first end