Skip to content

Commit 509752f

Browse files
kbukum1Copilot
andcommitted
Address Copilot review: validate parsed JSON shape, pre-register experiments
- Validate parsed response is a Hash before calling fetch on it - Add test for non-object JSON body (e.g. bare array) - Pre-register experiments from job definition before the blocked_versions gate - Add test verifying experiment flag works when set via job definition Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent fac5354 commit 509752f

4 files changed

Lines changed: 52 additions & 0 deletions

File tree

updater/lib/dependabot/api_client.rb

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -403,6 +403,10 @@ def fetch_blocked_versions(package_manager)
403403
end
404404

405405
parsed = JSON.parse(response.body)
406+
unless parsed.is_a?(Hash)
407+
Dependabot.logger.warn("Unexpected blocked versions format, continuing without them")
408+
return []
409+
end
406410
data = parsed.fetch("data", [])
407411
unless data.is_a?(Array)
408412
Dependabot.logger.warn("Unexpected blocked versions format, continuing without them")

updater/lib/dependabot/update_files_command.rb

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,12 @@ def job
7373
definition = JSON.parse(JSON.generate(Environment.job_definition))
7474
job_hash = definition["job"]
7575

76+
# Register experiments from the job definition early so experiment
77+
# gates below can read flags that arrive via the job payload.
78+
(job_hash["experiments"] || {}).each do |name, value|
79+
Experiments.register(name, value)
80+
end
81+
7682
# Fetch blocked versions from the API if the experiment is enabled.
7783
# Inject them into the job definition so they're available at construction time.
7884
if Experiments.enabled?(:dependabot_blocked_versions)

updater/spec/dependabot/api_client_spec.rb

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -907,5 +907,19 @@
907907
expect(result).to eq([])
908908
end
909909
end
910+
911+
context "when the API returns a non-object JSON body" do
912+
before do
913+
stub_request(:get, blocked_versions_url)
914+
.with(query: { "package-manager": "npm_and_yarn" })
915+
.to_return(status: 200, body: "[]", headers: headers)
916+
end
917+
918+
it "returns an empty array and logs a warning" do
919+
expect(Dependabot.logger).to receive(:warn).with(/Unexpected blocked versions format/)
920+
result = client.fetch_blocked_versions("npm_and_yarn")
921+
expect(result).to eq([])
922+
end
923+
end
910924
end
911925
end

updater/spec/dependabot/update_files_command_spec.rb

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -472,6 +472,34 @@
472472
expect(job_instance.blocked_versions).to eq(blocked_versions)
473473
end
474474

475+
context "when the experiment is enabled via the job definition" do
476+
let(:job_definition) do
477+
definition = JSON.parse(fixture("jobs/job_without_credentials.json"))
478+
definition["job"]["experiments"] = { "dependabot_blocked_versions" => true }
479+
definition
480+
end
481+
482+
before do
483+
Dependabot::Experiments.reset!
484+
allow(service).to receive(:fetch_blocked_versions).and_return(blocked_versions)
485+
end
486+
487+
it "reads the experiment flag from the job definition and fetches blocked versions" do
488+
dummy_runner = double(run: nil)
489+
allow(Dependabot::Updater).to receive(:new).and_return(dummy_runner)
490+
allow(dummy_runner).to receive(:run)
491+
allow(service).to receive(:mark_job_as_processed)
492+
allow(service).to receive(:update_dependency_list)
493+
494+
expect(service).to receive(:fetch_blocked_versions).with("bundler")
495+
496+
perform_job
497+
498+
job_instance = job.send(:job)
499+
expect(job_instance.blocked_versions).to eq(blocked_versions)
500+
end
501+
end
502+
475503
context "when the API returns no blocked versions" do
476504
let(:blocked_versions) { [] }
477505

0 commit comments

Comments
 (0)