Skip to content
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

Add params to DELETE #145

Merged
merged 1 commit into from
Nov 20, 2023
Merged

Add params to DELETE #145

merged 1 commit into from
Nov 20, 2023

Conversation

amalagaura
Copy link
Contributor

I am trying to use this REST API which takes query params on DELETE

https://docs.camunda.org/manual/7.5/reference/rest/deployment/delete-deployment/

Made these changes and ran tests locally. But to run tests locally I had to rename MiniTest to Minitest. I am not familiar with Minitest to know about this.

@amalagaura
Copy link
Contributor Author

@balvig I rebased to main. Please let me know any feedback.

@balvig
Copy link
Owner

balvig commented Nov 20, 2023

Hello @amalagaura!
Thank you for this, looks great, and sorry for the slow reply.
I guess I'd just never encountered an API needing query params on DELETE, but this looks a good way to handle those! 👍

@balvig balvig merged commit 129f538 into balvig:main Nov 20, 2023
1 check passed
@amalagaura amalagaura deleted the destroy_params branch November 20, 2023 16:18
@keithxm23
Copy link

This change makes code that used to send a json payload param to a DELETE endpoint into url-params instead. It does not seem possible to send a json-payload with a DELETE request after this change. Am I mistaken?

@amalagaura
Copy link
Contributor Author

@keithxm23 You are right that the HTTP spec shows the DELETE may have a body. Has this changed the behavior to remove the ability to send a payload in the body. From what I remember there was no body and no params before.

@keithxm23
Copy link

keithxm23 commented Apr 11, 2024

The params sent with .where which were not identified as params inside the uri could be sent via a request body.

The Atlassian Bitbucket delete-branch api expects a request body with the HTTP-DELETE request to delete a branch.

My ruby client which was successfully sending request bodies with DELETE looks like this:

class DeleteBranch < Spyke::Base
  self.connection = GIT_BRANCH_UTILS_API
  def self.delete_branch project:, repo:, branch:
     self.with(:branches).where(project: project, repo: repo, name: branch).delete
  end
end

After upgrading beyond 7.1.1 the name: branch param gets added as a url-param.

I think it would be best if the change was reverted because we're losing functionality here. If we want URL-params with a DELETE request, this is possible prior to this change using a custom request like so:

DeleteBranch.request(:delete, 'projects/projectname/repos/reponame/branches?name=asd')

@amalagaura
Copy link
Contributor Author

I am ok to revert. I think it will need some more work. You should just create another PR.

@balvig
Copy link
Owner

balvig commented Apr 15, 2024

Good spot @keithxm23!
Did not consider that scenario 🙇

balvig added a commit that referenced this pull request Apr 15, 2024
balvig added a commit that referenced this pull request Apr 15, 2024
## What

Reverts #145

## Why

#145 unintentionally introduced an unintentional breaking
change for client sending parameters in the body on DELETE requests.
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.

3 participants