After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 708359 - Allow to modify REST function in serialize_params vfunc
Allow to modify REST function in serialize_params vfunc
Status: RESOLVED FIXED
Product: librest
Classification: Platform
Component: proxy
0.7.x
Other Linux
: Normal normal
: ---
Assigned To: librest-maint
librest-maint
Depends on:
Blocks:
 
 
Reported: 2013-09-19 10:25 UTC by Christophe Fergeau
Modified: 2014-09-03 08:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
proposed patch (3.33 KB, patch)
2013-09-19 10:25 UTC, Christophe Fergeau
needs-work Details | Review
Allow to modify REST function in serialize_params vfunc (3.38 KB, patch)
2014-04-17 08:40 UTC, Christophe Fergeau
reviewed Details | Review
Allow to modify REST function in serialize_params vfunc (3.97 KB, patch)
2014-04-25 14:18 UTC, Christophe Fergeau
reviewed Details | Review
Allow to modify REST function in serialize_params vfunc (4.76 KB, patch)
2014-08-25 16:40 UTC, Christophe Fergeau
committed Details | Review
flickr: Fix function setting regression (2.72 KB, patch)
2014-09-02 19:35 UTC, Christophe Fergeau
committed Details | Review
lastfm: Fix function setting regression (1.74 KB, patch)
2014-09-02 19:35 UTC, Christophe Fergeau
committed Details | Review

Description Christophe Fergeau 2013-09-19 10:25:28 UTC
Created attachment 255287 [details] [review]
proposed patch

RestProxyCall::prepare_message() calls the serialize_params vfunc so that child classes can serialize the call parameters in whichever way they want.

One way of doing that could be to append the parameters to the URI that is called (http://example.com?param1=value1;param2=value2). However, the URI to call is determined at the beginning of prepare_message(), and is not refreshed after calling RestProxyCall::serialize_params(), so it's not possible to append parameters to the URI that is going to be called.
Comment 1 Christophe Fergeau 2013-11-06 09:06:27 UTC
Anyone up for a review? I need this in libgovirt 0.3.0 in order to be able to append some parameter values to the URI before sending the request.
Comment 2 Emmanuele Bassi (:ebassi) 2014-03-10 17:30:56 UTC
Review of attachment 255287 [details] [review]:

it looks generally okay to me

::: rest/rest-proxy-call.c
@@ +754,3 @@
   }
 
+  g_free(priv->url);

coding style: missing space between function and parentheses.

@@ +755,3 @@
 
+  g_free(priv->url);
+

I'd probably add a check if priv->url == bound_url, to avoid going through the whole validation in case the call didn't change the URL at all.

@@ +790,3 @@
+  }
+
+  GError *error = NULL;

coding style: missing space between function name and parenthesis.

@@ +822,3 @@
+     * rest_proxy_call_set_function()
+     */
+    if (!set_url(call))

coding style: missing space between function name and parenthesis.
Comment 3 Christophe Fergeau 2014-03-13 15:45:04 UTC
Review of attachment 255287 [details] [review]:

Fixed the missing spaces locally.

::: rest/rest-proxy-call.c
@@ +755,3 @@
 
+  g_free(priv->url);
+

if priv->function was/is set, I don't think we have an easy way of checking whether the URL is the same as before (unless we start tracking whether the RestProxy object is modified in between the 2 calls to set_url().
If priv->function is not set, since priv->url is set to a g_strdup'ed version of bound_url, we'd have to strcmp() it, so I'm not sure it's worth it.
Am I missing/misunderstanding something?
Comment 4 Marc-Andre Lureau 2014-04-17 08:31:15 UTC
Christophe, could you update the patch? (to wake up activity and so that review flag can be placed)
Comment 5 Christophe Fergeau 2014-04-17 08:40:18 UTC
Created attachment 274581 [details] [review]
Allow to modify REST function in serialize_params vfunc

RestProxyCall::prepare_message() calls the serialize_params vfunc
so that child classes can serialize the call parameters in whichever
way they want.
One way of doing that could be to append the parameters to the URI
that is called (http://example.com?param1=value1;param2=value2).
However, the URI to call is determined at the beginning of
prepare_message(), and is not refreshed after calling
RestProxyCall::serialize_params(), so it's not possible to
append parameters to the URI that is going to be called.

This commit rebuilds the URI to call after calling serialize_params()
in case it has changed.
Comment 6 Christophe Fergeau 2014-04-17 08:41:04 UTC
This new version of the patch only had some space-before-parens added.
Comment 7 Marc-Andre Lureau 2014-04-17 10:38:45 UTC
Review of attachment 274581 [details] [review]:

looks good

::: rest/rest-proxy-call.c
@@ +739,3 @@
 
+static gboolean
+set_url(RestProxyCall *call)

missing space

@@ +825,3 @@
+    {
+        return NULL;
+    }

instead of calling set_url() 2 times, why didn't you move the class->prepare() before? afaik priv->url is private, so it shouldn't matter if it's done once after the prepare call.
Comment 8 Christophe Fergeau 2014-04-18 08:10:59 UTC
Review of attachment 274581 [details] [review]:

::: rest/rest-proxy-call.c
@@ +825,3 @@
+    {
+        return NULL;
+    }

The set_url() call needs to be after serialize_params(), not after prepare(). My serialize_params() implementation appends the arguments at the end of the URL rather than putting them in the body of the call.
Moving set_url() after serialize_params is possible, but involves calling it from all 3 branches of the if (call_class->serialize_params) {} test
Comment 9 Christophe Fergeau 2014-04-25 14:18:46 UTC
Created attachment 275129 [details] [review]
Allow to modify REST function in serialize_params vfunc

This v3 fixes a few leaks when set_url returns an error. This does not set an error as set_url only fails on programmer's errors.
Comment 10 Marc-Andre Lureau 2014-04-25 15:26:58 UTC
Review of attachment 275129 [details] [review]:

looks good to me (I'll let maintainer decide to ack)
Comment 11 Marc-Andre Lureau 2014-08-25 14:43:22 UTC
could you write a test for this?
Comment 12 Christophe Fergeau 2014-08-25 16:40:00 UTC
Created attachment 284426 [details] [review]
Allow to modify REST function in serialize_params vfunc

RestProxyCall::prepare_message() calls the serialize_params vfunc
so that child classes can serialize the call parameters in whichever
way they want.
One way of doing that could be to append the parameters to the URI
that is called (http://example.com?param1=value1;param2=value2).
However, the URI to call is determined at the beginning of
prepare_message(), and is not refreshed after calling
RestProxyCall::serialize_params(), so it's not possible to
append parameters to the URI that is going to be called.

This commit rebuilds the URI to call after calling serialize_params()
in case it has changed.
Comment 13 Marc-Andre Lureau 2014-08-25 17:41:50 UTC
Review of attachment 284426 [details] [review]:

ack
Comment 14 Christophe Fergeau 2014-08-26 09:18:15 UTC
Attachment 284426 [details] pushed as c66b6df - Allow to modify REST function in serialize_params vfunc
Comment 15 Christophe Fergeau 2014-09-02 19:35:48 UTC
Created attachment 285169 [details] [review]
flickr: Fix function setting regression

Since commit c66b6d, RestProxyCall::url is regenerated after calling the
RestProxyCall::prepare virtual method. This breaks the flickr code as
it needs to reset RestProxyCall::url in order to remove the function
from it.
It used to do that by directly accessing RestProxyCall private
data. Since c66b6d, this can be solved using only public methods as
if the function is reset on the RestProxyCall in ::prepare, ::url will
be regenerated to reflect that after ::prepare and ::serialize_params
have been called.
Comment 16 Christophe Fergeau 2014-09-02 19:35:56 UTC
Created attachment 285170 [details] [review]
lastfm: Fix function setting regression

Since commit c66b6d, RestProxyCall::url is regenerated after calling the
RestProxyCall::prepare virtual method. This breaks the lastfm code as
it needs to reset RestProxyCall::url in order to remove the function
from it.

It used to do that by directly accessing RestProxyCall private
data. Since c66b6d, this can be solved using only public methods as
if the function is reset on the RestProxyCall in ::prepare, ::url will
be regenerated to reflect that after ::prepare and ::serialize_params
have been called.
Comment 17 Marc-Andre Lureau 2014-09-02 21:15:15 UTC
Review of attachment 285169 [details] [review]:

ack
Comment 18 Marc-Andre Lureau 2014-09-02 21:15:41 UTC
Review of attachment 285170 [details] [review]:

ack
Comment 19 Christophe Fergeau 2014-09-03 08:56:35 UTC
Attachment 285169 [details] pushed as fe1a864 - flickr: Fix function setting regression
Attachment 285170 [details] pushed as 1d71e83 - lastfm: Fix function setting regression