Fix Repository Archive API Format Bug. #244
Conversation
this bug is caused by gitlab-ce itself, but there is a solution to request .../archive.:format instead of .../archive?format=:format.
There was a problem hiding this comment.
@zhengrenjie
First off thank you for the fix.
I would suggest that checkFormat(String format) simply checks the validity of the format and throws an exception if not a valid format, instead of it returning the format with a "." prepended, the method name does not infer that.
I've suggested code changes below.
| private String checkFormat(String format) throws GitLabApiException { | ||
|
|
||
| if(format == null) | ||
| throw new GitLabApiException("format should not be null"); |
There was a problem hiding this comment.
format should default to "tar.gz" is null or empty and not throw an exception.
| return ".".concat(format); | ||
| else | ||
| return ".tar.gz"; | ||
| } |
There was a problem hiding this comment.
If the format is not valid, why not throw a GitLabApiException indicating an invalid format? The user may be wanting something totally different than "tar.gz" and just misspelled it, which should result in an exception.
| Response response = getWithAccepts(Response.Status.OK, formData.asMap(), MediaType.MEDIA_TYPE_WILDCARD, | ||
| "projects", projectId, "repository", "archive"); | ||
| "projects", projectId, "repository", "archive".concat(checkFormat(format))); |
There was a problem hiding this comment.
I believe it would be more understandable to do the following:
// Throws a GitLabApiException if format is invalid
format = checkFormat(format);
Form formData = new GitLabApiForm().withParam("sha", sha);
Response response = getWithAccepts(Response.Status.OK, formData.asMap(),
MediaType.MEDIA_TYPE_WILDCARD, "projects", projectId, "repository", "archive", ".", format);
| Response response = getWithAccepts(Response.Status.OK, formData.asMap(), MediaType.MEDIA_TYPE_WILDCARD, | ||
| "projects", projectId, "repository", "archive"); | ||
| "projects", projectId, "repository", "archive".concat(checkFormat(format))); |
There was a problem hiding this comment.
I believe it would be more understandable to do the following:
// Throws a GitLabApiException if format is invalid
format = checkFormat(format);
Form formData = new GitLabApiForm().withParam("sha", sha);
Response response = getWithAccepts(Response.Status.OK, formData.asMap(),
MediaType.MEDIA_TYPE_WILDCARD, "projects", projectId, "repository", "archive", ".", format);
| throw new GitLabApiException("format should not be null"); | ||
|
|
||
| if(validFormat.contains(format)) | ||
| return ".".concat(format); |
There was a problem hiding this comment.
Simply return format and let the caller prepend the ".", this functionality is not obvious in the method name.
2. Revised getRepositoryArchive method to make it more understandable.
|
Thank you for your best suggestions. I have revised the PR according to your suggestion. If there is anything wrong, I will fix it as soon as I can. |
|
@zhengrenjie |
|
@zhengrenjie BTW, I added methods where you can now also specify an enum (Constants.ArchiveFormat) for the archive format. This enum is also used to validate the format when provided as a String. |
If I try to visit
/api/v4/projects/*/repository/archive?sha=master&format=zip,I will get "406 Not Acceptable" error.
Then I found this :
https://gitlab.com/gitlab-com/support-forum/issues/3067
https://gitlab.com/gitlab-org/gitlab-ce/issues/45992
This bug is caused by gitlab-ce itself, but there is a solution to request .../archive.:format instead of .../archive?format=:format. So I have changed the origin RepositoryApi.java a little to adjust to the "../archive.:format" way.