fix(codaveri): fix crash on import question with empty data files - #8534
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a crash when importing Codaveri programming questions that contain zero-byte files by avoiding in-place force_encoding on potentially frozen strings returned by rubyzip, and reduces duplication by centralizing the shared supporting-file extraction logic.
Changes:
- Promote
extract_supporting_fileintoLanguagePackageService, usingcontent.dup.force_encoding('UTF-8')to avoidFrozenErroron zero-byte entries. - Add an overridable
utf8_encodable?predicate (Java narrows plaintext handling to.javafiles as before). - Add specs + fixture coverage for UTF-8, binary, and empty-file cases (including frozen empty string behavior).
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| spec/services/course/assessment/question/programming_codaveri/language_package_service_spec.rb | Adds regression coverage for frozen empty strings and utf8_encodable? behavior (base + Java override). |
| app/services/course/assessment/question/programming_codaveri/language_package_service.rb | Centralizes supporting file extraction and avoids mutating frozen zip entry strings. |
| app/services/course/assessment/question/programming_codaveri/java/java_package_service.rb | Replaces duplicated extraction logic with a utf8_encodable? override to preserve Java’s .java restriction. |
| app/services/course/assessment/question/programming_codaveri/python/python_package_service.rb | Removes duplicated extract_supporting_file now provided by the base service. |
| app/services/course/assessment/question/programming_codaveri/java_script/java_script_package_service.rb | Removes duplicated extract_supporting_file now provided by the base service. |
| app/services/course/assessment/question/programming_codaveri/type_script/type_script_package_service.rb | Removes duplicated extract_supporting_file now provided by the base service. |
| app/services/course/assessment/question/programming_codaveri/go/go_package_service.rb | Removes duplicated extract_supporting_file now provided by the base service. |
| app/services/course/assessment/question/programming_codaveri/rust/rust_package_service.rb | Removes duplicated extract_supporting_file now provided by the base service. |
| app/services/course/assessment/question/programming_codaveri/r/r_package_service.rb | Removes duplicated extract_supporting_file now provided by the base service. |
| app/services/course/assessment/question/programming_codaveri/c_sharp/c_sharp_package_service.rb | Removes duplicated extract_supporting_file now provided by the base service. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
adi-herwana-nus
enabled auto-merge (rebase)
August 2, 2026 08:58
adi-herwana-nus
disabled auto-merge
August 2, 2026 11:05
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
Rollbar flagged
FrozenError: can't modify frozen String: ""when importing aCodaveri programming question, raised from the encoding check in
PythonPackageService#extract_supporting_file:force_encodingre-tags its receiver in place, so it raises as soon ascontentis frozen.What exactly triggers it.
contentoriginates fromProgrammingPackage#get_folder_files,which reads each entry via
entry.get_input_stream(&:read). rubyzip short-circuits that read forexhausted streams and hands back a shared frozen literal:
A zero-byte entry is
eof?on its very first read, so it never reaches the buffer path andreceives the frozen
''. Every non-empty entry gets a fresh mutable buffer. Measured against thelocked rubyzip:
force_encodingempty.csvtiny_binary.datbig_binary.datutf8.txtone_null.datSo the trigger is precisely: a package containing a zero-byte file, in the package root or the
tests/,submission/, orsolution/folders, whose name is not on the service's skip list(
Makefile,.meta,append.py,prepend.py,autograde.py,template.py).Two things this is not sensitive to:
branch only after
force_encodinghas already succeeded.Base64.strict_encode64does notmutate its argument, so that branch never needed a mutable string.
carried the identical call, so a zero-byte file broke every one of them (see Scope check).
In the reported package the trigger is
tests/empty.csv.tests/append.pyis also zero-byte but isskip-listed, so it never reaches the call.
Regression window. rubyzip 2.4.1's copy of
abstract_input_stream.rbhas nofrozen_string_literalmagic comment, so the identicalreturn ''produced a mutable string andforce_encodingsilently worked. The magic comment arrives in rubyzip 3.x, so this has been brokenfor every affected package since
dc14a58866("chore(deps): bump rubyzip from 2.4.1 to 3.2.2").Verified against the currently locked rubyzip 3.3.0.
Remediation
Tag the encoding on a copy rather than on the string owned by the package reader:
While fixing this,
extract_supporting_fileturned out to be duplicated across all eightlanguage package services — seven byte-identical, with Java differing by a single clause
(
&& filename.to_s.downcase.end_with?('.java')). Every copy carried the same defect, so rather thanpatch the same line eight times the method was promoted to
LanguagePackageServiceand Java's extra clause extracted into an overridable predicate:
This removes 125 lines of app code. The encoding decision is unchanged for every service: empty
files continue to be sent as
utf8by the seven generic services, and asbase64by Java (whosepredicate has always required a
.javaextension — for an empty file this is cosmetic either way,since
Base64.strict_encode64('')is''). The one byte-level difference: the base64 branch nowreceives the untouched
ASCII-8BIToriginal instead of a string re-tagged in place, which encodesidentically since Base64 operates on bytes.
Scope check
Audited every
force_encodingcall acrossapp,lib, andspec. Post-refactor exactly tworemain:
LanguagePackageService— this fix.CoursemologyDockerContainer#extract_test_report— already guarded, and worth noting because it broke in exactly this way before:
That guard was added in
d6f56afd1a("chore(ruby): upgrade to ruby 3.3.5", Sept 2024), whosebody reads "fix error due to tar files being frozen strings". Same bug class — a library reader
(rubygems'
TarReaderthere, rubyzip here) returning a frozen string into an in-placeforce_encoding. No action needed; it uses+strrather than.dup, which is equally safe.No other call site mutates strings returned by
ProgrammingPackage's file readers.Tests
Added
language_package_service_spec.rbcovering
extract_supporting_fileandutf8_encodable?at the base class, including the Javaoverride. The empty-file example asserts the fixture content is genuinely frozen
(
expect(content).to be_frozen) so the spec cannot silently stop reproducing the originalcondition. This path touches no DB, so no tenant block is needed.
Backed by a new fixture,
spec/fixtures/course/programming_question_template_codaveri_empty_data_file.zip, built on theexisting
programming_question_template_codaveri.zipscaffolding plus supporting files covering eachbranch:
data.csv(utf8),binary.dat(invalid UTF-8, base64), and zero-byteempty.csv,tests/empty.csvandtests/append.pymirroring the reported package. This is a new fixturerather than an extension of the existing one, which would have shifted
data_filesexpectations incurrent specs.
Verified end-to-end by reconstructing the Codaveri
/problemrequest body for the reported package:it raises the production
FrozenErrorbefore this change and serialises the empty file as{"path": "empty.csv", "content": "", "encoding": "utf8"}after.