Skip to content

GitLab

Closed
Created by Administrator@rootOwner
Report abuse

WIP: Respond with 409 when resources conflict

Created by: dblessing

** Work in progress ** Seeking feedback

Per the API docs, a 409 response should be sent when resources conflict. Right now only one part of the API actually implements this functionality (teams I believe). I believe we should make this work across the API where applicable. This is my first crack at a solution. I am seeking feedback on whether you feel this is a viable solution. If so, I will implement across the API.

Steps to reproduce: Using the API, attempt to create a project with a name or path that already exists.

Expected behavior: This should result in a 409 conflict response.

Observed behavior: A 404 not found response is received.

Errors that arise from conflicting resources are actually sent by ActiveRecord. It sends a hash that looks something like { 'name' => 'has already been taken' } or { 'path' => 'has already been taken' }. So if we check the value for 'has already been taken' we know there's a resource conflict. Then, the key of the has (like 'name') represents what attribute the error is for. Documentation for ActiveRecord validations is at http://api.rubyonrails.org/classes/ActiveRecord/Validations/ClassMethods.html

Thanks for your feedback.

Loading
Loading
  • Administrator
    Administrator @root ·
    Owner

    Created by: coveralls

    Coverage Status

    Coverage increased (+0%) when pulling 44c62f1e on bke-drewb:409 into e7c930a5 on gitlabhq:master.

    By Administrator on 2013-06-14T14:13:09 (imported from GitLab project)

  • Administrator
    Administrator @root started a thread on commit 44c62f1e
    lib/api/projects.rb
    85 85 if @project.errors[:limit_reached].present?
    86 86 error!(@project.errors[:limit_reached], 403)
    87 87 end
    88 handle_activerecord_errors(@project.errors)
    • Administrator
      Administrator @root ·
      Owner

      Created by: dzaporozhets

      not obvious there is a render context here. Also how does it handle both render_api_error!(message, 409) and not_found! together?

      By Administrator on 2013-06-14T16:33:38 (imported from GitLab project)

  • Administrator
    Administrator @root started a thread on commit 44c62f1e
    lib/api/helpers.rb
    60 60 attrs
    61 61 end
    62 62
    63 def handle_activerecord_errors(errors)
    64 attribute, message = errors.first
  • Administrator
    Administrator @root started a thread on commit 44c62f1e
    lib/api/projects.rb
    85 85 if @project.errors[:limit_reached].present?
    86 86 error!(@project.errors[:limit_reached], 403)
    87 87 end
    88 handle_activerecord_errors(@project.errors)
    • Administrator
      Administrator @root ·
      Owner

      Created by: dblessing

      what do you mean by the first part? From looking at error handling in general it seems that once an API error is thrown it stops. Even without this addition the errors are stacked - first limit_reached followed by not_found!

      I can confirm that a 409 is the only response generated by this code in a conflict scenario so only 1 error is getting evaluated.

      By Administrator on 2013-06-14T16:38:06 (imported from GitLab project)

  • Administrator
    Administrator @root started a thread on commit 44c62f1e
    lib/api/helpers.rb
    60 60 attrs
    61 61 end
    62 62
    63 def handle_activerecord_errors(errors)
    64 attribute, message = errors.first
    • Administrator
      Administrator @root ·
      Owner

      Created by: dblessing

      If there are multiple errors, how should it be handled? Currently, it is not handled. For example on project creation, it checks for limit_reached or throws not_found!. If there are multiple errors of the same type we can put them all together in one message, but it gets crazy when we have different types. Open to suggestions...

      By Administrator on 2013-06-14T16:40:01 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: jvanbaarsen

    @bke-drewb Is this still relevant? And are you still working on it?

    By Administrator on 2013-12-06T10:52:56 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: dblessing

    @jvanbaarsen It's definitely still relevant. It would be great to have some dev feedback on the above comments, too. If @randx or another dev wish to comment I will certainly try and get back to this.

    By Administrator on 2013-12-06T19:40:36 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: dzaporozhets

    yeap lets proceed with it. I have no comments to current code. Ping me when it done. We dont merge WIP pull requests :)

    By Administrator on 2013-12-07T12:18:45 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: jvanbaarsen

    @dblessing How is this PR coming along? Do you need feedback or help of any sort?

    By Administrator on 2014-01-10T21:59:31 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: dblessing

    @jvanbaarsen I haven't had much time to look into this further. If you have ideas on how this should be done, please feel free to take this.

    By Administrator on 2014-01-12T21:58:32 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: jvanbaarsen

    @Razer6 Maybe we can close this PR until @dblessing has more time to finish it?

    By Administrator on 2014-01-29T17:28:54 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: dblessing

    I'm OK with that. The way I understand it a new API version is in the works so hopefully this will be considered as part of that effort, too. Then this would be unnecessary.

    By Administrator on 2014-01-29T22:37:28 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: Razer6

    Ok then I close it

    By Administrator on 2014-01-30T06:54:58 (imported from GitLab project)

Please register or sign in to reply

Menu

Help