GNOME Bugzilla – Bug 708359
Allow to modify REST function in serialize_params vfunc
Last modified: 2014-09-03 08:56:49 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.
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.
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.
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?
Christophe, could you update the patch? (to wake up activity and so that review flag can be placed)
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.
This new version of the patch only had some space-before-parens added.
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.
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
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.
Review of attachment 275129 [details] [review]: looks good to me (I'll let maintainer decide to ack)
could you write a test for this?
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.
Review of attachment 284426 [details] [review]: ack
Attachment 284426 [details] pushed as c66b6df - Allow to modify REST function in serialize_params vfunc
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.
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.
Review of attachment 285169 [details] [review]: ack
Review of attachment 285170 [details] [review]: ack
Attachment 285169 [details] pushed as fe1a864 - flickr: Fix function setting regression Attachment 285170 [details] pushed as 1d71e83 - lastfm: Fix function setting regression