yanyongyu / githubkit

The modern, all-batteries-included GitHub SDK for Python, including rest api, graphql, webhooks, like octokit!
MIT License
159 stars 21 forks source link

Unexpected behavior when updating organization settings #25

Closed netomi closed 1 year ago

netomi commented 1 year ago

I tried to use the following snippet to update a specific value in my org:

utils.print_debug("updating settings via rest API")
print(f"updating org {data}")
response = self._client.rest.orgs.update(org_id, **data)
utils.print_trace(f"{response.raw_request.url} {response.raw_request.headers} {response.raw_request.content}")

specifically, I wanted to update the web_commit_signoff_required parameter. However, when running the code, it looks like the request contains more parameters that will be updated, and this is also what I experience when looking at the GitHub Web UI. These additional properties got updated as well.

[DEBUG] updating settings via rest API
updating org {'web_commit_signoff_required': True}
[TRACE] https://api.github.com/orgs/OtterdogTest Headers({'host': 'api.github.com', 'accept-encoding': 'gzip, deflate', 'connection': 'keep-alive', 'user-agent': 'GitHubKit/Python', 'accept': 'application/vnd.github+json', 'x-github-api-version': '2022-11-28', 'content-length': '284', 'content-type': 'application/json', 'authorization': '[secure]'}) b'{"default_repository_permission": "read", "members_can_create_repositories": true, "members_can_create_pages": true, "members_can_create_public_pages": true, "members_can_create_private_pages": true, "members_can_fork_private_repositories": false, "web_commit_signoff_required": true}'

Now, I tried to understand from the code what could go possibly wrong but failed to do so. Do you have any insights what could happen here?

netomi commented 1 year ago

Debugging reveals that the provided dict is parsed into a OrgsOrgPatchBody instance. There various fields are by default true, which is what i see in the output. That is a bit surprising, is there a way to update only fields provided via a dict?

netomi commented 1 year ago

I see that in the openapi definition some fields have a default value assigned to them which githubkit honors accordingly. Something I was not fully aware of. Also the behavior of Github does not seem to be always fully in sync with the openapi spec of their API, but it does not look like a problem on the side of githubkit.

yanyongyu commented 1 year ago

githubkit will exclude unset value if it is not required and do not have a default value, as described in the schema.

netomi commented 1 year ago

I realized now that there are default values, thanks to githubkit. However I stumbled upon some weird behavior in github:

I only change the web_commit_signoff_required setting from True to False (I did my own rest requests before, that only changes single fields) for an org, but the members_can_create_private_repositories setting is automatically set to True, even though it does not have a default value specified in the schema. You can do the same thing on the web ui and it will happen as well.

yanyongyu commented 1 year ago

It's really confusing 😂 . If you find some error in the schema, you can report it to https://github.com/github/rest-api-description or github support and open an issue/pr for githubkit. githubkit may temporarily overwrite the schema in configs and correct it.

netomi commented 1 year ago

I created a ticket here: https://github.com/github/rest-api-description/issues/2411

to get some insights on the use of default values. I did some more tests, and I dont see that default values are even considered by the github rest endpoint so its purpose in the rest spec might not match one according to openapi.

There is a cornercase I encountered, but that might be a bug.

netomi commented 1 year ago

btw. looking at go-github from google (https://github.com/google/go-github), no default values are considered when doing updates, so I assume that the intention of default values in the rest model is different to what githubkit considers, but lets wait for the clarification.

yanyongyu commented 1 year ago

We could add exclude_unset=True option to request body transform to exclude these values from passing to request.