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

Support and automatically detect multipart encoding for parameters #15

Merged
merged 9 commits into from
Jul 3, 2020
Merged

Support and automatically detect multipart encoding for parameters #15

merged 9 commits into from
Jul 3, 2020

Conversation

mashedkeyboard
Copy link
Contributor

Fixes #14. Any request that has a parameter value of string length greater than 8000 will be converted automatically to a POSTed multipart request. All MW API endpoints support this sort of request, so there shouldn't be any issue with it being POST and not GET (I checked this with Reedy from the MediaWiki dev team) - and it's handled automatically by the library, so it oughtn't get in the way of any implementations.

Note that, whilst I've written tests for the new methods and all seems to be working well, I've not tested this against the actual MediaWiki API yet - mostly because it's a huge pain to do so without Go Modules in the repository, as it seems to mean that to override the package in testing, you have to change the //import comments, then change all of the actual imports too, to make it somewhere on your hard disk. I may be missing something obvious though! Either way, this should do what it says on the tin (and Golang's multipart parser is perfectly happy with it, per the tests).

@cgt cgt self-assigned this Jul 2, 2020
@cgt
Copy link
Owner

cgt commented Jul 2, 2020

Thank you, I will review this tomorrow. Do you agree to release your contribution under the terms of the Creative Commons CC0 1.0 Public Domain Dedication? (See COPYING file in the repository)

As for testing: If you have the package on your GOPATH under the canonical import path (cgt.name/pkg/go-mwclient), then you can checkout your PR branch in that location and you should be able to use it normally. Alternatively, you could set up a different GOPATH just for that purpose. I haven't used modules much, and none at all for developing libraries, so I'm not sure if there's anything that needs to be done to go-mwclient to make it work in that context, or if it would make it easier to use somehow.

@mashedkeyboard
Copy link
Contributor Author

Yes, I am happy to release my contribution under CC0 1.0.

I have the package on my GOPATH at that location, but changing the stuff there doesn't seem to want to change anything when I'm using it; it still seems to be using the old version. I'm not sure what's going on there; I suspect it's to do with having installed it through go modules for my project using go get.

This also reorders the params test parameters, to make sure that token ordering actually happens; before, there was technically nothing to check that, as the parameters were provided in the right order.
@mashedkeyboard
Copy link
Contributor Author

Looking at this today, I realised I managed to mix up form fields and multipart headers... whoops facepalms

Fixed that mistake in 99d0ac8, and I've now tested the output against the actual MW API too, which all seems to be working well.

params/params.go Outdated Show resolved Hide resolved
core.go Outdated Show resolved Hide resolved
core.go Show resolved Hide resolved
core.go Outdated Show resolved Hide resolved
core.go Outdated Show resolved Hide resolved
params/params.go Outdated Show resolved Hide resolved
core_test.go Outdated Show resolved Hide resolved
@mashedkeyboard
Copy link
Contributor Author

I've resolved the code conversation because this isn't specifically about that, but the build failure here is the same issue I was having. Because params is technically a separate package, GitHub, like me, is getting the live version of it, and trying to compile the PR with that, rather than the new version that the PR contains.

@cgt
Copy link
Owner

cgt commented Jul 3, 2020

I believe I have resolved the problem with the CI build, but I think GitHub will keep using version that you have in your branch. Would you mind merging my master branch into your PR branch and pushing again? Just so I can see if it really works. Thanks.

Also, please add yourself to the end of the list in the CONTRIBUTORS file.

Other than that, everything looks good. I just tested the new version with my wikibot programs and it works fine. Even tried with a modified version that forces it to always use multipart POST. No problems.

Once we've verified that the CI build issue is resolved and you've added yourself to CONTRIBUTORS, I'll merge the PR, update the changelog, and tag a new release. Thank you so much for your contribution. It's been a pleasure working with you on this PR.

@mashedkeyboard
Copy link
Contributor Author

Likewise! 😄

Merge complete, and name added to CONTRIBUTORS - looks like it's working now, thankfully!

@cgt cgt merged commit 241f518 into cgt:master Jul 3, 2020
@mashedkeyboard
Copy link
Contributor Author

Thanks very much for merging @cgt! Would you mind creating a new release on GitHub, with a tag for v1.2.0? The GH releases are what go modules goes off when checking for updated versions of the library 😄

@cgt
Copy link
Owner

cgt commented Jul 3, 2020

Sorry, I created the tag, but forgot to push it. It's done. 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.

Support multipart/form-data for large edits
2 participants