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.

Sign up for GitHub

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

Jump to bottom

Errors loading config larger than 1MB #227

Open
bluekeyes opened this issue Feb 16, 2023 · 2 comments · May be fixed by #248
Open

Errors loading config larger than 1MB #227

bluekeyes opened this issue Feb 16, 2023 · 2 comments · May be fixed by #248

Comments

@bluekeyes
Copy link
Member

I think this has been broken since we introduced the appconfig package, but it's also possible that GitHub's API changed. The GetContents method returns content if the file is less than 1MB and an empty content field in the JSON for content between 1MB and 100MB. The endpoint returns an error for content over 100MB.

We handle the case where the content is larger than 100MB, but do not handle the case where the content is larger than 1MB. Instead, we return an error when we try to decode content with the none encoding.

When the content field is empty, I think there are two options:

  1. Make another request to the "Get repository contents" API, but provide the raw media type
  2. Switch to using the DownloadContents method, like we do when the file is larger than 100MB

The reason to not use DownloadContents every time is that it makes two API calls: one to list the directory containing the file, and then one to download the file. Since most apps perform configuration lookups all the time, minimizing requests here is a good idea.
@SoulPancake
Copy link

@bluekeyes
Can we do something like

func getFileContents(ctx context.Context, client *github.Client, owner, repo, ref, path string) ([]byte, bool, error) {
    file, _, _, err := client.Repositories.GetContents(ctx, owner, repo, path, &github.RepositoryContentGetOptions{
        Ref: ref,
    })
    if err != nil {
        switch {
        case isNotFound(err):
            return nil, false, nil
        case isTooLargeError(err):
            b, err := getLargeFileContents(ctx, client, owner, repo, ref, path)
            return b, true, err
        }
        return nil, false, errors.Wrap(err, "failed to read file")
    }

    // file will be nil if the path exists but is a directory
    if file == nil {
        return nil, false, nil
    }

    if file.GetSize() > 100*1024*1024 {
        b, err := getLargeFileContents(ctx, client, owner, repo, ref, path)
        return b, true, err
    } else if file.GetSize() > 1024*1024 {
        rawFile, _, _, err := client.Repositories.GetContents(ctx, owner, repo, path, &github.RepositoryContentGetOptions{
            Ref: ref,
            Media: "application/octet-stream",
        })
        if err != nil {
            return nil, false, errors.Wrap(err, "failed to read file content")
        }
        content, err := rawFile.GetContent()
        if err != nil {
            return nil, true, errors.Wrap(err, "failed to decode file content")
        }
        return []byte(content), true, nil
    }

    content, err := file.GetContent()
    if err != nil {
        return nil, true, errors.Wrap(err, "failed to decode file content")
    }

    return []byte(content), true, nil
}

@bluekeyes
Copy link
Member Author

Hey @SoulPancake, thanks for the suggestion. I think we can simplify this further by making one request with the application/vnd.github.raw content type, checking for the file size error, and then using the getLargeFileContents method in that case. The raw content type is supposed to work for all files up to 100MB, although I haven't tested it yet to make sure it still returns the expected error for files larger than 100MB.

Also, it looks like the Media property is not a field in the RepositoryContentGetOptions, at least in go-github v50.2.0. Are you looking at a different version of the library? I think we'll probably have to fall back to the lower-level methods provided by go-github for this request.

@SoulPancake SoulPancake linked a pull request Apr 7, 2023 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Handling error loading config > 1MB SoulPancake/go-githubapp
2 participants
@bluekeyes @SoulPancake