Skip to content

Improve DownloadContents and DownloadContentsWithMeta methods - #3573

Merged
gmlewis merged 9 commits into
google:masterfrom
kuzminT:improve-downloadContents
May 12, 2025
Merged

Improve DownloadContents and DownloadContentsWithMeta methods#3573
gmlewis merged 9 commits into
google:masterfrom
kuzminT:improve-downloadContents

Conversation

@kuzminT

@kuzminTkuzminT commented May 10, 2025

Copy link
Copy Markdown
Contributor

Fixes: #3566.
By parsing the content field from the response, we can retrieve the most recent state of the file without making an additional request. Only when the content field is empty—such as for object format and files exceeding the 1 MB limit (see GitHub API docs) — do we fall back to the previous approach, which involves directory parsing and fetching the content via download_url or as raw data.
Additional tests were added to cover various cases.

@google-cla

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@kuzminT
kuzminT marked this pull request as draft May 10, 2025 12:21
@gmlewisgmlewis mentioned this pull request May 10, 2025
@gmlewis

Copy link
Copy Markdown
Collaborator

Ah, I see now what is happening. Your PR is changing how downloads work... which is probably not a good sign in that this change as written will cause compatibility problems with existing code as demonstrated in the newTestServer here:

mux.HandleFunc(
path.Join(repoPath, "commits", ref),
jsonHandler(emptyQuery, &github.RepositoryCommit{SHA: github.Ptr("s")}),
)
vardescriptionsContent []*github.RepositoryContent
forname, content:=rangefiles {
descriptionsContent=append(descriptionsContent, &github.RepositoryContent{
Name: github.Ptr(path.Base(path.Dir(name))),
})
mux.HandleFunc(
path.Join(repoPath, "contents/descriptions", path.Dir(name)),
jsonHandler(refQuery, []*github.RepositoryContent{
{
Name: github.Ptr(path.Base(name)),
DownloadURL: github.Ptr(server.URL+"/dl/"+name),
},
}),
)
mux.HandleFunc(
path.Join("/dl", name),
jsonHandler(emptyQuery, content),
)
}

It looks like you are going to have to make it so that your changes do not adversely affect existing code, if possible.

If it is not possible, then we probably are going to have to keep the legacy behavior consistent for existing users and instead add a new endpoint that behaves the way you propose, with detailed explanations in comments as to when to use the legacy mechanism and when to use your new mechanism.

Does that make sense?

@kuzminT

Copy link
Copy Markdown
ContributorAuthor

It looks like you are going to have to make it so that your changes do not adversely affect existing code, if possible.

I believe I’ve finally managed to fix the problem. Take a look when you have a moment.

@codecov

codecovBot commented May 10, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.29%. Comparing base (6a7684f) to head (c1da150).
Report is 14 commits behind head on master.

Additional details and impacted files
@@ Coverage Diff @@## master #3573 +/- ##
==========================================
+ Coverage 91.23% 91.29% +0.05% 
==========================================
Files 183 183 Lines 16053 16087 +34 ==========================================
+ Hits 14646 14686 +40 + Misses 1231 1227 -4 + Partials 176 174 -2 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @kuzminT!
Two tiny nits to address, please, otherwise looks good.
If you could increase the code coverage, that would be great, but not critical.

After the nits, we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.

Comment threadgithub/repos_contents.go Outdated
Comment threadgithub/repos_contents.go Outdated
@kuzminT
kuzminT marked this pull request as ready for review May 11, 2025 09:42
@gmlewisgmlewis added the NeedsReview PR is awaiting a review before merging. label May 11, 2025

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @kuzminT!
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@stevehipwell - might you have time for a code review? Thank you!

@stevehipwellstevehipwell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@gmlewis

Copy link
Copy Markdown
Collaborator

Thank you, @stevehipwell!
Merging.

@gmlewisgmlewis removed the NeedsReview PR is awaiting a review before merging. label May 12, 2025
@gmlewis
gmlewis merged commit d436b52 into google:masterMay 12, 2025
jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
Sign up for freeto 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.

Bug: DownloadContentsWithMeta returns old content for a file

3 participants

@kuzminT@gmlewis@stevehipwell