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

Provide IFile consumers a rudimentary HTTP POST option #1650

Merged
1 commit merged into from Oct 25, 2012

Conversation

Red-F
Copy link
Contributor

@Red-F Red-F commented Oct 20, 2012

This PR replaces original #1572.

The implementation has been moved to a subclass of CurlFile to address comments made both in the original PR #1572 and a colliding PR #1567.

Fixes made to CurlFile in the original PR were maintained.

Notice: Since I have no (easy) access to an OS X system at this moment I was not able to add the new CHTTPFile class implementation to the various .xcodeproj 's

Original description:

Currently binary addons can not access and use the full functionality provided by CCurlFile to access a webservice.

eg. PVR addons can use a simple GET via the IFile interface by using the sequence:

Open
ReadString or Read (until data is exhausted)
Close

This change extends that with a very rudimentary support for POST with post data via the IFile interface by using the sequence:

OpenForWrite
Write (postdata)
ReadString or Read (until data is exhausted)
Close

What's not pretty? Content-Type is hard coded to application/json at this time. In a future version this might be configurable by allowing headers to be added via IoControl (IOCTRL_NATIVE).

fix: allow post with postdata length of zero bytes
fix: SetMimeType didn't work
added: implementation of IFile's OpenForWrite and Write methods

@elupus
Copy link
Contributor

elupus commented Oct 22, 2012

m_httpresponse need init in constructor

@@ -21,7 +21,6 @@
#include "CurlFile.h"
#include "utils/URIUtils.h"
#include "Util.h"
#include "URL.h"

This comment was marked as spam.

This comment was marked as spam.

@opdenkamp
Copy link
Member

+1 from me, but will need to get an ok from @cptspiff first (and squashed before merging ofc)

@Red-F
Copy link
Contributor Author

Red-F commented Oct 23, 2012

Huh?

@ghost
Copy link

ghost commented Oct 23, 2012

what's the huh addressing? the C H D? where i'm from it's a b c d e f g h.

@Red-F
Copy link
Contributor Author

Red-F commented Oct 23, 2012

Ah, sorry, I've just added HTTPFile close to it's parent. Didn't notice the alphabetical ordering, and since CHD usually means "Coronary heart disease" I was a little taken aback :)

And once again I really would like to point out that this is only to provide very minimal post functionality for PVR addons that are limited to using IFile. It is not pretty, it is not clean and it definitely is not the way I would like to engineer it. It's a cornered cat trying to find a solution since it can not instantiate a CurlFile within a PVR addon.

Cheers,
Fred

@ghost
Copy link

ghost commented Oct 23, 2012

yeah, we all know this is a bandaid. fix up the makefile, squash it up and i'll pull.

@Red-F
Copy link
Contributor Author

Red-F commented Oct 23, 2012

ok, when I get home from work I'll rebase and squash

@Red-F
Copy link
Contributor Author

Red-F commented Oct 23, 2012

Git's power never ceases to amaze me. Just when I think I understand, I manage to do something that behaves not quite as expected. Yet I've never lost history sofar.

Anyway, after 'git rebase -i master' the above is what I ended with. Commit edbf643 is indeed the squashed set of commits I made (and I trust the 'H' is now properly located ;)).

@opdenkamp
Copy link
Member

awesome :P and now you need to fix this up and never do a rebase onto another branch anymore and force-pushing to your PR branch :)

something like this:

git checkout -b tmp
git branch -D httpfile_post
git checkout -b httpfile_post master
git cherry-pick edbf643
git push -f origin httpfile_post

@Red-F
Copy link
Contributor Author

Red-F commented Oct 23, 2012

Thanks Lars, I knew I simply had to continue branching, cherry picking and forcing updates, but was a little lost how to reduce the number of commits instead of expand...

Copied and pasted to the iron clad memory of my evernote :)

@opdenkamp
Copy link
Member

thanks, but can you please update the description to something more descriptive (git commit --amend). thanks

… sub-class called HTTPFile.

Implements IFile's OpenForWrite and Write methods to POST data with Content-Type set to "Application/Json"
ghost pushed a commit that referenced this pull request Oct 25, 2012
Provide IFile consumers a rudimentary HTTP POST option
@ghost ghost merged commit 93b9549 into xbmc:master Oct 25, 2012
using namespace XFILE;

CHTTPFile::CHTTPFile(void)
{

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@Red-F
Copy link
Contributor Author

Red-F commented Oct 26, 2012

Yes, we can move it. Actually I think we really should initialise the m_postdata and m_postdataset within CCurlFile::Post() as well as CCurlFile::Get() and re-factor CCurlFile::Service without the postdata argument. That way it would more be like prepare request -> execute request in both cases.

You want me to do that and either make a new PR or update this one?

Of course the real underlying problem is using side effects to control the Open() method ;).

@koying
Copy link
Contributor

koying commented Oct 26, 2012

As this one has already been merged and been closed, you have to create a new PR, IMO

@Red-F
Copy link
Contributor Author

Red-F commented Oct 26, 2012

np

@Red-F Red-F mentioned this pull request Oct 26, 2012
@Red-F
Copy link
Contributor Author

Red-F commented Oct 26, 2012

Fixed as described in pull request #1681

@FernetMenta
Copy link
Contributor

@Red-F @opdenkamp would you know if this is still used by any addon? @mapfau would like to clean this up #9469

@opdenkamp
Copy link
Member

no sorry. it was only used by one add-on, fortherecord, for logging in. but that may have changed by now

return -1;

// Finally (and this is a clumsy hack) return the http response code
return (int) m_httpresponse;

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This pull request was closed.
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.

None yet

7 participants