Skip to content
Snippets Groups Projects

WIP: Adds requirements that supports anything in sha params

Open WIP: Adds requirements that supports anything in sha params
gvieira37/gitlab-ce:sha-handling into master
3 unresolved threads
Open username-removed-1616781 requested to merge gvieira37/gitlab-ce:sha-handling into master
3 unresolved threads

What does this MR do?

Fix the 404 error in API when the branch has a dot in its name as stated in #26561 (moved) and in #2709 (moved).

Why was this MR needed?

Since we use Grape gem, every param we define for a given endpoint is restricted to a default format. This default format does not allow dots in value, so whenever we have a :sha param we are going to explicitly allow any kind of text for it to work. This has been already done for some endpoints, so I just replicated the strategy for the other cases.

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Closes #26561 (moved), #2709 (moved)

Edited by username-removed-1616781

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • username-removed-1616781 resolved all discussions

    resolved all discussions

  • username-removed-1616781 marked the checklist item Has been reviewed by Backend as completed

    marked the checklist item Has been reviewed by Backend as completed

  • username-removed-1616781
    username-removed-1616781 @gvieira37 ·
    Author Contributor

    @markglenfletcher Do you think this can be merged or should I do anything else?

  • username-removed-419655
    username-removed-419655 @markglenfletcher1 ·
    Developer

    @godfat Please can you review it?

  • username-removed-423915
    username-removed-423915 @godfat ·
    Maintainer

    @gvieira37 Well done, looks good to me. On the other hand, what do you think about that we also add a similar test for v4 (current) API, like the one for spec/requests/api/v3/repositories_spec.rb? I guess it's fine not testing all the routes you touched for v4 for now, but let's at least add something for v4?

  • username-removed-423915 assigned to @gvieira37

    assigned to @gvieira37

  • username-removed-423915 changed the description

    changed the description

  • username-removed-1616781 marked as a Work In Progress

    marked as a Work In Progress

  • username-removed-1616781
    username-removed-1616781 @gvieira37 ·
    Author Contributor

    @godfat Sure! I'm gonna take a look on this tests for v4. Thanks for reviewing.

  • username-removed-1616781
    username-removed-1616781 @gvieira37 ·
    Author Contributor

    @godfat Sorry for my late response. I took a look and I found out that this "filepath" API was moved to files API (lib/api/files.rb) in V4 and we have similar tests for this cases in spec/requests/api/files_spec.rb. Of course things have changed, but I believe it covers everything V3 spec does. WDYT?

  • username-removed-423915
    username-removed-423915 @godfat started a thread on the diff
    • lib/api/repositories.rb
    69 69 params do
    70 70 requires :sha, type: String, desc: 'The commit, branch name, or tag name'
    71 71 end
    72 get ':id/repository/blobs/:sha' do
    72 get ':id/repository/blobs/:sha', requirements: API::COMMIT_ENDPOINT_REQUIREMENTS do
    • username-removed-423915
      username-removed-423915 @godfat ·
      Maintainer

      I just realized that, it seems that this API, the :sha, cannot be any ref, and it could only be the OID of a blob? If that's the case, we don't need this requirement, and we might need to update the description.

      What do you think?

    • username-removed-1616781
      username-removed-1616781 @gvieira37 ·
      Author Contributor

      @godfat I followed the assign_blob_vars! and it feels like maybe it can be a ref name as well. Also, I did some tests and it actually works with a tag name, even that it is not actually correct to assign a tag to a blob sha. Also, even the params definition a few lines above and the docs says it has to be "The commit or branch name ". Anyway, it feels wrong to me, so I think I'm gonna remove the requirement I added and fix de docs. Does it still makes sense for you?

    • Please register or sign in to reply
  • username-removed-423915
    username-removed-423915 @godfat started a thread on the diff
    • lib/api/commit_statuses.rb
    21 21 optional :all, type: String, desc: 'Show all statuses, default: false'
    22 22 use :pagination
    23 23 end
    24 get ':id/repository/commits/:sha/statuses' do
    24 get ':id/repository/commits/:sha/statuses', requirements: API::COMMIT_ENDPOINT_REQUIREMENTS do
  • username-removed-423915
    username-removed-423915 @godfat started a thread on the diff
    • lib/api/v3/builds.rb
    47 47 use :optional_scope
    48 48 use :pagination
    49 49 end
    50 get ':id/repository/commits/:sha/builds' do
    50 get ':id/repository/commits/:sha/builds', requirements: API::COMMIT_ENDPOINT_REQUIREMENTS do
  • username-removed-423915
    username-removed-423915 @godfat ·
    Maintainer

    @gvieira37 You're right, I think we don't need more tests for v4, however after some investigation, I also realized that we don't need to change some API for v4, as they're really just for SHA. Let's remove them then.

  • username-removed-1616781
    username-removed-1616781 @gvieira37 ·
    Author Contributor

    @godfat Nice. At first, I thought sha could only be hexa, but then I found some places where sha could also be a branch or tag name. So I assumed this was right. Anyway, I'll take care of this. Thank you again for reviewing it and helping me not mess with the code.

  • username-removed-423915
    username-removed-423915 @godfat ·
    Maintainer

    @gvieira37 Yeah, we're not very consistent here yet. Thank you for checking all the possible places we should also fix, that's really nice.

  • username-removed-1616781
    username-removed-1616781 @gvieira37 ·
    Author Contributor

    @godfat No problem at all. I know this can be hard sometimes. I was looking at git ref-parse docs and found out they refer to this args that allow anything as rev. If you would like, I can rename does places where this is accepted from sha to rev. Of course this is not gonna make any difference to the API user, but can avoid conflicts in the future. If that is the case, maybe it would be better to make this in another MR, so the change can be as small as possible.

  • username-removed-423915
    username-removed-423915 @godfat ·
    Maintainer

    @gvieira37 Yes, let's separate the changes so that it's easier to review (and easier to revert if needed). On the other hand, it's not just doc, some of them are API parameters, so we can't just change them. We also need to make sure they're backward compatible, for example.

  • username-removed-1616781
    username-removed-1616781 @gvieira37 ·
    Author Contributor

    @godfat Sure. I'm gonna be careful with this. Thank you for the hint.

    • Please register or sign in to reply