Skip to content

Feature: Add support for application/x-www-form-urlencoded encoding #19

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 3 commits into from
Mar 23, 2025

Conversation

Okabintaro
Copy link
Collaborator

Hey there, thanks for making this library!

I was trying to use this extension to talk to an API but found out that the params are always encoded as a JSON object, while the API I was using expected application/x-www-form-urlencoded encoding.

I added a new function http_post_form that has the same signature as the old http_post but uses that encoding instead.
Luckily I was able to reuse a lot of code, since the httplib already has a method for the use case that uses a similar map like the headers that get passed in already.

I found it a bit confusing at first that http_post was posting JSON by default so I noted that down in the README.md too.
But I think it might be a good idea to rename http_post to http_post_json in the future. Let me know what you think.
It might also be a good idea to consider adding support for multipart/form-data and arbitrary text/varchar body for other APIs.

I am open for feedback, but I think this would be a useful feature to talk to more APIs.

This method uses `application/x-www-form-urlencoded` instead of formatting the body as json like `http_post`.
It only tests a single value right now using httpbin but I have seen multiple work just as fine
@Okabintaro Okabintaro force-pushed the feat_post_form_encoding branch from 4039f50 to f538fd8 Compare March 23, 2025 19:19
@lmangani
Copy link
Collaborator

Hello @Okabintaro and first of all thanks for your contribution! I think all of your observations make sense - this was an early extension and leaves a bit to be desired (and designed) but we're glad to welcome other contributors and maintainers to merge and include anything integrations need to make the extension as flexible as possible for as many users and usecases.

@lmangani
Copy link
Collaborator

PS: Don't worry about the builder errors for now, thinks change in DuckDB build land so I'll try to resolve them (unless there's a code issue, ofc)

@lmangani lmangani merged commit a4b9652 into quackscience:main Mar 23, 2025
16 of 26 checks passed
@lmangani
Copy link
Collaborator

Thanks again @Okabintaro for your contribution and time - the PR has been merged and the new version (0.0.7) submitted to the DuckDB Community Repository in (duckdb/community-extensions#335) I also invited you to the repository as co-maintainer in case you need to make other changes.

@Okabintaro
Copy link
Collaborator Author

Wow, thank you for the fast merge and invitation! This will be useful for work very soon.
I might try implementing the other features I mentioned, but I have no immediate need for those so I am not sure when I get around to that.

@lmangani
Copy link
Collaborator

@Okabintaro the latest version is in distribution - could you confirm if it works as desired? Thanks!

@Okabintaro
Copy link
Collaborator Author

I was able to install and use the updated extension from the community repository an hour ago. Thanks again!

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.

2 participants