Merge branch 'bugs/#12220' into 'master'

Return 404 when a commit or a branch cannot be found

See merge request !77

app/controllers/application_controller.rb

@@ -61,6 +61,11 @@ class ApplicationController < ActionController::Base
   rescue_from BadGateway, with: :bad_gateway                        # for 502
   rescue_from ServerUnavailable, with: :server_unavailable          # for 503
 
+  # exception-handlers for custom repository errors
+  rescue_from Grid5000::Errors::BranchNotFound, with: :not_found
+  rescue_from Grid5000::Errors::CommitNotFound, with: :not_found
+  rescue_from Grid5000::Errors::RefNotFound, with: :not_found
+
   protected
 
   def set_default_format

app/controllers/resources_controller.rb

@@ -92,11 +92,11 @@ class ResourcesController < ApplicationController
                           %w[admin default production]
                         else
                           params[:queues].split(',')
-                                          end
+                        end
                       end
 
     if params[:controller] == 'sites' && params[:action] == 'show' && params[:deep] && params[:job_id]
-      params[:version] = OAR::Job.expanded.find(params[:job_id]).start_time
+      params[:timestamp] = OAR::Job.expanded.find(params[:job_id]).start_time
     end
   end
 

app/controllers/versions_controller.rb

@@ -62,7 +62,7 @@ class VersionsController < ApplicationController
       limit: 1
     )
     if versions['total'] == 0
-      raise NotFound, "The requested version '#{version}' does not exist or the resource '#{resource_path}' does not exist."
+      raise NotFound, "The resource '#{resource_path}' does not exist."
     end
 
     # etag compute_etag(commit.id, resource_uri, response['Content-Type'], options.release_hash)

lib/grid5000/repository.rb

@@ -42,13 +42,13 @@ module Grid5000
       @commit = nil
       begin
         @commit = find_commit_for(options)
-        logger.info "    commit = #{@commit} {id: #{@commit.oid}, message: #{@commit.message.chomp}}"
         return nil if @commit.nil?
+        logger.info "    commit = #{@commit} {id: #{@commit.oid}, message: #{@commit.message.chomp}}"
 
         object = find_object_at(path, @commit)
         logger.debug "    object = #{object}"
         return nil if object.nil?
-      rescue StandardError => e
+      rescue Rugged::Error => e
         logger.debug "#{Time.now}: Got a Rugged exception #{e}"
         return e
       end
@@ -187,18 +187,20 @@ module Grid5000
     def find_commit_for(options = {})
       options[:branch] ||= 'master'
       version, branch, timestamp, date = options.values_at(:version, :branch, :timestamp, :date)
-      if version && version.to_s.length == 40 # SHA
-        instance.lookup(version)
-      elsif timestamp || date || version
+      if version
+        begin
+          instance.lookup(version)
+        rescue
+          raise Errors::CommitNotFound.new(version)
+        end
+      elsif timestamp || date
         if timestamp
           ts = timestamp.to_i
-        elsif date
-          ts = Time.parse(date).to_i
         else
-          ts = version.to_i
+          ts = Time.parse(date).to_i
         end
 
-        return nil if instance.branches[branch].nil?
+        raise Errors::BranchNotFound.new(branch) if instance.branches[branch].nil?
 
         walker = Rugged::Walker.new(instance)
         walker.sorting(Rugged::SORT_DATE)
@@ -212,10 +214,9 @@ module Grid5000
         sha = commits.first
         find_commit_for(options.merge(version: sha))
       else
+        raise Errors::BranchNotFound.new(branch) unless instance.branches.exist?(branch)
         instance.branches[branch].target
       end
-    rescue Rugged::OdbError
-      nil
     end
 
     def find_object_at(path, commit, relative_to = nil)
@@ -256,15 +257,17 @@ module Grid5000
       path = full_path(path)
       commits = []
 
-      if instance.branches.exist?(branch)
-        oid = instance.branches[branch].target.oid
-      else
-        begin
-          oid = (instance.lookup(branch).oid if instance.exists?(branch))
-        rescue StandardError
-          oid = nil
-        end
-      end
+      oid = if instance.branches.exist?(branch)
+              instance.branches[branch].target.oid
+            else
+              begin
+                instance.exists?(branch)
+              rescue
+                raise Errors::RefNotFound.new(branch)
+              end
+
+              instance.lookup(branch).oid
+            end
 
       if oid
         walker = Rugged::Walker.new(instance)
@@ -289,4 +292,42 @@ module Grid5000
       }
     end
   end
+
+  module Errors
+    class RepositoryError < StandardError
+      def initialize(message)
+        super(message)
+      end
+    end
+
+    class BranchNotFound < RepositoryError
+      def initialize(branch = nil)
+        if branch
+          super("Branch '#{branch}' cannot be found.")
+        else
+          super('Branch cannot be found.')
+        end
+      end
+    end
+
+    class CommitNotFound < RepositoryError
+      def initialize(commit = nil)
+        if commit
+          super("Commit '#{commit}' cannot be found.")
+        else
+          super('Commit cannot be found.')
+        end
+      end
+    end
+
+    class RefNotFound < RepositoryError
+      def initialize(ref = nil)
+        if ref
+          super("Reference (branch or commit) '#{ref}' cannot be found.")
+        else
+          super('Reference (branch or commit) cannot be found.')
+        end
+      end
+    end
+  end
 end # module Grid5000

spec/controllers/sites_controller_spec.rb

@@ -132,6 +132,16 @@ describe SitesController do
         l['rel'] == 'version'
       end['href']).to eq '/sites/rennes/versions/b00bd30bf69c322ffe9aca7a9f6e3be0f29e20f4'
     end
+
+    it 'should return 404 if the specified branch does not exist' do
+      get :show, params: { id: 'rennes', format: :json, branch: 'doesnotexist' }
+      expect(response.status).to eq 404
+    end
+
+    it 'should return 404 if the specified version does not exist' do
+      get :show, params: { id: 'rennes', format: :json, version: 'doesnotexist' }
+      expect(response.status).to eq 404
+    end
   end
 
   describe 'GET /sites/{{site_id}}/status (authenticated)' do

spec/controllers/versions_controller_spec.rb

@@ -40,7 +40,7 @@ describe VersionsController do
       get :show, params: { resource: '/', id: 'doesnotexist', format: :json }
       expect(response.status).to eq(404)
       assert_vary_on :accept
-      expect(response.body).to match %r{The requested version 'doesnotexist' does not exist or the resource '/' does not exist.}
+      expect(response.body).to match "Reference (branch or commit) 'doesnotexist' cannot be found."
     end
 
     it 'should return the version' do

spec/lib/grid5000/repository_spec.rb

@@ -65,12 +65,6 @@ describe Grid5000::Repository do
         expect(commit.oid).to eq(@latest_commit)
       end
 
-      it 'should find the commit associated with the given timestamp [version=TS]' do
-        date = Time.parse('2009-03-13 17:24:20 +0100')
-        commit = @repository.find_commit_for(version: date.to_i)
-        expect(commit.oid).to eq('b00bd30bf69c322ffe9aca7a9f6e3be0f29e20f4')
-      end
-
       it 'should find the commit associated with the given timestamp [timestamp=TS] 1/2' do
         date = Time.parse('2009-03-13 17:24:20 +0100')
         commit = @repository.find_commit_for(timestamp: date.to_i)
@@ -90,21 +84,19 @@ describe Grid5000::Repository do
         expect(commit.oid).to eq('e07895a4b480aaa8e11c35549a97796dcc4a307d')
       end
 
-      it 'should return nil when asking for a version from a branch that does not exist' do
+      it 'should return Errors::BranchNotFound when asking for a version from a branch that does not exist' do
         date = Time.parse('Fri Mar 13 17:24:47 2009 +0100')
-        commit = @repository.find_commit_for(
+        expect { @repository.find_commit_for(
           timestamp: date.to_i,
           branch: 'doesnotexist'
-        )
-        expect(commit).to be_nil
+        ) }.to raise_error(Grid5000::Errors::BranchNotFound)
       end
 
-      it 'should return nil if the request version cannot be found' do
-        commit = @repository.find_commit_for(
+      it 'should return Errors::CommitNotfound if the request version cannot be found' do
+        expect { @repository.find_commit_for(
           version: 'aaa895a4b480aaa8e11c35549a97796dcc4a307d',
           branch: 'master'
-        )
-        expect(commit).to be_nil
+        ) }.to raise_error(Grid5000::Errors::CommitNotFound)
       end
     end # describe "finding a specific version"