From 890b7f1d93908a3330f4ac4de10e666b828db116 Mon Sep 17 00:00:00 2001
From: Piotr Sarnacki <drogus@gmail.com>
Date: Thu, 11 Sep 2014 17:44:36 +0200
Subject: [PATCH] Fix Accept header support for logs request
When log is aggregated and archived we don't have a way to return it in
JSON format, only text format. Till recently we were returning a text
response or a redirect to S3 even if Accept header was set only to JSON.
This commit fixes it.
---
lib/travis/api/app/endpoint/jobs.rb | 22 +++++++++++++++-------
lib/travis/api/app/helpers/accept.rb | 4 ++++
spec/integration/v2/jobs_spec.rb | 9 +++++++--
3 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/lib/travis/api/app/endpoint/jobs.rb b/lib/travis/api/app/endpoint/jobs.rb
index 0c497a58..302112e0 100644
--- a/lib/travis/api/app/endpoint/jobs.rb
+++ b/lib/travis/api/app/endpoint/jobs.rb
@@ -3,6 +3,8 @@ require 'travis/api/app'
class Travis::Api::App
class Endpoint
class Jobs < Endpoint
+ include Helpers::Accept
+
get '/' do
prefer_follower do
respond_with service(:find_jobs, params)
@@ -49,15 +51,21 @@ class Travis::Api::App
get '/:job_id/log' do
resource = service(:find_log, params).run
- if !resource || resource.archived?
- archived_log_path = archive_url("/jobs/#{params[:job_id]}/log.txt")
+ if (!resource || resource.archived?)
+ # the way we use responders makes it hard to validate proper format
+ # automatically here, so we need to check it explicitly
+ if accepts?('text/plain')
+ archived_log_path = archive_url("/jobs/#{params[:job_id]}/log.txt")
- if params[:cors_hax]
- status 204
- headers['Access-Control-Expose-Headers'] = 'Location'
- headers['Location'] = archived_log_path
+ if params[:cors_hax]
+ status 204
+ headers['Access-Control-Expose-Headers'] = 'Location'
+ headers['Location'] = archived_log_path
+ else
+ redirect archived_log_path, 307
+ end
else
- redirect archived_log_path, 307
+ status 406
end
else
respond_with resource
diff --git a/lib/travis/api/app/helpers/accept.rb b/lib/travis/api/app/helpers/accept.rb
index a57dfa8e..229f544e 100644
--- a/lib/travis/api/app/helpers/accept.rb
+++ b/lib/travis/api/app/helpers/accept.rb
@@ -64,6 +64,10 @@ class Travis::Api::App
end
end
+ def accepts?(mime_type)
+ accept_entries.any? { |e| e.accepts?(mime_type) }
+ end
+
def accept_entries
entries = env['HTTP_ACCEPT'].to_s.delete(' ').to_s.split(',').map { |e| Entry.new(e) }
entries.empty? ? [Entry.new('*/*')] : entries.sort
diff --git a/spec/integration/v2/jobs_spec.rb b/spec/integration/v2/jobs_spec.rb
index 13be7a25..1183d619 100644
--- a/spec/integration/v2/jobs_spec.rb
+++ b/spec/integration/v2/jobs_spec.rb
@@ -28,6 +28,7 @@ describe 'Jobs' do
context 'when log is archived' do
it 'redirects to archive' do
job.log.update_attributes!(content: 'the log', archived_at: Time.now, archive_verified: true)
+ headers = { 'HTTP_ACCEPT' => 'text/plain; version=2' }
response = get "/jobs/#{job.id}/log.txt", {}, headers
response.should redirect_to("https://s3.amazonaws.com/archive.travis-ci.org/jobs/#{job.id}/log.txt")
end
@@ -36,6 +37,7 @@ describe 'Jobs' do
context 'when log is missing' do
it 'redirects to archive' do
job.log.destroy
+ headers = { 'HTTP_ACCEPT' => 'text/plain; version=2' }
response = get "/jobs/#{job.id}/log.txt", {}, headers
response.should redirect_to("https://s3.amazonaws.com/archive.travis-ci.org/jobs/#{job.id}/log.txt")
end
@@ -44,6 +46,7 @@ describe 'Jobs' do
context 'with cors_hax param' do
it 'renders No Content response with location of the archived log' do
job.log.destroy
+ headers = { 'HTTP_ACCEPT' => 'text/plain; version=2' }
response = get "/jobs/#{job.id}/log.txt?cors_hax=true", {}, headers
response.status.should == 204
response.headers['Location'].should == "https://s3.amazonaws.com/archive.travis-ci.org/jobs/#{job.id}/log.txt"
@@ -76,8 +79,10 @@ describe 'Jobs' do
end
it 'responds with 406 when log is already aggregated' do
- job.log.update_attributes(aggregated_at: Time.now)
- headers = { 'HTTP_ACCEPT' => 'application/vnd.travis-ci.2+json; chunked=true' }
+ job.log.update_attributes(aggregated_at: Time.now, archived_at: Time.now, archive_verified: true)
+ job.log.should be_archived
+
+ headers = { 'HTTP_ACCEPT' => 'application/json; version=2; chunked=true' }
response = get "/jobs/#{job.id}/log", {}, headers
response.status.should == 406
end