Skip to content

fix(api): head requests for projectfilemanager #2977

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.

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

Merged
merged 2 commits into from
Sep 14, 2024

Conversation

holysoles
Copy link
Contributor

Changes

Closes #2976

The ProjectFileManager class current has a head method, which throws a 404 error when used, as it doesn't have the same logic for URL path construction that its get method has. This is because it is inheriting from GetMixin, which inherits from HeadMixin. The benefit of having the HEAD request properly support is you can inspect and compare the file without needing to fully download it, as the actual file content is not returned.

This change:

  • Removes the ProjectFileManager inheritance of GetMixin, since we're not making proper use of that class.
  • adds a proper head method to ProjectFileManager that correctly constructs the API query
  • Adds a unit test for the new head method based on mocked real-world headers

Note that because the HEAD request returns the requested data as headers and not a JSON object, we need to map the keys to the expected ones for ProjectFile. I did this in a dynamic way, but happy to re-implement it with a hardcoded map of expected values.

Documentation and testing

Please consider whether this PR needs documentation and tests. This is not required, but highly appreciated:

@JohnVillalovos
Copy link
Member

Also looks like it needs to be rebased against the current main branch. Thanks.

@holysoles
Copy link
Contributor Author

So looks like an existing functional test is failing because it expects the return value of headers to be a dict rather than a ProjectFile. Should I conform to that behavior or should I update the test based on me now constructing a ProjectFile from the metadata? I assume the former but I feel like the latter is more usable, but arguably a breaking change.

https://github.com/python-gitlab/python-gitlab/blob/main/tests/functional/api/test_repository.py#L43

@nejch
Copy link
Member

nejch commented Sep 12, 2024

So looks like an existing functional test is failing because it expects the return value of headers to be a dict rather than a ProjectFile. Should I conform to that behavior or should I update the test based on me now constructing a ProjectFile from the metadata? I assume the former but I feel like the latter is more usable, but arguably a breaking change.

https://github.com/python-gitlab/python-gitlab/blob/main/tests/functional/api/test_repository.py#L43

@holysoles our client's http_head method (and also head()) returns just the headers dict without really transforming them, so I think we should either keep it consistent with that, or change it everywhere. IMO it's best to leave this as-is for now, so it's a bit more generic, and discuss that for all head methods in a follow-up.

But I'm a bit confused our existing tests work as intended as opposed to your issue description, it seems like the issue there is actually your use of keyword args, but I'll have to check in more detail when I get home.

@holysoles
Copy link
Contributor Author

@nejch now that you say that, the keyword args do seem like the problem. I think my confusion initially stemmed from the official REST API docs indicate that there isnt a difference in how the API call should be made, but here, the user needs to pass id=my_file_path compared to file_path=my_file_path.

Given all this, does it make sense to not transform the headers dict and just return it, but still proceed with the other changes so the consistency of the parameter name can be improved?

@nejch
Copy link
Member

nejch commented Sep 13, 2024

@nejch now that you say that, the keyword args do seem like the problem. I think my confusion initially stemmed from the official REST API docs indicate that there isnt a difference in how the API call should be made, but here, the user needs to pass id=my_file_path compared to file_path=my_file_path.

Given all this, does it make sense to not transform the headers dict and just return it, but still proceed with the other changes so the consistency of the parameter name can be improved?

Yes, I think that makes sense. TBH I'm questioning whether we really need to even be overriding the GetMixin for the file manager because we technically already kind of cover this via the _id_attr = "file_path" and we could just document usage, but this is historical and not part of this PR.

For now IMO best to just align the head() override with the get() one and keep whatever is returned from http_head. They are HTTP headers in the end and not really one-to-one replacement for ProjectFile attributes so this could also be confusing in some cases otherwise. WDYT?

Copy link

codecov bot commented Sep 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.61%. Comparing base (10c7aee) to head (5afd332).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2977   +/-   ##
=======================================
  Coverage   96.60%   96.61%           
=======================================
  Files          95       95           
  Lines        6072     6080    +8     
=======================================
+ Hits         5866     5874    +8     
  Misses        206      206           
Flag Coverage Δ
api_func_v4 82.63% <100.00%> (-0.03%) ⬇️
cli_func_v4 82.94% <36.36%> (-0.08%) ⬇️
unit 88.73% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
gitlab/v4/objects/files.py 100.00% <100.00%> (ø)

Copy link
Member

@nejch nejch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again @holysoles for the PR and the rework, just had a small nit in the docstring that I applied directly so we can go ahead with this.

@nejch nejch enabled auto-merge (squash) September 14, 2024 16:17
@nejch nejch merged commit 96a18b0 into python-gitlab:main Sep 14, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ProjectFileManager does not implement head method to meet API spec
3 participants