From 7955fc6553947a38c88f5860bd99699373b67fd8 Mon Sep 17 00:00:00 2001 From: Alex Skrenchuk Date: Tue, 28 Jul 2026 14:12:41 -0700 Subject: [PATCH 1/2] refactor: consolidate ontology download routes into a shared helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three download routes (ontology `/download`, submission `/download`, and `/download_diff`) each reimplemented format resolution, file streaming, and access/restriction checks inline. The copy-paste had drifted into bugs. Introduce `Sinatra::Helpers::DownloadHelper` with `enforce_download_access` and `send_submission_download`, and reduce each route to "resolve the submission, then delegate." This fixes several latent issues in one place: - Fix a NameError (undefined `latest_submission`) that made `submissions/:id/download?download_format=csv` return 500 for the latest submission — the one case CSV is supposed to work. The reference now correctly uses `submission`. - Replace nil-path 500s (`File.readable?(nil)` -> TypeError) on the ontology `/download` and `/download_diff` routes with a clean 404. - Unify access control to the strictest policy across all three routes: admins bypass `restrict_download`, and the `restricted?`/`accessible?` private-ontology ACL check now applies everywhere (previously only on the ontology route). - `diff` becomes just another download format, so `download_diff` reuses the shared streamer. - Clearer error when CSV is requested for a non-latest submission (whose CSV the archiver deletes) instead of "Invalid download format: csv". Also mark download responses `Cache-Control: no-store`. The global Rack::Cache (Redis entitystore) was heuristically caching large `send_file` ontology bodies: buffering the file into Redis on write and reading it back as a single multi-second Redis GET on hit (observed 64s of a 79s request in New Relic), defeating send_file streaming and bloating the HTTP Redis. Add a regression test exercising CSV download of both the latest submission (200) and a non-latest submission (400). Co-Authored-By: Claude Opus 4.8 --- controllers/ontologies_controller.rb | 31 +------- .../ontology_submissions_controller.rb | 46 ++---------- helpers/download_helper.rb | 75 +++++++++++++++++++ .../test_ontology_submissions_controller.rb | 20 +++++ 4 files changed, 105 insertions(+), 67 deletions(-) create mode 100644 helpers/download_helper.rb diff --git a/controllers/ontologies_controller.rb b/controllers/ontologies_controller.rb index 5405410e3..1f7b014ef 100644 --- a/controllers/ontologies_controller.rb +++ b/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 diff --git a/controllers/ontology_submissions_controller.rb b/controllers/ontology_submissions_controller.rb index bdda6f65f..818445383 100644 --- a/controllers/ontology_submissions_controller.rb +++ b/controllers/ontology_submissions_controller.rb @@ -192,37 +192,10 @@ 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 ## @@ -230,20 +203,13 @@ class OntologySubmissionsController < ApplicationController 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) diff --git a/helpers/download_helper.rb b/helpers/download_helper.rb new file mode 100644 index 000000000..e0d4a9343 --- /dev/null +++ b/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 diff --git a/test/controllers/test_ontology_submissions_controller.rb b/test/controllers/test_ontology_submissions_controller.rb index 5fc034f5c..11fbc2182 100644 --- a/test/controllers/test_ontology_submissions_controller.rb +++ b/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) From 7ba6174fe766286b39c962e50d53f04cccbacb9c Mon Sep 17 00:00:00 2001 From: Alex Skrenchuk Date: Tue, 28 Jul 2026 14:40:54 -0700 Subject: [PATCH 2/2] test: assert ontology download sets Cache-Control: no-store Guards against silently reintroducing the Rack::Cache regression where large send_file ontology bodies were buffered into the Redis entitystore (confirmed in production: entitystore entries up to ~535 MB). Co-Authored-By: Claude Opus 4.8 --- test/controllers/test_ontologies_controller.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/controllers/test_ontologies_controller.rb b/test/controllers/test_ontologies_controller.rb index c19605e33..1c464bf32 100644 --- a/test/controllers/test_ontologies_controller.rb +++ b/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,