fastai / ghapi

A delightful and complete interface to GitHub's amazing API
https://ghapi.fast.ai/
Apache License 2.0
527 stars 57 forks source link

Issues in the api wrappers while downloading logs #119

Open riccardo-di-lorenzo opened 2 years ago

riccardo-di-lorenzo commented 2 years ago

Hi, I stumbled upon this 2 bugs while trying out your project. It seems to me that those 2 functions that present the issue (actions.download_workflow_run_logs and actions.list_jobs_for_workflow_run) are working under the assumption that they're going to receive json, which is not the case. The first will receive a zip file (in the form of a byte array), while the second will receive the raw logs in the body of the response.

To reproduce:

from ghapi.all import GhApi

api = GhApi(owner='fastai', repo='fastcore')
res_runs = api.actions.list_workflow_runs_for_repo(status='completed',per_page=5,page=1) # last 5 workflow_runs of your proj

for run in res_runs.workflow_runs.items: 
    try:
        res = api.actions.download_workflow_run_logs(run.id) # gets a zip (byte array)
    except Exception as bug1:
        print(bug1)

    res_jobs = api.actions.list_jobs_for_workflow_run(run_id=run.id)
    for job in res_jobs.jobs.items:
        try:
            res = api.actions.download_job_logs_for_workflow_run(job.id) # gets plain text
        except Exception as bug2:
            print(bug2)

The problem to me seems that the urlsend() method in the fastcore dependency does not care about the response type, but tries to json decode the content, no matter what. Which fails because the Content-Type is, respectively, plain/text and application/zip

riccardo-di-lorenzo commented 2 years ago

The issue seems to be here: https://github.com/fastai/fastcore/blob/938135a5b6e419e2dc8bccca1e9cc32b1c9ff070/fastcore/net.py#L209 urlsend should not have a parameter that decides whether or not json decode the response, but it should be something that it determines on its own, by looking at what is the content type of the response. Then it could be up to the users to write a decoder for anything that is not a json (or provide an helper if we're being ambitious).

I can see why the assumption was made, as there is an API that downloads an artifact and it allows a parameter to decide its format (making it quite obvious that we're going to get some kind of file), but in other cases, like the one where we download the logs, we don't have this kind of luxury.

I'm not quite sure why this specific corner case is implemented in the fastcore library HERE, but it seems to me we shouldn't count on it in here. Maybe we should set the flags decode=False, return_json=False if the function we call contains the word "download"?