Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix gzip support #172

Merged
merged 5 commits into from
Sep 11, 2023
Merged

Fix gzip support #172

merged 5 commits into from
Sep 11, 2023

Conversation

alexskr
Copy link
Member

@alexskr alexskr commented Sep 8, 2023

follow up on #144

when I upload a new submission with a .gz file I get the following error:

- Zip::Error - Zip end of central directory signature not found:
        /srv/ontoportal/ontologies_api/shared/bundle/ruby/2.7.0/gems/rubyzip-2.3.2/lib/zip/central_directory.rb:143:in `get_e_o_c_d'
 ...
        /srv/ontoportal/ontologies_api/shared/bundle/ruby/2.7.0/bundler/gems/ontologies_linked_data-e33a0e451f8a/lib/ontologies_linked_data/utils/file.rb:33:in `files_from_zip'
        /srv/ontoportal/ontologies_api/shared/bundle/ruby/2.7.0/bundler/gems/ontologies_linked_data-e33a0e451f8a/lib/ontologies_linked_data/models/ontology_submission.rb:209:in `sanity_check'
        /srv/ontoportal/ontologies_api/shared/bundle/ruby/2.7.0/bundler/gems/ontologies_linked_data-e33a0e451f8a/lib/ontologies_linked_data/models/ontology_submission.rb:146:in `valid?'
        /srv/ontoportal/ontologies_api/releases/20230908165536/helpers/ontology_helper.rb:27:in `create_submission'
        /srv/ontoportal/ontologies_api/releases/20230908165536/controllers/ontology_submissions_controller.rb:14:in `block in <class:OntologySubmissionsController>'

this PR:

  • updates mimitype string for gzip files from application/x-gzip to application/gzip
  • adds unit test for determining gzip mimetype

application/x-gzip is a deprecated mimetype
add unit test for determining gzip mimetype
@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #172 (5ddd9ae) into develop (e33a0e4) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 5ddd9ae differs from pull request most recent head 4f9fcd3. Consider uploading reports for the commit 4f9fcd3 to get more accurate results

@@           Coverage Diff            @@
##           develop     #172   +/-   ##
========================================
  Coverage    80.75%   80.75%           
========================================
  Files           63       63           
  Lines         4901     4901           
========================================
  Hits          3958     3958           
  Misses         943      943           
Flag Coverage Δ
unittests 80.75% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
lib/ontologies_linked_data/utils/file.rb 72.89% <100.00%> (ø)

@alexskr
Copy link
Member Author

alexskr commented Sep 8, 2023

sanity_check doesn't handle gzipped files yet.

3) Error:
TestOntologySubmission#test_valid_ontology:
Zip::Error: Zip end of central directory signature not found
    /srv/ontoportal/bundle/ruby/2.7.0/gems/rubyzip-1.3.0/lib/zip/central_directory.rb:143:in `get_e_o_c_d'
    /srv/ontoportal/bundle/ruby/2.7.0/gems/rubyzip-1.3.0/lib/zip/central_directory.rb:103:in `read_e_o_c_d'
    /srv/ontoportal/bundle/ruby/2.7.0/gems/rubyzip-1.3.0/lib/zip/central_directory.rb:136:in `read_from_stream'
    /srv/ontoportal/bundle/ruby/2.7.0/gems/rubyzip-1.3.0/lib/zip/file.rb:82:in `block in initialize'
    /srv/ontoportal/bundle/ruby/2.7.0/gems/rubyzip-1.3.0/lib/zip/file.rb:81:in `open'
    /srv/ontoportal/bundle/ruby/2.7.0/gems/rubyzip-1.3.0/lib/zip/file.rb:81:in `initialize'
    /srv/ontoportal/bundle/ruby/2.7.0/gems/rubyzip-1.3.0/lib/zip/file.rb:111:in `new'
    /srv/ontoportal/bundle/ruby/2.7.0/gems/rubyzip-1.3.0/lib/zip/file.rb:111:in `open'
    /srv/ontoportal/ontologies_linked_data/lib/ontologies_linked_data/utils/file.rb:33:in `files_from_zip'
    /srv/ontoportal/ontologies_linked_data/lib/ontologies_linked_data/models/ontology_submission.rb:209:in `sanity_check'
    /srv/ontoportal/ontologies_linked_data/lib/ontologies_linked_data/models/ontology_submission.rb:146:in `valid?'
    /srv/ontoportal/ontologies_linked_data/test/models/test_ontology_submission.rb:36:in `test_valid_ontology'

@@ -8,7 +8,7 @@ def test_valid_ontology

acronym = "BRO-TST"
name = "SNOMED-CT TEST"
ontologyFile = "./test/data/ontology_files/BRO_v3.2.owl"
ontologyFile = "./test/data/ontology_files/BRO_v3.2.owl.gz"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree with this change at all. It's not a good practice to modify existing unit tests (that were developed for a reason) to test new functionality. Pull request #144 that introduced support for gzip files should have come with it's own set of unit tests. There was even a request for unit tests in the pull request conversation, but it doesn't appear that they were ever added.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the feedback, overloading unit tests is less than ideal.

one of the purposes of this PR draft is to show that we are missing important unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants