Merge pull request #249 from ncbo/refactor/ontology-download-endpoints

refactor: consolidate ontology download routes into a shared helper

Author: alexskr

controllers/ontologies_controller.rb

@@ -114,33 +114,10 @@ class OntologiesController < ApplicationController
       acronym = params["acronym"]
       ont = Ontology.find(acronym).include(Ontology.goo_attrs_to_load).first
       error 422, "You must provide an existing `acronym` to download" if ont.nil?
-      ont.bring(:viewingRestriction) if ont.bring?(:viewingRestriction)
-      check_access(ont)
-      restricted_download = LinkedData::OntologiesAPI.settings.restrict_download.include?(acronym)
-      error 403, "License restrictions on download for #{acronym}" if restricted_download && !current_user.admin?
-      error 403, "Ontology #{acronym} is not accessible to your user" if ont.restricted? && !ont.accessible?(current_user)
-      latest_submission = ont.latest_submission(status: :rdf) # Should resolve to latest successfully loaded submission
-      error 404, "There is no latest submission loaded for download" if latest_submission.nil?
-      latest_submission.bring(:uploadFilePath)
-
-      download_format = params["download_format"].to_s.downcase
-      allowed_formats = ["csv", "rdf"]
-      if download_format.empty?
-        file_path = latest_submission.uploadFilePath
-      elsif ([download_format] - allowed_formats).length > 0
-        error 400, "Invalid download format: #{download_format}."
-      elsif download_format.eql?("csv")
-        latest_submission.bring(ontology: [:acronym])
-        file_path = latest_submission.csv_path
-      elsif download_format.eql?("rdf")
-        file_path = latest_submission.rdf_path
-      end
-
-      if File.readable? file_path
-        send_file file_path, :filename => File.basename(file_path)
-      else
-        error 500, "Cannot read latest submission upload file: #{file_path}"
-      end
+      enforce_download_access(ont)
+      submission = ont.latest_submission(status: :rdf) # Should resolve to latest successfully loaded submission
+      error 404, "There is no latest submission loaded for download" if submission.nil?
+      send_submission_download(ont, submission, params["download_format"])
     end
 
     private

controllers/ontology_submissions_controller.rb

@@ -192,58 +192,24 @@ class OntologySubmissionsController < ApplicationController
       included = Ontology.goo_attrs_to_load.concat([submissions: submission_attributes])
       ont = Ontology.find(acronym).include(included).first
       error 422, "You must provide an existing `acronym` to download" if ont.nil?
-      ont.bring(:viewingRestriction) if ont.bring?(:viewingRestriction)
-      check_access(ont)
-      ont_restrict_downloads = LinkedData::OntologiesAPI.settings.restrict_download
-      error 403, "License restrictions on download for #{acronym}" if ont_restrict_downloads.include? acronym
+      enforce_download_access(ont)
       submission = ont.submission(params['ontology_submission_id'].to_i)
       error 404, "There is no such submission for download" if submission.nil?
-      file_path = submission.uploadFilePath
-      # handle edge case where uploadFilePath is not set
-      error 422, "Upload File Path is not set for this submission" if file_path.to_s.empty?
-      download_format = params["download_format"].to_s.downcase
-      allowed_formats = ["csv", "rdf"]
-      if download_format.empty?
-        file_path = submission.uploadFilePath
-      elsif ([download_format] - allowed_formats).length > 0
-        error 400, "Invalid download format: #{download_format}."
-      elsif download_format.eql?("csv")
-        if ont.latest_submission.id != submission.id
-          error 400, "Invalid download format: #{download_format}."
-        else
-          latest_submission.bring(ontology: [:acronym])
-          file_path = submission.csv_path
-        end
-      elsif download_format.eql?("rdf")
-        file_path = submission.rdf_path
-      end
-
-      if File.readable? file_path
-        send_file file_path, :filename => File.basename(file_path)
-      else
-        error 500, "Cannot read submission upload file: #{file_path}"
-      end
+      send_submission_download(ont, submission, params["download_format"])
     end
 
     ##
     # Download a submission diff file
     get '/:ontology_submission_id/download_diff' do
       acronym = params["acronym"]
       submission_attributes = [:submissionId, :submissionStatus, :diffFilePath]
-      ont = Ontology.find(acronym).include(:submissions => submission_attributes).first
+      included = Ontology.goo_attrs_to_load.concat([submissions: submission_attributes])
+      ont = Ontology.find(acronym).include(included).first
       error 422, "You must provide an existing `acronym` to download" if ont.nil?
-      ont.bring(:viewingRestriction)
-      check_access(ont)
-      ont_restrict_downloads = LinkedData::OntologiesAPI.settings.restrict_download
-      error 403, "License restrictions on download for #{acronym}" if ont_restrict_downloads.include? acronym
+      enforce_download_access(ont)
       submission = ont.submission(params['ontology_submission_id'].to_i)
       error 404, "There is no such submission for download" if submission.nil?
-      file_path = submission.diffFilePath
-      if File.readable? file_path
-        send_file file_path, :filename => File.basename(file_path)
-      else
-        error 500, "Cannot read submission diff file: #{file_path}"
-      end
+      send_submission_download(ont, submission, "diff")
     end
 
     def delete_submissions(startId, endId)

helpers/download_helper.rb

@@ -0,0 +1,75 @@
+require 'sinatra/base'
+
+module Sinatra
+  module Helpers
+    module DownloadHelper
+
+      # Formats that can be requested via `download_format`. An empty/absent
+      # value streams the original uploaded source file.
+      ALLOWED_DOWNLOAD_FORMATS = %w[csv rdf diff].freeze
+
+      ##
+      # Single access gate shared by every download endpoint. Enforces, in order:
+      #   * read ACL (check_access / read_restricted?)
+      #   * license-based download restriction (admins bypass)
+      #   * private-ontology accessibility
+      def enforce_download_access(ont)
+        ont.bring(:viewingRestriction) if ont.bring?(:viewingRestriction)
+        check_access(ont)
+        restricted = LinkedData::OntologiesAPI.settings.restrict_download.include?(ont.acronym)
+        error 403, "License restrictions on download for #{ont.acronym}" if restricted && !current_user.admin?
+        error 403, "Ontology #{ont.acronym} is not accessible to your user" if ont.restricted? && !ont.accessible?(current_user)
+      end
+
+      ##
+      # Resolve the file for a submission + requested format and stream it.
+      # Shared by the ontology-latest, specific-submission, and diff routes so
+      # format handling, missing-file guards, and streaming live in one place.
+      def send_submission_download(ont, submission, download_format)
+        download_format = download_format.to_s.downcase
+
+        unless download_format.empty? || ALLOWED_DOWNLOAD_FORMATS.include?(download_format)
+          error 400, "Invalid download format: #{download_format}."
+        end
+
+        file_path = resolve_download_path(ont, submission, download_format)
+
+        error 404, "No #{download_format.empty? ? 'source' : download_format} file is available for this submission" if file_path.to_s.empty?
+        error 404, "Download file is not readable: #{File.basename(file_path)}" unless File.readable?(file_path)
+
+        # Downloads are large (often hundreds of MB) ontology files streamed from
+        # disk. Mark them no-store so the global Rack::Cache (Redis entitystore)
+        # does not buffer the body into Redis — doing so defeats send_file's
+        # streaming and makes serving a cached hit a multi-second Redis GET.
+        cache_control :no_store
+
+        send_file file_path, filename: File.basename(file_path)
+      end
+
+      private
+
+      def resolve_download_path(ont, submission, download_format)
+        case download_format
+        when ""
+          submission.bring(:uploadFilePath) if submission.bring?(:uploadFilePath)
+          submission.uploadFilePath
+        when "rdf"
+          submission.rdf_path
+        when "diff"
+          submission.bring(:diffFilePath) if submission.bring?(:diffFilePath)
+          submission.diffFilePath
+        when "csv"
+          # The CSV is an index artifact that only survives for the current
+          # (highest-id) submission; the archiver deletes it for older ones.
+          unless ont.latest_submission&.id == submission.id
+            error 400, "CSV download is only available for the latest submission of #{ont.acronym}."
+          end
+          submission.bring(ontology: [:acronym])
+          submission.csv_path
+        end
+      end
+    end
+  end
+end
+
+helpers Sinatra::Helpers::DownloadHelper

test/controllers/test_ontologies_controller.rb

@@ -182,6 +182,16 @@ def test_download_ontology
     # see also test_ontologies_submissions_controller::test_download_submission
   end
 
+  def test_download_ontology_sets_no_store
+    # Downloads must be marked no-store so the global Rack::Cache does not
+    # buffer large ontology file bodies into Redis (see helpers/download_helper.rb).
+    acronym = create_ontologies_and_submissions(ont_count: 1, submission_count: 1, process_submission: true)[1].first
+    get "/ontologies/#{acronym}/download"
+    assert_equal(200, last_response.status, msg='failed download for ontology : ' + get_errors(last_response))
+    assert_includes(last_response.headers['Cache-Control'].to_s, 'no-store',
+      msg="download response must set Cache-Control: no-store to bypass Rack::Cache")
+  end
+
   def test_download_ontology_csv
     num_onts_created, created_ont_acronyms, onts = create_ontologies_and_submissions(ont_count: 1, submission_count: 1,
                                                                                      process_submission: true,

test/controllers/test_ontology_submissions_controller.rb

@@ -233,6 +233,26 @@ def test_download_ontology_submission_rdf
     assert_equal(400, last_response.status, "Download failure for '#{acronym}' ontology: " + get_errors(last_response))
   end
 
+  def test_download_ontology_submission_csv
+    # Regression: CSV download of the *latest* submission previously raised a
+    # NameError (undefined `latest_submission`) -> 500. It must stream the CSV,
+    # while a non-latest submission (whose CSV the archiver deletes) returns 400.
+    _, created_ont_acronyms, onts = create_ontologies_and_submissions(ont_count: 1, submission_count: 2,
+      process_submission: true,
+      process_options: { process_rdf: true, extract_metadata: true, index_search: true })
+    acronym = created_ont_acronyms.first
+    ont = onts.first
+    ont.bring(:submissions)
+    subs = ont.submissions.each { |s| s.bring(:submissionId) }.sort_by(&:submissionId)
+    older, latest = subs.first, subs.last
+
+    get "/ontologies/#{acronym}/submissions/#{latest.submissionId}/download?download_format=csv"
+    assert_equal(200, last_response.status, "CSV download failed for latest submission: " + get_errors(last_response))
+
+    get "/ontologies/#{acronym}/submissions/#{older.submissionId}/download?download_format=csv"
+    assert_equal(400, last_response.status, "Expected 400 for CSV of non-latest submission: " + get_errors(last_response))
+  end
+
   def test_download_acl_only
     _, created_ont_acronyms, onts = create_ontologies_and_submissions(ont_count: 1, submission_count: 1,
       process_submission: false)