Created attachment 341958 [details] [review]
goa-authorizer: Append access token in SoupMessage header for multipart POST

Created attachment 341959 [details] [review]
photo: Add file uploading support

Currently, the patches will not work with access_token from GOA. I have tested the patches with access_token taken from Graph Explorer tool [1] and checked their working. We still need to investigate why they do not work with GOA key.

The specific problem is that access_token from GOA does not has "publish_actions" permission when (checked using cURL). But looking at GOA source, we do request "publish_actions" permission.

[1] https://developers.facebook.com/tools/explorer/

Created attachment 360437 [details] [review]
gfbgraph-photo: Add file uploading support

New version of patches. We start independently using GFBGraphSimpleAuthorizer. We will worry about gnome platform (gnome-online-accounts) later. First, let's get it working stand-alone.

In this patch: 
* Introduce a new property "mime_type"
* a _new function which returns GFBGraphPhoto from file source (i.e. URI)
* Basic uploading logic to upload image file to Facebook.

Please use this patch in conjunction with the following test-program.

Created attachment 360443 [details]
test-program (gtk_simple_auth.c)

Test program to test the above patch.

Review of attachment 360437 [details] [review]:

::: gfbgraph/gfbgraph-photo.c
@@ +129,2 @@
         /**
+         * GFBGraphPhoto:mime_type:

I wonder if can't you auto-detect the mime type from the passed-in source URI, or the photo itself.

@@ +222,3 @@
+                case PROP_MIME_TYPE:
+                        if (priv->mime_type)
+                                g_free (priv->mime_type);

Use g_clear_pointer()

@@ +223,3 @@
+                        if (priv->mime_type)
+                                g_free (priv->mime_type);
+                        priv->mime_type = g_strdup (g_value_get_string (value));

You can use g_value_dup_string()

@@ +405,3 @@
+/**
+ * gfbgraph_photo_new_from_file_source:
+ *

Parameters are missing

@@ +661,3 @@
+    priv = GFBGRAPH_PHOTO_GET_PRIVATE (self);
+
+    GFile *file;

There should be no need to use g_strconcat() here; AFAICS url can be a const char *

@@ +671,3 @@
+    basename = g_file_get_basename (file);
+
+    gchar *url = NULL;

This strikes me as an odd way of loading the data... A more conventional way of loading the contents would be to use g_file_load_contents() or g_file_get_contents()

Hi Umang!

Thanks for take the time to do this, it's a really useful feature!

(In reply to Umang Jain from comment #4)
> Created attachment 360437 [details] [review] [review]
> gfbgraph-photo: Add file uploading support
> 
> New version of patches. We start independently using
> GFBGraphSimpleAuthorizer. We will worry about gnome platform
> (gnome-online-accounts) later. First, let's get it working stand-alone.

I think that it's a good idea that libgfbgraph doesn't depends on goa, the simple authorizer will be enough for the moment. We can create a goa authorizer in the future, as an extra.

> 
> In this patch: 
> * Introduce a new property "mime_type"
> * a _new function which returns GFBGraphPhoto from file source (i.e. URI)
> * Basic uploading logic to upload image file to Facebook.

I'm going to review your patch, but in a first look I just see that you don't use the FACEBOOK_ENDPOINT string for the URL and it will be a must.

> 
> Please use this patch in conjunction with the following test-program.

Really helpful. It will be great to have a GTest into /tests, you just need the cliend id and the secret key. If you need help, please contact me at my email.

And please, take a look at the patch which update the graph api to 2.10 in https://bugzilla.gnome.org/show_bug.cgi?id=772243 and we can ensure that your patch will work with the last Facebook API.

Thanks for your work!

 
> I'm going to review your patch, but in a first look I just see that you
> don't use the FACEBOOK_ENDPOINT string for the URL and it will be a must.

Yeah, I missed that. I will revamp the current version of the patch including Cosimo's review. So, no need to review this one. 

> Thanks for your work!

no problem :)

Created attachment 360978 [details] [review]
gfbgraph-photo: Add file uploading support

Review of attachment 360978 [details] [review]:

Looks great, but why you don't use the current Node API?

You can implement the required post params in gfbgraph_photo_get_connection_post_params, so we can use a gfbgraph_node_append_connection attaching a photo to a user or to an album or to whatever node we want. You can do it detecting if the "source" param is a local file, then just append the file content. About the multipart stuff, I think that we need some kind of property in the new node which allows as to create a soup_multipart_new instead a rest_call... some extra work here is necessary, I know.

In other way, it would be great to have a GTest instead an apart test from the current.

Review of attachment 360978 [details] [review]:

Looks great, but why you don't use the current Node API?

You can implement the required post params in gfbgraph_photo_get_connection_post_params, so we can use a gfbgraph_node_append_connection attaching a photo to a user or to an album or to whatever node we want. You can do it detecting if the "source" param is a local file, then just append the file content. About the multipart stuff, I think that we need some kind of property in the new node which allows as to create a soup_multipart_new instead a rest_call... some extra work here is necessary, I know.

In other way, it would be great to have a GTest instead an apart test from the current.

Review of attachment 360978 [details] [review]:

Looks great, but why you don't use the current Node API?

You can implement the required post params in gfbgraph_photo_get_connection_post_params, so we can use a gfbgraph_node_append_connection attaching a photo to a user or to an album or to whatever node we want. You can do it detecting if the "source" param is a local file, then just append the file content. About the multipart stuff, I think that we need some kind of property in the new node which allows as to create a soup_multipart_new instead a rest_call... some extra work here is necessary, I know.

In other way, it would be great to have a GTest instead an apart test from the current.

Created attachment 366751 [details] [review]
node: Add MUTLIPART_UPLOAD property

A boolean property to make sure the current node has a media file
to upload. Typically, used in conjunction with upload_file's
existing checks.

Created attachment 366752 [details] [review]
photo: Add gfbgraph_new_from_file_source API

API for client program to create GFBGraphPhoto node by passing
upload_file's URI and a optional caption for it.

Created attachment 366753 [details] [review]
photo: Include file source and caption in request parameters

Created attachment 366754 [details] [review]
common: Helper functions for file checks and libsoup's API calls

Created attachment 366755 [details] [review]
node: File upload support

Provide file uploading support with reliable file existing and
permissible mime_type checks. Facebook uploading media requires
media files to be sent as enctype multipart/form-data.
See gfbgraph_new_multipart_upload_soup_call for implementation
using libsoup.

Created attachment 366759 [details] [review]
[RFC]tests: Add test for photo upload

Not tested thoroughly due to access token / publish permissions issues
Probably, will get back to it later.

(In reply to Umang Jain from comment #16)
> Created attachment 366754 [details] [review] [review]
> common: Helper functions for file checks and libsoup's API calls

Relevant for video uploads (future reference) :
If you can tweak the url endpoint to "/me/video" and pass a video file as GFile, these functions, can upload a video file out of the box. I just tested it and seems to work.

Other work required:
* Better to go with gfbgraph-video.[ch] 
* [TODO] Use gfbgraph_connectable_get_connection_path for issuing relevant endpoints.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/libgfbgraph/-/issues/15.