Fix artifact download for non-binary bodies (#621)#738
Open
pmgower wants to merge 1 commit intoNARKOZ:masterfrom
Open
Fix artifact download for non-binary bodies (#621)#738pmgower wants to merge 1 commit intoNARKOZ:masterfrom
pmgower wants to merge 1 commit intoNARKOZ:masterfrom
Conversation
job_artifacts_download, download_job_artifact_file, and download_branch_artifact_file raise Gitlab::Error::Parsing on any artifact whose response body is not tagged ASCII_8BIT — YAML, plain text, CSV, JUnit XML, etc. The else-branch of each parser proc routes non-binary bodies through Gitlab::Request.parse, which calls JSON.load and re-raises non-JSON input as Gitlab::Error::Parsing. The non-binary path is reached for two cases: error responses with a JSON envelope (where Gitlab::Error::ResponseError#initialize relies on parsed_response to build the error message — Request.parse is correct here) and successful text artifacts (where it isn't). Keep the JSON parse and fall back to FileResponse on Gitlab::Error::Parsing. Add spec coverage for the text-response path on each method.
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
1 participant
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.
Fixes #621.
job_artifacts_download,download_job_artifact_file, anddownload_branch_artifact_fileraiseGitlab::Error::Parsingon any artifact whose response body is not taggedASCII_8BIT— YAML coverage reports, plain-text logs, CSVs, JUnit XML, etc. The else-branch of each parser proc routes non-binary bodies throughGitlab::Request.parse, which callsJSON.loadand re-raises non-JSON input asGitlab::Error::Parsing.The issue reports
download_job_artifact_file; the same parser proc is duplicated across all three methods, so all three have the bug. Fixing them together. I hit it downloading a YAML coverage report from a GitLab CI job; the issue reporter hit it on a.txt.The non-binary path is reached for two cases — the
# error with json responsecomment captures one of them:Gitlab::Error::ResponseError#initializecalls@response.parsed_responseto construct the human-readable error message, soRequest.parseis correct here and removing it regresses the error path.Request.parseis wrong here.The patch keeps the JSON parse and falls back to
FileResponseonGitlab::Error::Parsingfor the second case. Added a'when successful text response'spec context per method, assertingFileResponsereturn type,#filename, and#readbody content. Existing binary-body and bad-request specs remain green.rakepasses (1377 examples, 0 failures).rake rubocopintroduces no new offenses.