From a83c1cd236f9c84015d2cacdfc021b6276a57a4f Mon Sep 17 00:00:00 2001 From: Alex Burgel Date: Fri, 22 Nov 2019 14:52:34 -0500 Subject: [PATCH 1/9] Initial pass --- .../v2/legacy_split_registries_controller.rb | 7 +++++++ .../api/v2/split_registries_controller.rb | 14 +++++++++++++- app/models/build_timestamp.rb | 19 +++++++++++++++++++ app/models/split_registry.rb | 8 ++++++-- .../show.json.jbuilder | 10 ++++++++++ config/routes.rb | 3 ++- ...egacy_split_registries_controller_spec.rb} | 2 +- spec/models/split_registry_spec.rb | 2 +- 8 files changed, 59 insertions(+), 6 deletions(-) create mode 100644 app/controllers/api/v2/legacy_split_registries_controller.rb create mode 100644 app/models/build_timestamp.rb create mode 100644 app/views/api/v2/legacy_split_registries/show.json.jbuilder rename spec/controllers/api/v2/{split_registries_controller_spec.rb => legacy_split_registries_controller_spec.rb} (94%) diff --git a/app/controllers/api/v2/legacy_split_registries_controller.rb b/app/controllers/api/v2/legacy_split_registries_controller.rb new file mode 100644 index 00000000..41d20c2a --- /dev/null +++ b/app/controllers/api/v2/legacy_split_registries_controller.rb @@ -0,0 +1,7 @@ +class Api::V2::LegacySplitRegistriesController < UnauthenticatedApiController + include CorsSupport + + def show + @split_registry = SplitRegistry.new(Time.zone.now) + end +end diff --git a/app/controllers/api/v2/split_registries_controller.rb b/app/controllers/api/v2/split_registries_controller.rb index d5ababc3..b5e58653 100644 --- a/app/controllers/api/v2/split_registries_controller.rb +++ b/app/controllers/api/v2/split_registries_controller.rb @@ -2,6 +2,18 @@ class Api::V2::SplitRegistriesController < UnauthenticatedApiController include CorsSupport def show - @split_registry = SplitRegistry.instance + build_timestamp = BuildTimestamp.new(build_params) + if build_timestamp.valid? + @split_registry = SplitRegistry.new(build_timestamp) + else + render_errors build_timestamp + end + + end + + private + + def build_params + params.permit(:timestamp) end end diff --git a/app/models/build_timestamp.rb b/app/models/build_timestamp.rb new file mode 100644 index 00000000..6b93b922 --- /dev/null +++ b/app/models/build_timestamp.rb @@ -0,0 +1,19 @@ +class BuildTimestamp + include ActiveModel::Validations + + attr_reader :build_timestamp + + validates :build_timestamp, presence: true + # We want to make sure the client is shipping us high-precision ISO + # timestamps so we choose to allow timestamps at either millisecond or second + # precision based on the W3C interpretation of iso8601 from this SO answer: + # + # https://stackoverflow.com/a/3143231 + validates :build_timestamp, format: { + with: /\A\d{4}-[01]\d-[0-3]\dT[0-2]\d:[0-5]\d:[0-5]\d(.\d+)?([+-][0-2]\d:[0-5]\d|Z)\z/, allow_blank: true + } + + def initialize(params) + @build_timestamp = params[:timestamp] + end +end diff --git a/app/models/split_registry.rb b/app/models/split_registry.rb index db94b026..a1381ea4 100644 --- a/app/models/split_registry.rb +++ b/app/models/split_registry.rb @@ -1,8 +1,10 @@ class SplitRegistry - include Singleton + def initialize(build_timestamp) + @build_timestamp = build_timestamp + end def splits - Split.for_presentation + Split.active(as_of: build_timestamp.build_timestamp) end def experience_sampling_weight @@ -11,6 +13,8 @@ def experience_sampling_weight private + attr_reader :build_timestamp + def _experience_sampling_weight Integer(ENV.fetch('EXPERIENCE_SAMPLING_WEIGHT', '1')).tap do |weight| raise <<~TEXT if weight.negative? diff --git a/app/views/api/v2/legacy_split_registries/show.json.jbuilder b/app/views/api/v2/legacy_split_registries/show.json.jbuilder new file mode 100644 index 00000000..44d2b71b --- /dev/null +++ b/app/views/api/v2/legacy_split_registries/show.json.jbuilder @@ -0,0 +1,10 @@ +json.splits do + @split_registry.splits.each do |split| + json.set! split.name do + json.weights split.registry + json.feature_gate split.feature_gate? + end + end + json.merge!({}) +end +json.(@split_registry, :experience_sampling_weight) diff --git a/config/routes.rb b/config/routes.rb index e419206e..2869979e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -50,7 +50,8 @@ end namespace :v2 do - resource :split_registry, only: :show + resource :split_registry, only: :show, controller: 'legacy_split_registries' + resources :split_registries, only: :show, param: :timestamp resources :migrations do collection do diff --git a/spec/controllers/api/v2/split_registries_controller_spec.rb b/spec/controllers/api/v2/legacy_split_registries_controller_spec.rb similarity index 94% rename from spec/controllers/api/v2/split_registries_controller_spec.rb rename to spec/controllers/api/v2/legacy_split_registries_controller_spec.rb index bae36a2c..09086cbc 100644 --- a/spec/controllers/api/v2/split_registries_controller_spec.rb +++ b/spec/controllers/api/v2/legacy_split_registries_controller_spec.rb @@ -1,6 +1,6 @@ require 'rails_helper' -RSpec.describe Api::V2::SplitRegistriesController, type: :controller do +RSpec.describe Api::V2::LegacySplitRegistriesController, type: :controller do let(:split_1) { FactoryBot.create :split, name: "one", finished_at: Time.zone.now, registry: { all: 100 } } let(:split_2) { FactoryBot.create :split, name: "two", registry: { on: 50, off: 50 } } let(:split_3) { FactoryBot.create :split, name: "three_enabled", registry: { true: 99, false: 1 }, feature_gate: true } diff --git a/spec/models/split_registry_spec.rb b/spec/models/split_registry_spec.rb index 3f12edbb..7382c51d 100644 --- a/spec/models/split_registry_spec.rb +++ b/spec/models/split_registry_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' RSpec.describe SplitRegistry do - subject { described_class.instance } + subject { described_class.new(Time.zone.now) } describe "#splits" do it "doesn't cache the instance" do From 0e45a36fb0c30eaedb61f70ea8a678cae72bda02 Mon Sep 17 00:00:00 2001 From: Andrew Freeman Date: Wed, 4 Dec 2019 15:19:46 -0500 Subject: [PATCH 2/9] add specs --- .../api/v2/split_registries_controller.rb | 3 +- app/models/split_registry.rb | 2 +- ...legacy_split_registries_controller_spec.rb | 2 +- .../v2/split_registries_controller_spec.rb | 53 +++++++++++++++++++ spec/models/build_timestamp_spec.rb | 23 ++++++++ spec/models/split_registry_spec.rb | 14 +++-- 6 files changed, 88 insertions(+), 9 deletions(-) create mode 100644 spec/controllers/api/v2/split_registries_controller_spec.rb create mode 100644 spec/models/build_timestamp_spec.rb diff --git a/app/controllers/api/v2/split_registries_controller.rb b/app/controllers/api/v2/split_registries_controller.rb index b5e58653..ca776595 100644 --- a/app/controllers/api/v2/split_registries_controller.rb +++ b/app/controllers/api/v2/split_registries_controller.rb @@ -4,11 +4,10 @@ class Api::V2::SplitRegistriesController < UnauthenticatedApiController def show build_timestamp = BuildTimestamp.new(build_params) if build_timestamp.valid? - @split_registry = SplitRegistry.new(build_timestamp) + @split_registry = SplitRegistry.new(build_timestamp.build_timestamp) else render_errors build_timestamp end - end private diff --git a/app/models/split_registry.rb b/app/models/split_registry.rb index a1381ea4..e7c22d41 100644 --- a/app/models/split_registry.rb +++ b/app/models/split_registry.rb @@ -4,7 +4,7 @@ def initialize(build_timestamp) end def splits - Split.active(as_of: build_timestamp.build_timestamp) + Split.active(as_of: build_timestamp) end def experience_sampling_weight diff --git a/spec/controllers/api/v2/legacy_split_registries_controller_spec.rb b/spec/controllers/api/v2/legacy_split_registries_controller_spec.rb index 09086cbc..b4b4aa71 100644 --- a/spec/controllers/api/v2/legacy_split_registries_controller_spec.rb +++ b/spec/controllers/api/v2/legacy_split_registries_controller_spec.rb @@ -7,7 +7,7 @@ describe "#show" do before do - allow(SplitRegistry.instance).to receive(:experience_sampling_weight).and_return(10) + allow(ENV).to receive(:fetch).with('EXPERIENCE_SAMPLING_WEIGHT', any_args).and_return(10) end it "includes sampling weight" do diff --git a/spec/controllers/api/v2/split_registries_controller_spec.rb b/spec/controllers/api/v2/split_registries_controller_spec.rb new file mode 100644 index 00000000..f9b85832 --- /dev/null +++ b/spec/controllers/api/v2/split_registries_controller_spec.rb @@ -0,0 +1,53 @@ +require 'rails_helper' + +RSpec.describe Api::V2::SplitRegistriesController, type: :controller do + let(:split_1) { FactoryBot.create :split, name: "one", finished_at: Time.zone.parse('2019-11-13'), registry: { all: 100 } } + let(:split_2) { FactoryBot.create :split, name: "two", registry: { on: 50, off: 50 } } + let(:split_3) { FactoryBot.create :split, name: "three_enabled", registry: { true: 99, false: 1 }, feature_gate: true } + + describe "#show" do + before do + allow(ENV).to receive(:fetch).with('EXPERIENCE_SAMPLING_WEIGHT', any_args).and_return(10) + end + + it "includes sampling weight" do + get :show, params: { timestamp: '2019-11-11T14:35:30Z' } + + expect(response).to have_http_status :ok + expect(response_json['experience_sampling_weight']).to eq(10) + end + + it "returns empty with no active splits on the timestamp" do + expect(split_1).to be_finished + + get :show, params: { timestamp: '2019-11-14T14:35:30Z' } + + expect(response).to have_http_status :ok + expect(response_json['splits']).to eq({}) + end + + it "returns the full split registry of splits that are active during timestamp" do + expect(split_1).to be_finished + expect(split_2).not_to be_finished + expect(split_3).not_to be_finished + + get :show, params: { timestamp: '2019-11-12T14:35:30Z' } + + expect(response).to have_http_status :ok + expect(response_json['splits']).to eq( + "one" => { + "weights" => { "all" => 100 }, + "feature_gate" => false + }, + "two" => { + "weights" => { "on" => 50, "off" => 50 }, + "feature_gate" => false + }, + "three_enabled" => { + "weights" => { "true" => 99, "false" => 1 }, + "feature_gate" => true + } + ) + end + end +end diff --git a/spec/models/build_timestamp_spec.rb b/spec/models/build_timestamp_spec.rb new file mode 100644 index 00000000..702fddb8 --- /dev/null +++ b/spec/models/build_timestamp_spec.rb @@ -0,0 +1,23 @@ +require 'rails_helper' + +describe BuildTimestamp do + it "is valid with valid args" do + expect(described_class.new(timestamp: "2019-04-16T14:35:30Z")).to be_valid + end + + it "is invalid with no build timestamp" do + expect(described_class.new(timestamp: "")).to be_invalid + end + + it "is invalid with a non-ISO date" do + expect(described_class.new(timestamp: "2019-04-16 10:38:08 -0400")).to be_invalid + end + + it "is valid with an ISO date with millis" do + expect(described_class.new(timestamp: "2019-04-16T14:35:30.123Z")).to be_valid + end + + it "is invalid with an ISO date without seconds" do + expect(described_class.new(timestamp: "2019-04-16T14:35Z")).to be_invalid + end +end diff --git a/spec/models/split_registry_spec.rb b/spec/models/split_registry_spec.rb index 7382c51d..54b69950 100644 --- a/spec/models/split_registry_spec.rb +++ b/spec/models/split_registry_spec.rb @@ -9,23 +9,27 @@ expect(subject.splits).not_to eql(subject.splits) end - it "returns active splits" do + it "returns active splits as of provided timestamp" do split = FactoryBot.create(:split) expect(subject.splits.all).to include(split) end - it "doesn't return inactive splits" do - split = FactoryBot.create(:split, finished_at: Time.zone.now) + it "doesn't return inactive splits as of given timestamp" do + split = FactoryBot.create(:split, finished_at: 1.day.ago) expect(subject.splits.all).not_to include(split) end + + it "returns splits that were retired after the given timestamp" do + split = FactoryBot.create(:split, finished_at: Time.zone.now) + + expect(described_class.new(1.day.ago).splits.all).to include(split) + end end describe "#experience_sampling_weight" do context "bypassing singleton memoization" do - subject { described_class.send(:new) } - it "memoizes the env var fetch" do allow(ENV).to receive(:fetch).and_call_original From 1e7a8699911c6c5d86c3337c6801a73c6901ff7b Mon Sep 17 00:00:00 2001 From: Andrew Freeman Date: Fri, 13 Dec 2019 15:57:29 -0500 Subject: [PATCH 3/9] split_registry_snapshots After discussion we decided to rename this endpoint and fold the build_timestamp validation logic into SplitRegistry. Besides making this more of a true resource route, it explains what this endpoint is retunring much better, and consolidates code. Now the SplitRegistrySnapshot class accepts a timestamp and returns to you the split registry as of that time. So this change will be backwards compatifble as we just need to pass it `Time.zone.now` --- .../v2/legacy_split_registries_controller.rb | 7 ---- .../api/v2/split_registries_controller.rb | 13 +------ .../v2/split_registry_snapshots_controller.rb | 19 ++++++++++ app/models/build_timestamp.rb | 19 ---------- app/models/split_registry.rb | 25 ------------- app/models/split_registry_snapshot.rb | 37 +++++++++++++++++++ .../show.json.jbuilder | 4 +- config/routes.rb | 4 +- .../v2/split_registries_controller_spec.rb | 20 +++------- ...lit_registry_snapshots_controller_spec.rb} | 34 +++++++++++++---- spec/models/build_timestamp_spec.rb | 23 ------------ ...pec.rb => split_registry_snapshot_spec.rb} | 28 ++++++++++++-- 12 files changed, 119 insertions(+), 114 deletions(-) delete mode 100644 app/controllers/api/v2/legacy_split_registries_controller.rb create mode 100644 app/controllers/api/v2/split_registry_snapshots_controller.rb delete mode 100644 app/models/build_timestamp.rb delete mode 100644 app/models/split_registry.rb create mode 100644 app/models/split_registry_snapshot.rb rename app/views/api/v2/{legacy_split_registries => split_registry_snapshots}/show.json.jbuilder (58%) rename spec/controllers/api/v2/{legacy_split_registries_controller_spec.rb => split_registry_snapshots_controller_spec.rb} (53%) delete mode 100644 spec/models/build_timestamp_spec.rb rename spec/models/{split_registry_spec.rb => split_registry_snapshot_spec.rb} (67%) diff --git a/app/controllers/api/v2/legacy_split_registries_controller.rb b/app/controllers/api/v2/legacy_split_registries_controller.rb deleted file mode 100644 index 41d20c2a..00000000 --- a/app/controllers/api/v2/legacy_split_registries_controller.rb +++ /dev/null @@ -1,7 +0,0 @@ -class Api::V2::LegacySplitRegistriesController < UnauthenticatedApiController - include CorsSupport - - def show - @split_registry = SplitRegistry.new(Time.zone.now) - end -end diff --git a/app/controllers/api/v2/split_registries_controller.rb b/app/controllers/api/v2/split_registries_controller.rb index ca776595..796e2b29 100644 --- a/app/controllers/api/v2/split_registries_controller.rb +++ b/app/controllers/api/v2/split_registries_controller.rb @@ -2,17 +2,6 @@ class Api::V2::SplitRegistriesController < UnauthenticatedApiController include CorsSupport def show - build_timestamp = BuildTimestamp.new(build_params) - if build_timestamp.valid? - @split_registry = SplitRegistry.new(build_timestamp.build_timestamp) - else - render_errors build_timestamp - end - end - - private - - def build_params - params.permit(:timestamp) + @split_registry = SplitRegistrySnapshot.new(timestamp: Time.zone.now) end end diff --git a/app/controllers/api/v2/split_registry_snapshots_controller.rb b/app/controllers/api/v2/split_registry_snapshots_controller.rb new file mode 100644 index 00000000..36986634 --- /dev/null +++ b/app/controllers/api/v2/split_registry_snapshots_controller.rb @@ -0,0 +1,19 @@ +class Api::V2::SplitRegistrySnapshotsController < UnauthenticatedApiController + include CorsSupport + + def show + snapshot = SplitRegistrySnapshot.new(timestamp: snapshot_params[:timestamp]) + + if snapshot.valid? + @split_registry_snapshot = snapshot + else + render_errors snapshot + end + end + + private + + def snapshot_params + params.permit(:timestamp) + end +end diff --git a/app/models/build_timestamp.rb b/app/models/build_timestamp.rb deleted file mode 100644 index 6b93b922..00000000 --- a/app/models/build_timestamp.rb +++ /dev/null @@ -1,19 +0,0 @@ -class BuildTimestamp - include ActiveModel::Validations - - attr_reader :build_timestamp - - validates :build_timestamp, presence: true - # We want to make sure the client is shipping us high-precision ISO - # timestamps so we choose to allow timestamps at either millisecond or second - # precision based on the W3C interpretation of iso8601 from this SO answer: - # - # https://stackoverflow.com/a/3143231 - validates :build_timestamp, format: { - with: /\A\d{4}-[01]\d-[0-3]\dT[0-2]\d:[0-5]\d:[0-5]\d(.\d+)?([+-][0-2]\d:[0-5]\d|Z)\z/, allow_blank: true - } - - def initialize(params) - @build_timestamp = params[:timestamp] - end -end diff --git a/app/models/split_registry.rb b/app/models/split_registry.rb deleted file mode 100644 index e7c22d41..00000000 --- a/app/models/split_registry.rb +++ /dev/null @@ -1,25 +0,0 @@ -class SplitRegistry - def initialize(build_timestamp) - @build_timestamp = build_timestamp - end - - def splits - Split.active(as_of: build_timestamp) - end - - def experience_sampling_weight - @experience_sampling_weight ||= _experience_sampling_weight - end - - private - - attr_reader :build_timestamp - - def _experience_sampling_weight - Integer(ENV.fetch('EXPERIENCE_SAMPLING_WEIGHT', '1')).tap do |weight| - raise <<~TEXT if weight.negative? - EXPERIENCE_SAMPLING_WEIGHT, if specified, must be greater than or equal to 0. Use 0 to disable experience events. - TEXT - end - end -end diff --git a/app/models/split_registry_snapshot.rb b/app/models/split_registry_snapshot.rb new file mode 100644 index 00000000..be4091bc --- /dev/null +++ b/app/models/split_registry_snapshot.rb @@ -0,0 +1,37 @@ +class SplitRegistrySnapshot + include ActiveModel::Validations + + validates :timestamp, presence: true + # We want to make sure the client is shipping us high-precision ISO + # timestamps so we choose to allow timestamps at either millisecond or second + # precision based on the W3C interpretation of iso8601 from this SO answer: + # + # https://stackoverflow.com/a/3143231 + validates :timestamp, format: { + with: /\A\d{4}-[01]\d-[0-3]\dT[0-2]\d:[0-5]\d:[0-5]\d(.\d+)?([+-][0-2]\d:[0-5]\d|Z)\z/, allow_blank: true + } + + def initialize(timestamp:) + @timestamp = timestamp + end + + def splits + Split.active(as_of: timestamp) + end + + def experience_sampling_weight + @experience_sampling_weight ||= _experience_sampling_weight + end + + private + + attr_reader :timestamp + + def _experience_sampling_weight + Integer(ENV.fetch('EXPERIENCE_SAMPLING_WEIGHT', '1')).tap do |weight| + raise <<~TEXT if weight.negative? + EXPERIENCE_SAMPLING_WEIGHT, if specified, must be greater than or equal to 0. Use 0 to disable experience events. + TEXT + end + end +end diff --git a/app/views/api/v2/legacy_split_registries/show.json.jbuilder b/app/views/api/v2/split_registry_snapshots/show.json.jbuilder similarity index 58% rename from app/views/api/v2/legacy_split_registries/show.json.jbuilder rename to app/views/api/v2/split_registry_snapshots/show.json.jbuilder index 44d2b71b..6b4fb3f2 100644 --- a/app/views/api/v2/legacy_split_registries/show.json.jbuilder +++ b/app/views/api/v2/split_registry_snapshots/show.json.jbuilder @@ -1,5 +1,5 @@ json.splits do - @split_registry.splits.each do |split| + @split_registry_snapshot.splits.each do |split| json.set! split.name do json.weights split.registry json.feature_gate split.feature_gate? @@ -7,4 +7,4 @@ json.splits do end json.merge!({}) end -json.(@split_registry, :experience_sampling_weight) +json.(@split_registry_snapshot, :experience_sampling_weight) diff --git a/config/routes.rb b/config/routes.rb index 2869979e..d7c89ef3 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -50,8 +50,8 @@ end namespace :v2 do - resource :split_registry, only: :show, controller: 'legacy_split_registries' - resources :split_registries, only: :show, param: :timestamp + resource :split_registry, only: :show + resources :split_registry_snapshots, only: :show, param: :timestamp resources :migrations do collection do diff --git a/spec/controllers/api/v2/split_registries_controller_spec.rb b/spec/controllers/api/v2/split_registries_controller_spec.rb index f9b85832..3dfefd17 100644 --- a/spec/controllers/api/v2/split_registries_controller_spec.rb +++ b/spec/controllers/api/v2/split_registries_controller_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' RSpec.describe Api::V2::SplitRegistriesController, type: :controller do - let(:split_1) { FactoryBot.create :split, name: "one", finished_at: Time.zone.parse('2019-11-13'), registry: { all: 100 } } + let(:split_1) { FactoryBot.create :split, name: "one", finished_at: Time.zone.now, registry: { all: 100 } } let(:split_2) { FactoryBot.create :split, name: "two", registry: { on: 50, off: 50 } } let(:split_3) { FactoryBot.create :split, name: "three_enabled", registry: { true: 99, false: 1 }, feature_gate: true } @@ -11,34 +11,26 @@ end it "includes sampling weight" do - get :show, params: { timestamp: '2019-11-11T14:35:30Z' } - + get :show expect(response).to have_http_status :ok expect(response_json['experience_sampling_weight']).to eq(10) end - it "returns empty with no active splits on the timestamp" do - expect(split_1).to be_finished - - get :show, params: { timestamp: '2019-11-14T14:35:30Z' } - + it "returns empty with no active splits" do + get :show expect(response).to have_http_status :ok expect(response_json['splits']).to eq({}) end - it "returns the full split registry of splits that are active during timestamp" do + it "returns the full split registry" do expect(split_1).to be_finished expect(split_2).not_to be_finished expect(split_3).not_to be_finished - get :show, params: { timestamp: '2019-11-12T14:35:30Z' } + get :show expect(response).to have_http_status :ok expect(response_json['splits']).to eq( - "one" => { - "weights" => { "all" => 100 }, - "feature_gate" => false - }, "two" => { "weights" => { "on" => 50, "off" => 50 }, "feature_gate" => false diff --git a/spec/controllers/api/v2/legacy_split_registries_controller_spec.rb b/spec/controllers/api/v2/split_registry_snapshots_controller_spec.rb similarity index 53% rename from spec/controllers/api/v2/legacy_split_registries_controller_spec.rb rename to spec/controllers/api/v2/split_registry_snapshots_controller_spec.rb index b4b4aa71..955983e0 100644 --- a/spec/controllers/api/v2/legacy_split_registries_controller_spec.rb +++ b/spec/controllers/api/v2/split_registry_snapshots_controller_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' -RSpec.describe Api::V2::LegacySplitRegistriesController, type: :controller do - let(:split_1) { FactoryBot.create :split, name: "one", finished_at: Time.zone.now, registry: { all: 100 } } +RSpec.describe Api::V2::SplitRegistrySnapshotsController, type: :controller do + let(:split_1) { FactoryBot.create :split, name: "one", finished_at: Time.zone.parse('2019-11-13'), registry: { all: 100 } } let(:split_2) { FactoryBot.create :split, name: "two", registry: { on: 50, off: 50 } } let(:split_3) { FactoryBot.create :split, name: "three_enabled", registry: { true: 99, false: 1 }, feature_gate: true } @@ -11,26 +11,34 @@ end it "includes sampling weight" do - get :show + get :show, params: { timestamp: '2019-11-11T14:35:30Z' } + expect(response).to have_http_status :ok expect(response_json['experience_sampling_weight']).to eq(10) end - it "returns empty with no active splits" do - get :show + it "returns empty with no active splits on the timestamp" do + expect(split_1).to be_finished + + get :show, params: { timestamp: '2019-11-14T14:35:30Z' } + expect(response).to have_http_status :ok expect(response_json['splits']).to eq({}) end - it "returns the full split registry" do + it "returns the full split registry of splits that are active during timestamp" do expect(split_1).to be_finished expect(split_2).not_to be_finished expect(split_3).not_to be_finished - get :show + get :show, params: { timestamp: '2019-11-12T14:35:30Z' } expect(response).to have_http_status :ok expect(response_json['splits']).to eq( + "one" => { + "weights" => { "all" => 100 }, + "feature_gate" => false + }, "two" => { "weights" => { "on" => 50, "off" => 50 }, "feature_gate" => false @@ -41,5 +49,17 @@ } ) end + + it "returns unprocessable_entity if the timestamp url param is invalid" do + get :show, params: { timestamp: "2019-04-16 10:38:08 -0400" } + + expect(response).to have_http_status :unprocessable_entity + end + + it "returns unprocessable_entity if the timestamp url param is missing" do + get :show, params: { timestamp: "" } + + expect(response).to have_http_status :unprocessable_entity + end end end diff --git a/spec/models/build_timestamp_spec.rb b/spec/models/build_timestamp_spec.rb deleted file mode 100644 index 702fddb8..00000000 --- a/spec/models/build_timestamp_spec.rb +++ /dev/null @@ -1,23 +0,0 @@ -require 'rails_helper' - -describe BuildTimestamp do - it "is valid with valid args" do - expect(described_class.new(timestamp: "2019-04-16T14:35:30Z")).to be_valid - end - - it "is invalid with no build timestamp" do - expect(described_class.new(timestamp: "")).to be_invalid - end - - it "is invalid with a non-ISO date" do - expect(described_class.new(timestamp: "2019-04-16 10:38:08 -0400")).to be_invalid - end - - it "is valid with an ISO date with millis" do - expect(described_class.new(timestamp: "2019-04-16T14:35:30.123Z")).to be_valid - end - - it "is invalid with an ISO date without seconds" do - expect(described_class.new(timestamp: "2019-04-16T14:35Z")).to be_invalid - end -end diff --git a/spec/models/split_registry_spec.rb b/spec/models/split_registry_snapshot_spec.rb similarity index 67% rename from spec/models/split_registry_spec.rb rename to spec/models/split_registry_snapshot_spec.rb index 54b69950..1e4e71d9 100644 --- a/spec/models/split_registry_spec.rb +++ b/spec/models/split_registry_snapshot_spec.rb @@ -1,7 +1,29 @@ require 'rails_helper' -RSpec.describe SplitRegistry do - subject { described_class.new(Time.zone.now) } +RSpec.describe SplitRegistrySnapshot do + subject { described_class.new(timestamp: Time.zone.now) } + + describe 'validations' do + it "is valid with valid args" do + expect(described_class.new(timestamp: "2019-04-16T14:35:30Z")).to be_valid + end + + it "is invalid with no timestamp" do + expect(described_class.new(timestamp: "")).to be_invalid + end + + it "is invalid with a non-ISO date" do + expect(described_class.new(timestamp: "2019-04-16 10:38:08 -0400")).to be_invalid + end + + it "is valid with an ISO date with millis" do + expect(described_class.new(timestamp: "2019-04-16T14:35:30.123Z")).to be_valid + end + + it "is invalid with an ISO date without seconds" do + expect(described_class.new(timestamp: "2019-04-16T14:35Z")).to be_invalid + end + end describe "#splits" do it "doesn't cache the instance" do @@ -24,7 +46,7 @@ it "returns splits that were retired after the given timestamp" do split = FactoryBot.create(:split, finished_at: Time.zone.now) - expect(described_class.new(1.day.ago).splits.all).to include(split) + expect(described_class.new(timestamp: 1.day.ago).splits.all).to include(split) end end From 490c4f7b2246c26dc226c050b4504f7666dbeee7 Mon Sep 17 00:00:00 2001 From: Andrew Freeman Date: Fri, 20 Dec 2019 15:24:24 -0500 Subject: [PATCH 4/9] v2/builds/:timestamp/split_registry --- ...ots_controller.rb => build_split_registries_controller.rb} | 2 +- config/routes.rb | 4 +++- ...ller_spec.rb => build_split_registries_controller_spec.rb} | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) rename app/controllers/api/v2/{split_registry_snapshots_controller.rb => build_split_registries_controller.rb} (79%) rename spec/controllers/api/v2/{split_registry_snapshots_controller_spec.rb => build_split_registries_controller_spec.rb} (96%) diff --git a/app/controllers/api/v2/split_registry_snapshots_controller.rb b/app/controllers/api/v2/build_split_registries_controller.rb similarity index 79% rename from app/controllers/api/v2/split_registry_snapshots_controller.rb rename to app/controllers/api/v2/build_split_registries_controller.rb index 36986634..a6b6e6a7 100644 --- a/app/controllers/api/v2/split_registry_snapshots_controller.rb +++ b/app/controllers/api/v2/build_split_registries_controller.rb @@ -1,4 +1,4 @@ -class Api::V2::SplitRegistrySnapshotsController < UnauthenticatedApiController +class Api::V2::BuildSplitRegistriesController < UnauthenticatedApiController include CorsSupport def show diff --git a/config/routes.rb b/config/routes.rb index d7c89ef3..74a59d62 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -51,7 +51,9 @@ namespace :v2 do resource :split_registry, only: :show - resources :split_registry_snapshots, only: :show, param: :timestamp + resources :builds, only: [], param: :timestamp do + resource :split_registry, only: :show, controller: 'build_split_registries' + end resources :migrations do collection do diff --git a/spec/controllers/api/v2/split_registry_snapshots_controller_spec.rb b/spec/controllers/api/v2/build_split_registries_controller_spec.rb similarity index 96% rename from spec/controllers/api/v2/split_registry_snapshots_controller_spec.rb rename to spec/controllers/api/v2/build_split_registries_controller_spec.rb index 955983e0..a8050822 100644 --- a/spec/controllers/api/v2/split_registry_snapshots_controller_spec.rb +++ b/spec/controllers/api/v2/build_split_registries_controller_spec.rb @@ -1,6 +1,7 @@ require 'rails_helper' -RSpec.describe Api::V2::SplitRegistrySnapshotsController, type: :controller do + +RSpec.describe Api::V2::BuildSplitRegistriesController, type: :controller do let(:split_1) { FactoryBot.create :split, name: "one", finished_at: Time.zone.parse('2019-11-13'), registry: { all: 100 } } let(:split_2) { FactoryBot.create :split, name: "two", registry: { on: 50, off: 50 } } let(:split_3) { FactoryBot.create :split, name: "three_enabled", registry: { true: 99, false: 1 }, feature_gate: true } From 65d0718d7c5f0c9b0fae1f70bb74e76fc27262ab Mon Sep 17 00:00:00 2001 From: Andrew Freeman Date: Fri, 20 Dec 2019 15:52:43 -0500 Subject: [PATCH 5/9] uses build_timestamp param --- .../v2/build_split_registries_controller.rb | 4 ++-- .../show.json.jbuilder | 0 .../build_split_registries_controller_spec.rb | 19 +++++++++---------- 3 files changed, 11 insertions(+), 12 deletions(-) rename app/views/api/v2/{split_registry_snapshots => build_split_registries}/show.json.jbuilder (100%) diff --git a/app/controllers/api/v2/build_split_registries_controller.rb b/app/controllers/api/v2/build_split_registries_controller.rb index a6b6e6a7..5ae4133f 100644 --- a/app/controllers/api/v2/build_split_registries_controller.rb +++ b/app/controllers/api/v2/build_split_registries_controller.rb @@ -2,7 +2,7 @@ class Api::V2::BuildSplitRegistriesController < UnauthenticatedApiController include CorsSupport def show - snapshot = SplitRegistrySnapshot.new(timestamp: snapshot_params[:timestamp]) + snapshot = SplitRegistrySnapshot.new(timestamp: snapshot_params[:build_timestamp]) if snapshot.valid? @split_registry_snapshot = snapshot @@ -14,6 +14,6 @@ def show private def snapshot_params - params.permit(:timestamp) + params.permit(:build_timestamp) end end diff --git a/app/views/api/v2/split_registry_snapshots/show.json.jbuilder b/app/views/api/v2/build_split_registries/show.json.jbuilder similarity index 100% rename from app/views/api/v2/split_registry_snapshots/show.json.jbuilder rename to app/views/api/v2/build_split_registries/show.json.jbuilder diff --git a/spec/controllers/api/v2/build_split_registries_controller_spec.rb b/spec/controllers/api/v2/build_split_registries_controller_spec.rb index a8050822..d4014955 100644 --- a/spec/controllers/api/v2/build_split_registries_controller_spec.rb +++ b/spec/controllers/api/v2/build_split_registries_controller_spec.rb @@ -1,6 +1,5 @@ require 'rails_helper' - RSpec.describe Api::V2::BuildSplitRegistriesController, type: :controller do let(:split_1) { FactoryBot.create :split, name: "one", finished_at: Time.zone.parse('2019-11-13'), registry: { all: 100 } } let(:split_2) { FactoryBot.create :split, name: "two", registry: { on: 50, off: 50 } } @@ -12,27 +11,27 @@ end it "includes sampling weight" do - get :show, params: { timestamp: '2019-11-11T14:35:30Z' } + get :show, params: { build_timestamp: '2019-11-11T14:35:30Z' } expect(response).to have_http_status :ok expect(response_json['experience_sampling_weight']).to eq(10) end - it "returns empty with no active splits on the timestamp" do + it "returns empty with no active splits on the build_timestamp" do expect(split_1).to be_finished - get :show, params: { timestamp: '2019-11-14T14:35:30Z' } + get :show, params: { build_timestamp: '2019-11-14T14:35:30Z' } expect(response).to have_http_status :ok expect(response_json['splits']).to eq({}) end - it "returns the full split registry of splits that are active during timestamp" do + it "returns the full split registry of splits that are active during build_timestamp" do expect(split_1).to be_finished expect(split_2).not_to be_finished expect(split_3).not_to be_finished - get :show, params: { timestamp: '2019-11-12T14:35:30Z' } + get :show, params: { build_timestamp: '2019-11-12T14:35:30Z' } expect(response).to have_http_status :ok expect(response_json['splits']).to eq( @@ -51,14 +50,14 @@ ) end - it "returns unprocessable_entity if the timestamp url param is invalid" do - get :show, params: { timestamp: "2019-04-16 10:38:08 -0400" } + it "returns unprocessable_entity if the build_timestamp url param is invalid" do + get :show, params: { build_timestamp: "2019-04-16 10:38:08 -0400" } expect(response).to have_http_status :unprocessable_entity end - it "returns unprocessable_entity if the timestamp url param is missing" do - get :show, params: { timestamp: "" } + it "returns unprocessable_entity if the build_timestamp url param is missing" do + get :show, params: { build_timestamp: "" } expect(response).to have_http_status :unprocessable_entity end From c584872da08d00536897d978d4507bc69396e8bc Mon Sep 17 00:00:00 2001 From: Andrew Freeman Date: Fri, 20 Dec 2019 16:07:54 -0500 Subject: [PATCH 6/9] upgrade to v3 --- .../split_registries_controller.rb} | 6 +++--- .../split_registries}/show.json.jbuilder | 0 config/routes.rb | 7 ++++--- .../split_registries_controller_spec.rb} | 20 +++++++++---------- 4 files changed, 17 insertions(+), 16 deletions(-) rename app/controllers/api/{v2/build_split_registries_controller.rb => v3/split_registries_controller.rb} (65%) rename app/views/api/{v2/build_split_registries => v3/split_registries}/show.json.jbuilder (100%) rename spec/controllers/api/{v2/build_split_registries_controller_spec.rb => v3/split_registries_controller_spec.rb} (70%) diff --git a/app/controllers/api/v2/build_split_registries_controller.rb b/app/controllers/api/v3/split_registries_controller.rb similarity index 65% rename from app/controllers/api/v2/build_split_registries_controller.rb rename to app/controllers/api/v3/split_registries_controller.rb index 5ae4133f..01200ecb 100644 --- a/app/controllers/api/v2/build_split_registries_controller.rb +++ b/app/controllers/api/v3/split_registries_controller.rb @@ -1,8 +1,8 @@ -class Api::V2::BuildSplitRegistriesController < UnauthenticatedApiController +class Api::V3::SplitRegistriesController < UnauthenticatedApiController include CorsSupport def show - snapshot = SplitRegistrySnapshot.new(timestamp: snapshot_params[:build_timestamp]) + snapshot = SplitRegistrySnapshot.new(timestamp: snapshot_params[:timestamp]) if snapshot.valid? @split_registry_snapshot = snapshot @@ -14,6 +14,6 @@ def show private def snapshot_params - params.permit(:build_timestamp) + params.permit(:timestamp) end end diff --git a/app/views/api/v2/build_split_registries/show.json.jbuilder b/app/views/api/v3/split_registries/show.json.jbuilder similarity index 100% rename from app/views/api/v2/build_split_registries/show.json.jbuilder rename to app/views/api/v3/split_registries/show.json.jbuilder diff --git a/config/routes.rb b/config/routes.rb index 74a59d62..58cfa0a4 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -51,9 +51,6 @@ namespace :v2 do resource :split_registry, only: :show - resources :builds, only: [], param: :timestamp do - resource :split_registry, only: :show, controller: 'build_split_registries' - end resources :migrations do collection do @@ -67,6 +64,10 @@ end end end + + namespace :v3 do + resource :split_registry, only: :show, param: :timestamp + end end if ENV['SAML_ISSUER'].present? diff --git a/spec/controllers/api/v2/build_split_registries_controller_spec.rb b/spec/controllers/api/v3/split_registries_controller_spec.rb similarity index 70% rename from spec/controllers/api/v2/build_split_registries_controller_spec.rb rename to spec/controllers/api/v3/split_registries_controller_spec.rb index d4014955..f6a56eee 100644 --- a/spec/controllers/api/v2/build_split_registries_controller_spec.rb +++ b/spec/controllers/api/v3/split_registries_controller_spec.rb @@ -1,6 +1,6 @@ require 'rails_helper' -RSpec.describe Api::V2::BuildSplitRegistriesController, type: :controller do +RSpec.describe Api::V3::SplitRegistriesController, type: :controller do let(:split_1) { FactoryBot.create :split, name: "one", finished_at: Time.zone.parse('2019-11-13'), registry: { all: 100 } } let(:split_2) { FactoryBot.create :split, name: "two", registry: { on: 50, off: 50 } } let(:split_3) { FactoryBot.create :split, name: "three_enabled", registry: { true: 99, false: 1 }, feature_gate: true } @@ -11,27 +11,27 @@ end it "includes sampling weight" do - get :show, params: { build_timestamp: '2019-11-11T14:35:30Z' } + get :show, params: { timestamp: '2019-11-11T14:35:30Z' } expect(response).to have_http_status :ok expect(response_json['experience_sampling_weight']).to eq(10) end - it "returns empty with no active splits on the build_timestamp" do + it "returns empty with no active splits on the timestamp" do expect(split_1).to be_finished - get :show, params: { build_timestamp: '2019-11-14T14:35:30Z' } + get :show, params: { timestamp: '2019-11-14T14:35:30Z' } expect(response).to have_http_status :ok expect(response_json['splits']).to eq({}) end - it "returns the full split registry of splits that are active during build_timestamp" do + it "returns the full split registry of splits that are active during timestamp" do expect(split_1).to be_finished expect(split_2).not_to be_finished expect(split_3).not_to be_finished - get :show, params: { build_timestamp: '2019-11-12T14:35:30Z' } + get :show, params: { timestamp: '2019-11-12T14:35:30Z' } expect(response).to have_http_status :ok expect(response_json['splits']).to eq( @@ -50,14 +50,14 @@ ) end - it "returns unprocessable_entity if the build_timestamp url param is invalid" do - get :show, params: { build_timestamp: "2019-04-16 10:38:08 -0400" } + it "returns unprocessable_entity if the timestamp url param is invalid" do + get :show, params: { timestamp: "2019-04-16 10:38:08 -0400" } expect(response).to have_http_status :unprocessable_entity end - it "returns unprocessable_entity if the build_timestamp url param is missing" do - get :show, params: { build_timestamp: "" } + it "returns unprocessable_entity if the timestamp url param is missing" do + get :show, params: { timestamp: "" } expect(response).to have_http_status :unprocessable_entity end From a0f214440ce422c879fbac00682ac230deeaad1e Mon Sep 17 00:00:00 2001 From: Andrew Freeman Date: Thu, 2 Jan 2020 12:57:52 -0500 Subject: [PATCH 7/9] combine endpoint naming approaches --- app/controllers/api/v3/split_registries_controller.rb | 4 ++-- config/routes.rb | 4 +++- .../api/v3/split_registries_controller_spec.rb | 10 +++++----- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/app/controllers/api/v3/split_registries_controller.rb b/app/controllers/api/v3/split_registries_controller.rb index 01200ecb..71d5b0ca 100644 --- a/app/controllers/api/v3/split_registries_controller.rb +++ b/app/controllers/api/v3/split_registries_controller.rb @@ -2,7 +2,7 @@ class Api::V3::SplitRegistriesController < UnauthenticatedApiController include CorsSupport def show - snapshot = SplitRegistrySnapshot.new(timestamp: snapshot_params[:timestamp]) + snapshot = SplitRegistrySnapshot.new(timestamp: snapshot_params[:build_timestamp]) if snapshot.valid? @split_registry_snapshot = snapshot @@ -14,6 +14,6 @@ def show private def snapshot_params - params.permit(:timestamp) + params.permit(:build_timestamp) end end diff --git a/config/routes.rb b/config/routes.rb index 58cfa0a4..b379bed8 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -66,7 +66,9 @@ end namespace :v3 do - resource :split_registry, only: :show, param: :timestamp + resources :builds, only: [], param: :timestamp do + resource :split_registry, only: :show, controller: 'split_registries' + end end end diff --git a/spec/controllers/api/v3/split_registries_controller_spec.rb b/spec/controllers/api/v3/split_registries_controller_spec.rb index f6a56eee..595aa87e 100644 --- a/spec/controllers/api/v3/split_registries_controller_spec.rb +++ b/spec/controllers/api/v3/split_registries_controller_spec.rb @@ -11,7 +11,7 @@ end it "includes sampling weight" do - get :show, params: { timestamp: '2019-11-11T14:35:30Z' } + get :show, params: { build_timestamp: '2019-11-11T14:35:30Z' } expect(response).to have_http_status :ok expect(response_json['experience_sampling_weight']).to eq(10) @@ -20,7 +20,7 @@ it "returns empty with no active splits on the timestamp" do expect(split_1).to be_finished - get :show, params: { timestamp: '2019-11-14T14:35:30Z' } + get :show, params: { build_timestamp: '2019-11-14T14:35:30Z' } expect(response).to have_http_status :ok expect(response_json['splits']).to eq({}) @@ -31,7 +31,7 @@ expect(split_2).not_to be_finished expect(split_3).not_to be_finished - get :show, params: { timestamp: '2019-11-12T14:35:30Z' } + get :show, params: { build_timestamp: '2019-11-12T14:35:30Z' } expect(response).to have_http_status :ok expect(response_json['splits']).to eq( @@ -51,13 +51,13 @@ end it "returns unprocessable_entity if the timestamp url param is invalid" do - get :show, params: { timestamp: "2019-04-16 10:38:08 -0400" } + get :show, params: { build_timestamp: "2019-04-16 10:38:08 -0400" } expect(response).to have_http_status :unprocessable_entity end it "returns unprocessable_entity if the timestamp url param is missing" do - get :show, params: { timestamp: "" } + get :show, params: { build_timestamp: "" } expect(response).to have_http_status :unprocessable_entity end From 09773120bc8671de21aea15dd3483be9ce904010 Mon Sep 17 00:00:00 2001 From: Andrew Freeman Date: Fri, 3 Jan 2020 11:56:03 -0500 Subject: [PATCH 8/9] remove references to timestamp and split registry snapshot --- .../api/v2/split_registries_controller.rb | 2 +- .../api/v3/split_registries_controller.rb | 10 +++++----- ...it_registry_snapshot.rb => split_registry.rb} | 14 +++++++------- .../api/v3/split_registries/show.json.jbuilder | 4 ++-- ...y_snapshot_spec.rb => split_registry_spec.rb} | 16 ++++++++-------- 5 files changed, 23 insertions(+), 23 deletions(-) rename app/models/{split_registry_snapshot.rb => split_registry.rb} (79%) rename spec/models/{split_registry_snapshot_spec.rb => split_registry_spec.rb} (79%) diff --git a/app/controllers/api/v2/split_registries_controller.rb b/app/controllers/api/v2/split_registries_controller.rb index 796e2b29..f0b07385 100644 --- a/app/controllers/api/v2/split_registries_controller.rb +++ b/app/controllers/api/v2/split_registries_controller.rb @@ -2,6 +2,6 @@ class Api::V2::SplitRegistriesController < UnauthenticatedApiController include CorsSupport def show - @split_registry = SplitRegistrySnapshot.new(timestamp: Time.zone.now) + @split_registry = SplitRegistry.new(as_of: Time.zone.now) end end diff --git a/app/controllers/api/v3/split_registries_controller.rb b/app/controllers/api/v3/split_registries_controller.rb index 71d5b0ca..097785bf 100644 --- a/app/controllers/api/v3/split_registries_controller.rb +++ b/app/controllers/api/v3/split_registries_controller.rb @@ -2,18 +2,18 @@ class Api::V3::SplitRegistriesController < UnauthenticatedApiController include CorsSupport def show - snapshot = SplitRegistrySnapshot.new(timestamp: snapshot_params[:build_timestamp]) + split_registry = SplitRegistry.new(as_of: split_registry_params[:build_timestamp]) - if snapshot.valid? - @split_registry_snapshot = snapshot + if split_registry.valid? + @split_registry = split_registry else - render_errors snapshot + render_errors split_registry end end private - def snapshot_params + def split_registry_params params.permit(:build_timestamp) end end diff --git a/app/models/split_registry_snapshot.rb b/app/models/split_registry.rb similarity index 79% rename from app/models/split_registry_snapshot.rb rename to app/models/split_registry.rb index be4091bc..8f711d9d 100644 --- a/app/models/split_registry_snapshot.rb +++ b/app/models/split_registry.rb @@ -1,22 +1,22 @@ -class SplitRegistrySnapshot +class SplitRegistry include ActiveModel::Validations - validates :timestamp, presence: true + validates :as_of, presence: true # We want to make sure the client is shipping us high-precision ISO # timestamps so we choose to allow timestamps at either millisecond or second # precision based on the W3C interpretation of iso8601 from this SO answer: # # https://stackoverflow.com/a/3143231 - validates :timestamp, format: { + validates :as_of, format: { with: /\A\d{4}-[01]\d-[0-3]\dT[0-2]\d:[0-5]\d:[0-5]\d(.\d+)?([+-][0-2]\d:[0-5]\d|Z)\z/, allow_blank: true } - def initialize(timestamp:) - @timestamp = timestamp + def initialize(as_of:) + @as_of = as_of end def splits - Split.active(as_of: timestamp) + Split.active(as_of: as_of) end def experience_sampling_weight @@ -25,7 +25,7 @@ def experience_sampling_weight private - attr_reader :timestamp + attr_reader :as_of def _experience_sampling_weight Integer(ENV.fetch('EXPERIENCE_SAMPLING_WEIGHT', '1')).tap do |weight| diff --git a/app/views/api/v3/split_registries/show.json.jbuilder b/app/views/api/v3/split_registries/show.json.jbuilder index 6b4fb3f2..44d2b71b 100644 --- a/app/views/api/v3/split_registries/show.json.jbuilder +++ b/app/views/api/v3/split_registries/show.json.jbuilder @@ -1,5 +1,5 @@ json.splits do - @split_registry_snapshot.splits.each do |split| + @split_registry.splits.each do |split| json.set! split.name do json.weights split.registry json.feature_gate split.feature_gate? @@ -7,4 +7,4 @@ json.splits do end json.merge!({}) end -json.(@split_registry_snapshot, :experience_sampling_weight) +json.(@split_registry, :experience_sampling_weight) diff --git a/spec/models/split_registry_snapshot_spec.rb b/spec/models/split_registry_spec.rb similarity index 79% rename from spec/models/split_registry_snapshot_spec.rb rename to spec/models/split_registry_spec.rb index 1e4e71d9..2057776e 100644 --- a/spec/models/split_registry_snapshot_spec.rb +++ b/spec/models/split_registry_spec.rb @@ -1,27 +1,27 @@ require 'rails_helper' -RSpec.describe SplitRegistrySnapshot do - subject { described_class.new(timestamp: Time.zone.now) } +RSpec.describe SplitRegistry do + subject { described_class.new(as_of: Time.zone.now) } describe 'validations' do it "is valid with valid args" do - expect(described_class.new(timestamp: "2019-04-16T14:35:30Z")).to be_valid + expect(described_class.new(as_of: "2019-04-16T14:35:30Z")).to be_valid end it "is invalid with no timestamp" do - expect(described_class.new(timestamp: "")).to be_invalid + expect(described_class.new(as_of: "")).to be_invalid end it "is invalid with a non-ISO date" do - expect(described_class.new(timestamp: "2019-04-16 10:38:08 -0400")).to be_invalid + expect(described_class.new(as_of: "2019-04-16 10:38:08 -0400")).to be_invalid end it "is valid with an ISO date with millis" do - expect(described_class.new(timestamp: "2019-04-16T14:35:30.123Z")).to be_valid + expect(described_class.new(as_of: "2019-04-16T14:35:30.123Z")).to be_valid end it "is invalid with an ISO date without seconds" do - expect(described_class.new(timestamp: "2019-04-16T14:35Z")).to be_invalid + expect(described_class.new(as_of: "2019-04-16T14:35Z")).to be_invalid end end @@ -46,7 +46,7 @@ it "returns splits that were retired after the given timestamp" do split = FactoryBot.create(:split, finished_at: Time.zone.now) - expect(described_class.new(timestamp: 1.day.ago).splits.all).to include(split) + expect(described_class.new(as_of: 1.day.ago).splits.all).to include(split) end end From c3cc06c50547c3f70e7e2af7a97d9cf17575d791 Mon Sep 17 00:00:00 2001 From: Andrew Freeman Date: Fri, 3 Jan 2020 12:36:40 -0500 Subject: [PATCH 9/9] final pr code review tweaks --- config/routes.rb | 2 +- .../v3/split_registries_controller_spec.rb | 64 ++++++++++--------- spec/models/split_registry_spec.rb | 44 ++++++------- 3 files changed, 57 insertions(+), 53 deletions(-) diff --git a/config/routes.rb b/config/routes.rb index b379bed8..12201d8a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -67,7 +67,7 @@ namespace :v3 do resources :builds, only: [], param: :timestamp do - resource :split_registry, only: :show, controller: 'split_registries' + resource :split_registry, only: :show end end end diff --git a/spec/controllers/api/v3/split_registries_controller_spec.rb b/spec/controllers/api/v3/split_registries_controller_spec.rb index 595aa87e..fda8bb87 100644 --- a/spec/controllers/api/v3/split_registries_controller_spec.rb +++ b/spec/controllers/api/v3/split_registries_controller_spec.rb @@ -1,10 +1,6 @@ require 'rails_helper' RSpec.describe Api::V3::SplitRegistriesController, type: :controller do - let(:split_1) { FactoryBot.create :split, name: "one", finished_at: Time.zone.parse('2019-11-13'), registry: { all: 100 } } - let(:split_2) { FactoryBot.create :split, name: "two", registry: { on: 50, off: 50 } } - let(:split_3) { FactoryBot.create :split, name: "three_enabled", registry: { true: 99, false: 1 }, feature_gate: true } - describe "#show" do before do allow(ENV).to receive(:fetch).with('EXPERIENCE_SAMPLING_WEIGHT', any_args).and_return(10) @@ -17,37 +13,47 @@ expect(response_json['experience_sampling_weight']).to eq(10) end - it "returns empty with no active splits on the timestamp" do - expect(split_1).to be_finished + context "without active split on given timestamp" do + let!(:split_1) { FactoryBot.create :split, name: "one", finished_at: Time.zone.parse('2019-11-13'), registry: { all: 100 } } - get :show, params: { build_timestamp: '2019-11-14T14:35:30Z' } + it "returns empty with no active splits on the timestamp" do + expect(split_1).to be_finished - expect(response).to have_http_status :ok - expect(response_json['splits']).to eq({}) + get :show, params: { build_timestamp: '2019-11-14T14:35:30Z' } + + expect(response).to have_http_status :ok + expect(response_json['splits']).to eq({}) + end end - it "returns the full split registry of splits that are active during timestamp" do - expect(split_1).to be_finished - expect(split_2).not_to be_finished - expect(split_3).not_to be_finished + context "with splits active on given during timestamp" do + let(:split_1) { FactoryBot.create :split, name: "one", finished_at: Time.zone.parse('2019-11-13'), registry: { all: 100 } } + let(:split_2) { FactoryBot.create :split, name: "two", registry: { on: 50, off: 50 } } + let(:split_3) { FactoryBot.create :split, name: "three_enabled", registry: { true: 99, false: 1 }, feature_gate: true } - get :show, params: { build_timestamp: '2019-11-12T14:35:30Z' } + it "returns the full split registry of splits that are active during timestamp" do + expect(split_1).to be_finished + expect(split_2).not_to be_finished + expect(split_3).not_to be_finished - expect(response).to have_http_status :ok - expect(response_json['splits']).to eq( - "one" => { - "weights" => { "all" => 100 }, - "feature_gate" => false - }, - "two" => { - "weights" => { "on" => 50, "off" => 50 }, - "feature_gate" => false - }, - "three_enabled" => { - "weights" => { "true" => 99, "false" => 1 }, - "feature_gate" => true - } - ) + get :show, params: { build_timestamp: '2019-11-12T14:35:30Z' } + + expect(response).to have_http_status :ok + expect(response_json['splits']).to eq( + "one" => { + "weights" => { "all" => 100 }, + "feature_gate" => false + }, + "two" => { + "weights" => { "on" => 50, "off" => 50 }, + "feature_gate" => false + }, + "three_enabled" => { + "weights" => { "true" => 99, "false" => 1 }, + "feature_gate" => true + } + ) + end end it "returns unprocessable_entity if the timestamp url param is invalid" do diff --git a/spec/models/split_registry_spec.rb b/spec/models/split_registry_spec.rb index 2057776e..0a8eae34 100644 --- a/spec/models/split_registry_spec.rb +++ b/spec/models/split_registry_spec.rb @@ -51,38 +51,36 @@ end describe "#experience_sampling_weight" do - context "bypassing singleton memoization" do - it "memoizes the env var fetch" do - allow(ENV).to receive(:fetch).and_call_original + it "memoizes the env var fetch" do + allow(ENV).to receive(:fetch).and_call_original - subject.experience_sampling_weight - subject.experience_sampling_weight + subject.experience_sampling_weight + subject.experience_sampling_weight - expect(ENV).to have_received(:fetch).with('EXPERIENCE_SAMPLING_WEIGHT', any_args).exactly(:once) - end + expect(ENV).to have_received(:fetch).with('EXPERIENCE_SAMPLING_WEIGHT', any_args).exactly(:once) + end - it "returns 1 with no env var" do - with_env EXPERIENCE_SAMPLING_WEIGHT: nil do - expect(subject.experience_sampling_weight).to eq 1 - end + it "returns 1 with no env var" do + with_env EXPERIENCE_SAMPLING_WEIGHT: nil do + expect(subject.experience_sampling_weight).to eq 1 end + end - it "returns an positive value specified" do - with_env EXPERIENCE_SAMPLING_WEIGHT: '10' do - expect(subject.experience_sampling_weight).to eq 10 - end + it "returns an positive value specified" do + with_env EXPERIENCE_SAMPLING_WEIGHT: '10' do + expect(subject.experience_sampling_weight).to eq 10 end + end - it "returns zero when specified" do - with_env EXPERIENCE_SAMPLING_WEIGHT: '0' do - expect(subject.experience_sampling_weight).to eq 0 - end + it "returns zero when specified" do + with_env EXPERIENCE_SAMPLING_WEIGHT: '0' do + expect(subject.experience_sampling_weight).to eq 0 end + end - it "blows up on negative" do - with_env EXPERIENCE_SAMPLING_WEIGHT: '-1' do - expect { subject.experience_sampling_weight }.to raise_error(/greater than or equal/) - end + it "blows up on negative" do + with_env EXPERIENCE_SAMPLING_WEIGHT: '-1' do + expect { subject.experience_sampling_weight }.to raise_error(/greater than or equal/) end end end