Bug: DownloadContentsWithMeta returns old content for a file

[kuzminT]

When I use DownloadContentsWithMeta after making changes to a file, I consistently receive outdated content. I need to wait several minutes before calling this method again to retrieve the latest changes.

Upon investigation, I discovered that the issue originates from GitHub's side. The DownloadContentsWithMeta method uses a direct download link to fetch raw content from this URL: https://raw.githubusercontent.com/. If you attempt to access the raw content immediately after modifying a file, you will encounter the same problem.

However, perhaps we can implement a workaround on our side — for example, by adding some parameters to the URL or using a different method to bypass caching.

I also found a related bug reported in the GitHub community: https://github.com/orgs/community/discussions/46691. As of now, it remains unresolved.

Suggestions:

• Investigate if adding cache-busting parameters (e.g., timestamps or unique query strings) to the download URL forces GitHub to return the latest version.
• Explore using other link/API to fetch the most recent file contents.

[gmlewis]

Over the years, we have witnessed several issues like this that have typically been caused by the internal architecture of the GitHub servers being eventually-consistent.

When we report issues like this to GitHub Tech Support, the answer has usually been to "try again after a few seconds".

As you pointed out, the bug is not in this repo, and there is not a good way that I know of to create a workaround within this client library, as each use case varies. For example, users of this repo frequently work around this issue by performing one of these "eventually-consistent" operations, then going off and doing other work, and coming back later to check if the original operation appears to have propagated throughout the GitHub servers, at which point they can continue.

I'll leave this issue open in case others have ideas or want to try out the proposed suggestions above (but be warned that it will not be acceptable to add time.Sleep calls to this client library's methods as a workaround since that is never a 100% reliable solution). Eventually we will close this issue if there are no other ideas or working solutions.

[kuzminT]

I've checked the response from this GitHub API: https://docs.github.com/en/rest/repos/contents using in this method and found that the content field in the response (base64-encoded) contains the most recent changes to the file.
Would it be possible to return this content directly, or use it instead of making an additional request to the raw content by download_link?

[gmlewis]

If it exists in the response, then I don't see why we couldn't return it.
Would you like to make a PR to do this, @kuzminT ?

[kuzminT]

Hi @gmlewis,

I did some additional research and now understand why download_url is used in this method instead of the content field. The reason is that download_url allows access to the raw file content regardless of file size, while the content field in the GitHub API response is only populated under certain conditions:

If the requested file's size is:

• 1 MB or smaller: All features of this endpoint are supported.
• Between 1–100 MB: Only the raw or object custom media types are supported. When using the object media type, the content field will be an empty string and the encoding field will be "none". To access the file contents, the raw media type must be used.
• Greater than 100 MB: This endpoint is not supported.

Reference: GitHub REST API docs

That said, I believe in most cases we could still try using the content field first to get the latest content without making an additional request to the download_url. If the content field is empty, we could then fall back to using download_url.

Also, I’m not entirely sure why this method retrieves the entire directory tree and then searches for the file here. There might have been specific reasons for this approach.

I’ve attempted a refactor of the DownloadContentsWithMeta method in my fork. It seems to work correctly and retrieves the updated file content as expected.

However, to make this consistent, we should also refactor the DownloadContents method in a similar way, and update or add relevant tests. That said, I’m not completely certain if the current directory-parsing approach was introduced for a specific edge case or requirement.

What do you think about this approach?

[gmlewis]

Apparently, the directory tree approach was added in #1810 to solve the problem listed in #1809.
I'm going to have to study this more to give an opinion.

[kuzminT]

When trying to read, update and commit changes to a file larger than 1MB, the user currently has to additionally call GetContents of the directory to receive the SHA of the file currently being edited.

I see. However, in my approach, we still keep the SHA of the requested file, and I also return a fully populated *RepositoryContent object.

fileContent, _, resp, err := s.GetContents(ctx, owner, repo, filepath, opts)
	if err != nil {
		return nil, nil, resp, err
	}

	if fileContent != nil {
		content, err := fileContent.GetContent()
		if err == nil && content != "" {
			return io.NopCloser(strings.NewReader(content)), fileContent, resp, nil
		}

		if fileContent.DownloadURL == nil || *fileContent.DownloadURL == "" {
			return nil, fileContent, resp, fmt.Errorf("no download link found for %s", filepath)
		}

		dlReq, err := http.NewRequestWithContext(ctx, http.MethodGet, *fileContent.DownloadURL, nil)
		if err != nil {
			return nil, fileContent, resp, err
		}
		dlResp, err := s.client.client.Do(dlReq)
		if err != nil {
			return nil, fileContent, &Response{Response: dlResp}, err
		}

		return dlResp.Body, fileContent, &Response{Response: dlResp}, nil
	}

In cases where the file exceeds the 1MB limit for the object format, I fall back to using the download_url to retrieve raw content.

In most typical cases, this allows us to retrieve both the latest content and metadata in a single request—without needing to iterate through the entire directory.

Also worth noting: this API has an upper limit of 1,000 files per directory:

This API has an upper limit of 1,000 files for a directory. If you need to retrieve more files, use the Git Trees API.

[gmlewis]

I've taken a look at your fork, but I don't understand why you couldn't take a hybrid approach...

If the fileContent provides all that is needed, go ahead and return it.

However, if it doesn't, fall back to the current mechanism which searches through the dirContents.

What would be the drawbacks of this hybrid approach?

[kuzminT]

We can't use a hybrid approach because when requesting a directory instead of a specific file, the GitHub API does not populate the content field of the files.

In the current implementation, this method always requests the entire directory, which makes accessing the file content directly via the content field unreliable, and so we need another request to download_link:

dir := path.Dir(filepath)
	filename := path.Base(filepath)
	_, dirContents, resp, err := s.GetContents(ctx, owner, repo, dir, opts)
	if err != nil {
		return nil, nil, resp, err
	}

[gmlewis]

We can't use a hybrid approach because when requesting a directory instead of a specific file, the GitHub API does not populate the content field of the files.

Oh, I see. Would it be possible to first request the specific file, and if that is not possible for any of the reasons you list above, then make a second call to use the old mechanism that requests the directory?

In other words, your fork's mechanism would be inserted before (within the same method) the current mechanism.
Does that make sense?

[kuzminT]

Oh, I see. Would it be possible to first request the specific file, and if that is not possible for any of the reasons you list above, then make a second call to use the old mechanism that requests the directory?

Yes, we definitely can do that. But In the worst case, it would require calling GetContents() one additional time for the directory—resulting in three external requests instead of the current two. However, for most cases and typical source files less than 1 Mb, a single request will be enough to retrieve the relevant content.

[kuzminT]

Hi, @gmlewis. I tried to combine both approaches, as we recently discussed: PR.
However, I encountered the following error locally when running script/test.sh:

--- FAIL: TestUpdateOpenAPI (0.01s)
    main_test.go:127: GET http://127.0.0.1:40797/api/v3/repos/github/rest-api-description/contents/descriptions/ghec/ghec.json?ref=main: 404  []
    main_test.go:195: files "testdata/golden/TestUpdateOpenAPI/openapi_operations.yaml" and "/tmp/TestUpdateOpenAPI3673042149/001/openapi_operations.yaml" differ:   (
          	"""
          	... // 2 identical lines
          	    documentation_url: https://docs.github.com/rest/a/a#get-a
          	openapi_commit: s
        - 	openapi_operations:
        - 	  - name: GET /a/b/{a_id}
        - 	    documentation_url: https://docs.github.com/rest/reference/a
        - 	    openapi_files:
        - 	      - descriptions/ghec/ghec.json
        - 	      - descriptions/ghes-3.10/ghes-3.10.json
        - 	  - name: GET /a/{a_id}
        - 	    documentation_url: https://docs.github.com/rest/reference/a
        - 	    openapi_files:
        - 	      - descriptions/api.github.com/api.github.com.json
          	"""
          )
    main_test.go:128: To regenerate golden files run `UPDATE_GOLDEN=1 script/test.sh`

I'm not sure what exactly needs to be updated or changed to resolve this. Other tests finished successfully.

[gmlewis]

I'm not sure what exactly needs to be updated or changed to resolve this. Other tests finished successfully.

Ah, I think the OpenAPI definitions may need updating. I'll put together a PR that you should be able to merge in from master, and hopefully that will solve the problem.

[gmlewis]

OK, please pull the latest master branch then merge it into your PR branch and see if that solves the problem. Thanks, and sorry for the troubles.

[kuzminT]

I’ve merged it, but unfortunately it didn’t resolve the problem.
Perhaps there’s something else I need to do. This problem only started after my changes to the DownloadContents method— previously, these tests were passing without any issues.