python-gitlab / python-gitlab

A python wrapper for the GitLab API.
https://python-gitlab.readthedocs.io
GNU Lesser General Public License v3.0
2.23k stars 651 forks source link

ProjectFileManager does not implement head method to meet API spec #2976

Closed holysoles closed 3 days ago

holysoles commented 6 days ago

Description of the problem, including code/CLI snippet

Attempting to use a HEAD request to retrieve file metadata from a project repository (Gitlab REST API docs) fails with a 404

dest_file_get_args = {
  'file_path': file_def['dest_filepath'],
  'ref': 'HEAD',
}
existing_file = project.files.head(**dest_file_get_args)

Expected Behavior

Should return 200 and file metadata. The same request as a GET request returns 200, and they follow the same URL path and parameter requirements. Sending a GET request logs this URL path:

DEBUG:urllib3.connectionpool:https://gitlab.com:443 "GET /api/v4/projects/1/repository/files/somefile.txt?ref=HEAD HTTP/11" 200 418

Actual Behavior

A HTTP HEAD request is sent with the following URL path: DEBUG:urllib3.connectionpool:https://gitlab.com:443 "HEAD /api/v4/projects/1/repository/files?file_path=somefile.txt&ref=HEAD HTTP/11" 404 0

Specifications

JohnVillalovos commented 6 days ago

That doesn't seem to be a HEAD request. It is a GET request that is asking for the ref called HEAD

holysoles commented 6 days ago

@JohnVillalovos its a HEAD request asking for the HEAD ref. I can generate additional logs for other refs.

holysoles commented 6 days ago

Working GET for master ref DEBUG:urllib3.connectionpool:https://gitlab.com:443 "GET /api/v4/projects/1/repository/files/somefile.txt?ref=master HTTP/11" 200 417

Failing HEAD for master ref DEBUG:urllib3.connectionpool:https://gitlab.com:443 "HEAD /api/v4/projects/1/repository/files?file_path=somefile.txt&ref=master HTTP/11" 404 0

holysoles commented 6 days ago

I'm pretty sure this is because GetMixin is inheriting HeadMixin and in ProjectFileManager we are overriding the get method, but that wouldn't override the default head method as far as I'm aware

JohnVillalovos commented 6 days ago

As a note. Your python-gitlab version is fairly old.

This almost seems like an issue with GitLab. But not sure. Would be interesting to try a curl test using both GET and HEAD. If only HEAD fails then something wrong with GitLab. If they both work then something happening with python-gitlab.

Edit: Notice that the URLs set are slightly different. So pointing more toward python-gitlab

nejch commented 6 days ago

I think https://github.com/python-gitlab/python-gitlab/issues/2976#issuecomment-2343979194 is right as you can see the file path gets passed as a query param unfortunately in the head() call. I guess we can't rely on this being consistent across the API endpoints.

holysoles commented 6 days ago

@JohnVillalovos i can update my version and test again, but I've tested this commit and it works. Happy to open a PR if appropriate

Wasnt sure if i should explicitly inherit HeadMixin or not in it.

JohnVillalovos commented 6 days ago

@JohnVillalovos its a HEAD request asking for the HEAD ref. I can generate additional logs for other refs.

Thanks. I was confused as original message had

DEBUG:urllib3.connectionpool:[https://gitlab.com:443](https://gitlab.com/) "GET /api/v4/projects/1/repository/files/somefile.txt?ref=HEAD HTTP/11" 200 418
holysoles commented 6 days ago

Also apologies, my version is 4.9.0, i grabbed the wrong dependency version from my requirements file

JohnVillalovos commented 6 days ago

@JohnVillalovos i can update my version and test again, but I've tested this commit and it works. Happy to open a PR if appropriate

Wasnt sure if i should explicitly inherit HeadMixin or not in it.

PR sounds like a good idea. Thanks.

nejch commented 6 days ago

@JohnVillalovos @holysoles might also be a good idea to not inherit from GetMixin in that case if we're overriding it, to avoid this kind of confusion in the future. Not sure if there's any other implication there.

holysoles commented 5 days ago

I should be able to get the PR opened tomorrow after I functionally test my update, but its passing unit tests