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 746495 - Add GVariant (de)serializer for XML-RPC
Add GVariant (de)serializer for XML-RPC
Status: RESOLVED FIXED
Product: libsoup
Classification: Core
Component: Misc
unspecified
Other Linux
: Normal normal
: ---
Assigned To: libsoup-maint@gnome.bugs
libsoup-maint@gnome.bugs
Depends on:
Blocks:
 
 
Reported: 2015-03-20 01:20 UTC by Xavier Claessens
Modified: 2015-08-13 15:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add GVariant (de)serializer for XML-RPC (39.76 KB, patch)
2015-03-20 01:21 UTC, Xavier Claessens
none Details | Review
xmlrpc: Add GVariant serializer (19.58 KB, patch)
2015-06-13 17:55 UTC, Xavier Claessens
none Details | Review
xmlrpc: Add GVariant serializer (19.58 KB, patch)
2015-06-13 17:56 UTC, Xavier Claessens
none Details | Review
xmlrpc: Add GVariant deserializer (26.19 KB, patch)
2015-06-13 17:56 UTC, Xavier Claessens
none Details | Review
xmlrpc: Test parsing fault response (1.30 KB, patch)
2015-06-13 17:56 UTC, Xavier Claessens
none Details | Review
xmlrpc: align debug lines (1.20 KB, patch)
2015-06-13 17:56 UTC, Xavier Claessens
none Details | Review
xmlrpc: Add special GVariant representing a datatime (4.27 KB, patch)
2015-06-13 17:56 UTC, Xavier Claessens
none Details | Review
xmlrpc: Fix documentation (4.42 KB, patch)
2015-06-13 17:56 UTC, Xavier Claessens
none Details | Review
xmlrpc: Add soup_xmlrpc_message_set_fault() (3.46 KB, patch)
2015-06-13 17:56 UTC, Xavier Claessens
none Details | Review
xmlrpc: Deprecate old API (9.32 KB, patch)
2015-06-13 17:56 UTC, Xavier Claessens
none Details | Review
fixup! xmlrpc: Add GVariant deserializer (1.30 KB, patch)
2015-06-15 12:46 UTC, Xavier Claessens
none Details | Review
xmlrpc: Add GVariant serializer (19.95 KB, patch)
2015-06-15 21:22 UTC, Xavier Claessens
none Details | Review
xmlrpc: Add GVariant deserializer (28.06 KB, patch)
2015-06-15 21:22 UTC, Xavier Claessens
none Details | Review
xmlrpc: Test parsing fault response (1.30 KB, patch)
2015-06-15 21:22 UTC, Xavier Claessens
none Details | Review
xmlrpc: Add special GVariant representing a datatime (3.61 KB, patch)
2015-06-15 21:22 UTC, Xavier Claessens
none Details | Review
xmlrpc: Fix documentation (1.62 KB, patch)
2015-06-15 21:23 UTC, Xavier Claessens
none Details | Review
xmlrpc: Add soup_xmlrpc_message_set_fault() (3.46 KB, patch)
2015-06-15 21:23 UTC, Xavier Claessens
none Details | Review
xmlrpc: Deprecate old API (9.32 KB, patch)
2015-06-15 21:23 UTC, Xavier Claessens
none Details | Review
xmlrpc: Add support for unstandard <i8> (4.36 KB, patch)
2015-06-15 21:23 UTC, Xavier Claessens
none Details | Review
xmlrpc: Add GVariant (de)serializer (48.36 KB, patch)
2015-06-22 20:47 UTC, Xavier Claessens
none Details | Review
xmlrpc: Test parsing fault response (1.30 KB, patch)
2015-06-22 20:47 UTC, Xavier Claessens
none Details | Review
xmlrpc: Add special GVariant representing a datatime (5.12 KB, patch)
2015-06-22 20:47 UTC, Xavier Claessens
none Details | Review
xmlrpc: Add soup_xmlrpc_message_set_fault() (3.39 KB, patch)
2015-06-22 20:47 UTC, Xavier Claessens
none Details | Review
xmlrpc: Add support for nonstandard <i8> (4.40 KB, patch)
2015-06-22 20:48 UTC, Xavier Claessens
none Details | Review
xmlrpc: Deprecate old API and fix doc (26.39 KB, patch)
2015-06-22 20:48 UTC, Xavier Claessens
none Details | Review
xmlrpc: s/gchar/char/ and s/gint/int/ (17.66 KB, patch)
2015-06-22 20:48 UTC, Xavier Claessens
none Details | Review
xmlrpc: rename files (177.65 KB, patch)
2015-06-22 20:48 UTC, Xavier Claessens
none Details | Review
Add SOUP_DEPRECATED_IN_2_52 macro (1.54 KB, patch)
2015-07-27 20:43 UTC, Xavier Claessens
none Details | Review
xmlrpc: Rename files (4.75 KB, patch)
2015-07-27 20:44 UTC, Xavier Claessens
none Details | Review
xmlrpc: Add GVariant (de)serializer (43.92 KB, patch)
2015-07-27 20:44 UTC, Xavier Claessens
none Details | Review
xmlrpc: Port soup_xmlrpc_build_fault() to GVariant (17.89 KB, patch)
2015-07-27 20:44 UTC, Xavier Claessens
none Details | Review
xmlrpc: Add unit tests for GVariant API (29.49 KB, patch)
2015-07-27 20:44 UTC, Xavier Claessens
none Details | Review
xmlrpc: Deprecate gvalue API (17.37 KB, patch)
2015-07-27 20:44 UTC, Xavier Claessens
none Details | Review
xmlrpc: Add unit tests for GVariant API (29.44 KB, patch)
2015-07-28 14:23 UTC, Xavier Claessens
none Details | Review
Add SOUP_DEPRECATED_IN_2_52 macro (1.54 KB, patch)
2015-08-13 15:35 UTC, Xavier Claessens
committed Details | Review
xmlrpc: Rename files (6.54 KB, patch)
2015-08-13 15:35 UTC, Xavier Claessens
committed Details | Review
xmlrpc: Add GVariant (de)serializer (86.81 KB, patch)
2015-08-13 15:36 UTC, Xavier Claessens
committed Details | Review
xmlrpc: Deprecate gvalue API (19.97 KB, patch)
2015-08-13 15:36 UTC, Xavier Claessens
committed Details | Review

Description Xavier Claessens 2015-03-20 01:20:52 UTC
The current xmlrpc API is horrible and uses deprecated GValueArray. GVariant provides a much better API for that task. Patch coming.

GVariant doesn't provide a date/time type, one possibility was to convert int64 to a datetime, but I decided to not do that because <i8> extension seems common (at least used in my use case) and some implementations (libsoup included) round time to the nearest second so there would be a precision lost to convert a int64. So until a special type is added in GVariant, sending a date will require using the old GValue API.

However, <dateTime.iso8601> can be received and will be converted to int64 GVariant.
Comment 1 Xavier Claessens 2015-03-20 01:21:19 UTC
Created attachment 299899 [details] [review]
Add GVariant (de)serializer for XML-RPC
Comment 2 Xavier Claessens 2015-03-20 01:24:26 UTC
The parse is largely inspired from json-glib's and I added their copyright/author references.
Comment 3 Dan Winship 2015-03-20 13:43:40 UTC
some quick notes:

  - there should be variant-based versions of the existing xmlrpc-test tests

  - I'll probably want to deprecate the GValue-based functions.

  - OTOH, I'm not sure I like having the GValue-based and GVariant-based
    versions have different functionality, in terms of extension types,
    and the fact that the GVariant-based version accepts all integer
    types, while the GValue-based version accepts only G_TYPE_INT, etc.
    I'm not sure which side I'd want to change though. Do you actually
    need that functionality? (ie, the ability to pass a 'u' and have it
    turned into an <int>?)

  - "int64 and uint64 are serialized using the <i8> standard extension."
    I wouldn't say "standard".

  - For <nil>, I think it would make more sense to serialize to/from "()"
    (the empty tuple).

  - We need some solution for <dateTime.iso8601>. Maybe use "ms" or "mx"?
    Though maybe we want to have a more general solution, in case we want
    to support addition extension types in the future. So something like
    "m(sv)"? But then, that would be annoying to construct:

            g_variant_new ("m(sv)", TRUE,
                           "dateTime.iso8601",
                           g_variant_new_int64 (time))

    I guess that's no *so* awful... and we could provide a helper
    function/macro.
Comment 4 Dan Winship 2015-03-20 14:05:33 UTC
oh, also, yay!
Comment 5 Xavier Claessens 2015-03-20 15:42:43 UTC
(In reply to Dan Winship from comment #3)
> some quick notes:
> 
>   - there should be variant-based versions of the existing xmlrpc-test tests

Right

>   - I'll probably want to deprecate the GValue-based functions.

Right, if we find a way to handle dates

>   - OTOH, I'm not sure I like having the GValue-based and GVariant-based
>     versions have different functionality, in terms of extension types,
>     and the fact that the GVariant-based version accepts all integer
>     types, while the GValue-based version accepts only G_TYPE_INT, etc.
>     I'm not sure which side I'd want to change though. Do you actually
>     need that functionality? (ie, the ability to pass a 'u' and have it
>     turned into an <int>?)

In the project I'm working atm we need to send timestamps with nano-sec precision (a full guint64). We finally just decided to avoid all the mess by sending them as <string> and printf/scanf them. So it doesn't matter much to me any more.

>   - "int64 and uint64 are serialized using the <i8> standard extension."
>     I wouldn't say "standard".

fair enough

>   - For <nil>, I think it would make more sense to serialize to/from "()"
>     (the empty tuple).

I added that because that's what the json-glib parser does, but since <nil> isn't really standard I could just drop support for gvariant "m" type. I don't need it myself.

>   - We need some solution for <dateTime.iso8601>. Maybe use "ms" or "mx"?
>     Though maybe we want to have a more general solution, in case we want
>     to support addition extension types in the future. So something like
>     "m(sv)"? But then, that would be annoying to construct:
> 
>             g_variant_new ("m(sv)", TRUE,
>                            "dateTime.iso8601",
>                            g_variant_new_int64 (time))
> 
>     I guess that's no *so* awful... and we could provide a helper
>     function/macro.

Why using a "m" there? Special cased "sx" and "st" where if the string is "dateTime.iso8601" and the next value is int64 or uint64 then serialize that as a date/time. And that's extensible if we want to support other "dateTime.*". It is really unlikely that the user actually wanted to send "dateTime.iso8601" as a string, right?
Comment 6 Dan Winship 2015-03-20 17:47:30 UTC
(In reply to Xavier Claessens from comment #5)
> >   - For <nil>, I think it would make more sense to serialize to/from "()"
> >     (the empty tuple).
> 
> I added that because that's what the json-glib parser does, but since <nil>
> isn't really standard I could just drop support for gvariant "m" type. I
> don't need it myself.

I'd say json-glib is wrong then :-). "null" in json is essentially its own type, not the absence of a value of some other known type.

> Why using a "m" there? Special cased "sx" and "st" where if the string is
> "dateTime.iso8601" and the next value is int64 or uint64 then serialize that
> as a date/time. And that's extensible if we want to support other
> "dateTime.*". It is really unlikely that the user actually wanted to send
> "dateTime.iso8601" as a string, right?

Yes, it's unlikely, but it's still pretty bad as an API.

You're right that we don't need the maybe though; tuples aren't used for anything else in the mapping, so we can just use a plain tuple instead and say that (s*) indicates a non-GVariant type. In the "dateTime.iso8601" case, we could even accept both (ss) and (sx) if we wanted.
Comment 7 Dan Winship 2015-03-20 18:00:55 UTC
well, actually I guess using maybe types for null makes sense if you're parsing the data to match a particular type signature (which is probably the usual case). It just annoys me that you have to randomly declare it an 'mv' in the case where you don't have a signature...
Comment 8 Dan Winship 2015-03-21 16:32:45 UTC
What is the point of SoupXMLRPCParams, rather than just having soup_xmlrpc_variant_parse_method_call() just take a signature and return a GVariant* itself? (Another holdover from json-glib's API?)

Having to have _variant_ in all the names is annoying if this is going to be the preferred API going forward. If we deprecate all/most of the old functions, then we can go with a different naming scheme for the new ones without worrying about it being inconsistent.

Maybe:

  soup_xmlrpc_variant_build_method_call -> soup_xmlrpc_build_request
  soup_xmlrpc_variant_parse_method_response -> soup_xmlrpc_parse_response
  soup_xmlrpc_variant_request_new -> soup_xmlrpc_request_message_new
                                  or soup_xmlrpc_message_new

  soup_xmlrpc_variant_parse_method_call -> soup_xmlrpc_parse_request
  soup_xmlrpc_variant_build_method_response -> soup_xmlrpc_build_response
  soup_xmlrpc_variant_set_response -> soup_xmlrpc_message_respond ?

If we go with that last one we might also want to add soup_xmlrpc_message_respond_fault() as an alias for soup_xmlrpc_message_set_fault().
Comment 9 Xavier Claessens 2015-03-21 19:42:35 UTC
(In reply to Dan Winship from comment #8)
> What is the point of SoupXMLRPCParams, rather than just having
> soup_xmlrpc_variant_parse_method_call() just take a signature and return a
> GVariant* itself? (Another holdover from json-glib's API?)

You usually need to first know which method is invoked to know that's the expected params signature.

> Having to have _variant_ in all the names is annoying if this is going to be
> the preferred API going forward. If we deprecate all/most of the old
> functions, then we can go with a different naming scheme for the new ones
> without worrying about it being inconsistent.
> 
> Maybe:
> 
>   soup_xmlrpc_variant_build_method_call -> soup_xmlrpc_build_request
>   soup_xmlrpc_variant_parse_method_response -> soup_xmlrpc_parse_response
>   soup_xmlrpc_variant_request_new -> soup_xmlrpc_request_message_new
>                                   or soup_xmlrpc_message_new
> 
>   soup_xmlrpc_variant_parse_method_call -> soup_xmlrpc_parse_request
>   soup_xmlrpc_variant_build_method_response -> soup_xmlrpc_build_response
>   soup_xmlrpc_variant_set_response -> soup_xmlrpc_message_respond ?
> 
> If we go with that last one we might also want to add
> soup_xmlrpc_message_respond_fault() as an alias for
> soup_xmlrpc_message_set_fault().

Looks better indeed, +1
Comment 10 Xavier Claessens 2015-06-13 17:55:10 UTC
Created attachment 305207 [details] [review]
xmlrpc: Add GVariant serializer
Comment 11 Xavier Claessens 2015-06-13 17:56:11 UTC
Created attachment 305208 [details] [review]
xmlrpc: Add GVariant serializer
Comment 12 Xavier Claessens 2015-06-13 17:56:18 UTC
Created attachment 305209 [details] [review]
xmlrpc: Add GVariant deserializer
Comment 13 Xavier Claessens 2015-06-13 17:56:22 UTC
Created attachment 305210 [details] [review]
xmlrpc: Test parsing fault response
Comment 14 Xavier Claessens 2015-06-13 17:56:27 UTC
Created attachment 305211 [details] [review]
xmlrpc: align debug lines
Comment 15 Xavier Claessens 2015-06-13 17:56:32 UTC
Created attachment 305212 [details] [review]
xmlrpc: Add special GVariant representing a datatime
Comment 16 Xavier Claessens 2015-06-13 17:56:36 UTC
Created attachment 305213 [details] [review]
xmlrpc: Fix documentation
Comment 17 Xavier Claessens 2015-06-13 17:56:41 UTC
Created attachment 305214 [details] [review]
xmlrpc: Add soup_xmlrpc_message_set_fault()

This is an alias for soup_xmlrpc_set_fault() for naming
consistency.
Comment 18 Xavier Claessens 2015-06-13 17:56:46 UTC
Created attachment 305215 [details] [review]
xmlrpc: Deprecate old API
Comment 19 Xavier Claessens 2015-06-15 12:46:58 UTC
Created attachment 305291 [details] [review]
fixup! xmlrpc: Add GVariant deserializer
Comment 20 Xavier Claessens 2015-06-15 21:22:44 UTC
Created attachment 305344 [details] [review]
xmlrpc: Add GVariant serializer
Comment 21 Xavier Claessens 2015-06-15 21:22:49 UTC
Created attachment 305345 [details] [review]
xmlrpc: Add GVariant deserializer
Comment 22 Xavier Claessens 2015-06-15 21:22:54 UTC
Created attachment 305346 [details] [review]
xmlrpc: Test parsing fault response
Comment 23 Xavier Claessens 2015-06-15 21:22:59 UTC
Created attachment 305347 [details] [review]
xmlrpc: Add special GVariant representing a datatime
Comment 24 Xavier Claessens 2015-06-15 21:23:03 UTC
Created attachment 305348 [details] [review]
xmlrpc: Fix documentation
Comment 25 Xavier Claessens 2015-06-15 21:23:08 UTC
Created attachment 305349 [details] [review]
xmlrpc: Add soup_xmlrpc_message_set_fault()

This is an alias for soup_xmlrpc_set_fault() for naming
consistency.
Comment 26 Xavier Claessens 2015-06-15 21:23:14 UTC
Created attachment 305350 [details] [review]
xmlrpc: Deprecate old API
Comment 27 Xavier Claessens 2015-06-15 21:23:20 UTC
Created attachment 305351 [details] [review]
xmlrpc: Add support for unstandard <i8>

uint32 can overflow a int32 so it is serialized as <i8>.
uint64 can overflox a int64 so it still can't be serialized.

For maximum flexibility on the parser side we accept all types
as long as the value is in range.
Comment 28 Dan Winship 2015-06-20 19:56:48 UTC
Comment on attachment 305344 [details] [review]
xmlrpc: Add GVariant serializer

> xmlrpc: Add GVariant serializer

I think adding the serializing and deserializing parts separately is confusing. Just squash the first two commits together.

>-	soup-xmlrpc.c
>+	soup-xmlrpc.c			\
>+	soup-xmlrpc-variant.c

actually, we should make the new API be soup-xmlrpc.c, and move the old one to soup-xmlrpc-gvalue.c. (Maybe do that as part of the deprecation commit?)

>+ * Copyright (C) 2007 Red Hat, Inc.
>+ * Copyright (C) 2007 OpenedHand Ltd.
>+ * Copyright (C) 2015 Collabora ltd.

remove the "(C)"s. (Yeah, I know soup-xmlrpc.c has them, but it's wrong, and we shouldn't copy it into new files.) (Likewise in the .h file.)

>+#define ERROR(fmt, ...)								\
>+G_STMT_START{									\
>+	g_set_error (error, SOUP_XMLRPC_ERROR, SOUP_XMLRPC_ERROR_ARGUMENTS,	\
>+		     fmt, ##__VA_ARGS__);					\

__VA_ARGS__ is C99, which would break the compile on Windows.

The simplest solution is to just drop the macro and search+replace all the existing uses of it...

>+	if (g_variant_classify (mname) != G_VARIANT_CLASS_STRING)
>+		ERROR ("Only string keys are supported in dictionaries, got %s",
>+		       g_variant_get_type_string (mname));

whether the macro stays or goes, please put {} around if bodies that are multiple lines (even if they're only a single statement).

Also, you should _() the error messages. (soup-xmlrpc doesn't because at the time that API was added libsoup didn't have any translated strings. But new GError-using APIs should be properly localized.)

>+	case G_VARIANT_CLASS_HANDLE:

handles can't be serialized to XML-RPC

>+	case G_VARIANT_CLASS_OBJECT_PATH:
>+	case G_VARIANT_CLASS_SIGNATURE:

and I don't think it makes sense to serialize these either, since XML-RPC isn't going to treat them as any different from strings

>+		type = g_variant_get_type_string (value);
>+		if (type[1] == G_VARIANT_CLASS_BYTE) {

I'd say "if (g_variant_is_of_type (value, G_VARIANT_TYPE_BYTESTRING))"

>+	case G_VARIANT_CLASS_DICT_ENTRY: {

I'm pretty sure it should not be possible to reach this case, since it would imply a dict entry that was not inside a dict type, which GVariant wouldn't allow.

>+	case G_VARIANT_CLASS_UINT32:
>+	case G_VARIANT_CLASS_INT64:
>+	case G_VARIANT_CLASS_UINT64:
>+	default:
>+		ERROR ("Unsupported type: %s", g_variant_get_type_string (value));

Given that we accept INT8s and UINT16s and such as <int>s, it seems like we should accept UINT32s, INT64s, and UINT64s as well, and only return an error if the actual value is out of range for an <int>. ?

>+ * Limitations that will cause method to return %FALSE and set @error:

"Limitations" makes it sound like a problem with our implementation. Say something like "Trying to serialize data that does not correspond to any XML-RPC type will cause the method to return %FALSE and set @error" or something like that. Or else, expand the "special cases" section and just specify the complete GVariant->XML-RPC mapping, and state that anything other GVariants will result in an error. (Eg, given that you say that "tuples are serialized as <array>", it's not totally clear that arrays are also serialized as <array>.)

>+ * Return value: (nullable): the text of the methodCall, or %NULL on
>+ *  error.

(nullable) means it can return %NULL even if there *isn't* an error, which is not the case here. (Likewise for soup_xmlrpc_build_response())

>+gchar *
>+soup_xmlrpc_build_request (const gchar *method_name,

oh, I should have noticed this earlier, but please don't use gint and gchar. (All other "g" types are fine, but gint and gchar are pointless IMHO and make the code bigger rather than smaller...)

>+	xmlrpc-variant-test

This could possibly go in the existing xmlrpc-test. Either way, you should port the existing xmlrpc-test tests as well.
Comment 29 Dan Winship 2015-06-20 20:39:53 UTC
Comment on attachment 305345 [details] [review]
xmlrpc: Add GVariant deserializer

I'm not quite sure about the set of functions provided here...

I get the point of SoupXMLRPCParams now, but I wonder if it would be better to have something like "soup_xmlrpc_parse_method_name()" instead, so instead of doing

  method = soup_xmlrpc_parse_request (data, len, &params, &error);
  if (!strcmp (method, "doFoobar")) {
      args = soup_xmlrpc_params_parse (params, signature, &error);
      ...
  }
  soup_xmlrpc_params_free (params);
  g_free (method);

you do

  method = soup_xmlrpc_parse_method_name (data, len, &error);
  if (!strcmp (method, "doFoobar")) {
      args = soup_xmlrpc_parse_request (data, len, signature, &error);
      ...
  }
  g_free (method);

which saves a variable and a line of code, though at the expense of making us parse at least the beginning of the request twice.

Although my assumption would be that you don't *really* want a GVariant anyway, regardless of its signature. You just want the XML-RPC parameters parsed into local variables in a certain way... So we could just have parse_request() return a GVariant with some default parsing, and let the caller use g_variant_get() to extract values from it. Maybe add soup_xmlrpc_transform_variant() if you want a way to typecheck the variant and cast it to match a specific signature.

So, eg, if you have a call to "foobar" with args "<string>hello</string>", "<int>5</int>", and "<array><string>red</string><string>yellow</string><string>blue</string></array>" (simplifying the XML-RPC there a bit), the recipient does:

  if (soup_xmlrpc_parse_request (request, length, &method_name, &raw, &error)) {

and now @method_name is "foobar", and @params is a GVariant of type (si(sss)) (since arrays in XML-RPC are in general more like GVariant tuples than GVariant arrays). But then you could do

      params = soup_xmlrpc_transform_params (raw, "(syas)", &error);
      if (params) {
          const char *name;
          guchar code;
          char **labels;

          g_variant_get (params, "(&sy^as)", &name, &code, &labels);
          ...
      }

But I guess it would be good to see how the code is actually being used to see what would really make the most sense. (I realize now that soup_xmlrpc_extract_method_call() is actually pretty useless, since it requires you to know the call signature before you know the method name...)
Comment 30 Dan Winship 2015-06-20 21:06:50 UTC
Comment on attachment 305347 [details] [review]
xmlrpc: Add special GVariant representing a datatime

>Subject: [PATCH] xmlrpc: Add special GVariant representing a datatime

typo "datetime"

So I'd suggested before that "tuples aren't used for anything else in the mapping, so we can just use a plain tuple instead and say that (s*) indicates a non-GVariant type", but of course that's not actually true; tuples are sometimes used to represent <array>s.

And I don't like using "magic" values for this. I'd rather make the representation of a dateTime.iso8601 be something that is not otherwise a valid representation of an XML-RPC type. I think that means we have to use one of SIGNATURE, OBJECT_PATH, HANDLE, MAYBE, UINT64, or non-string-indexed DICTs. UINT64 I'd suggested we should translate to <int>. SIGNATURE and HANDLE are too limited in what values they can hold. MAYBE is bad if we potentially want to include <nil> in the future (or if we want to also have a parallel GVariant-based JSON API, which I do).

OBJECT_PATH seems like a definite possibility though: we could have extension values be represented as "(oss)", where the 'o' is "/org/gnome/libsoup/ExtensionType", the 's' is the type name ("dateTime.iso8601"), and the 's' is the value. (This would only be slightly different from the code you already wrote then.)

>+/**
>+ * soup_xmlrpc_new_datetime:
>+ * @t: a unix timestamp
>+ *
>+ * Construct a specia #GVariant used to serialize a &lt;dateTime.iso8601&gt;

typo "specia"

If we're doing the above, this should just be a wrapper around a generic "soup_xmlrpc_new_extension_type()" or something like that.

>+soup_xmlrpc_new_datetime (gint64 t)

we use time_t for times elsewhere

>+	str = g_time_val_to_iso8601 (&tv);

You should use soup_date_new_from_time_t() and soup_date_to_string() with format SOUP_DATE_ISO8601_XMLRPC; some implementations may be broken and not accept arbitrary ISO-8601 strings.

>+/* Utils */
>+GVariant *soup_xmlrpc_new_datetime (gint64 t);

Oh, belated, but everything needs "Since" tags in the gtk-doc comments, and SOUP_AVAILABLE_IN_ declarations in the .h file.
Comment 31 Dan Winship 2015-06-20 21:10:12 UTC
Comment on attachment 305350 [details] [review]
xmlrpc: Deprecate old API

Also deprecate all of soup-value-utils, which is only used by soup-xmlrpc


>+/* This whole file is pretty much deprecated and replaced by
>+ * soup-xmlrpc-variant.c */
>+#ifdef G_GNUC_BEGIN_IGNORE_DEPRECATIONS
>+G_GNUC_BEGIN_IGNORE_DEPRECATIONS
>+#endif

You don't need to ifdef it. It's always defined, it's just that sometimes it's defined to nothing.

Oh, I see, you copied the existing code. I think that was to preserve compat with versions of GLib from before G_GNUC_BEGIN_IGNORE_DEPRECATIONS was introduced, but we presumably depend on a new-enough version of GLib that it's always there now.
Comment 32 Dan Winship 2015-06-20 21:14:03 UTC
Comment on attachment 305351 [details] [review]
xmlrpc: Add support for unstandard <i8>

>Subject: [PATCH] xmlrpc: Add support for unstandard <i8>

"nonstandard"

>uint32 can overflow a int32 so it is serialized as <i8>.
>uint64 can overflox a int64 so it still can't be serialized.

typo "overflox"

>For maximum flexibility on the parser side we accept all types
>as long as the value is in range.

Hm... I guess if we're supporting <i8>, then probably what I said earlier about accepting within-range uint64s, etc, as <int> is wrong; a particular receiver might require either an <int> or an <i8> for a given method, and not accept the other, so the caller needs to be able to unambiguously indicate which one they meant...

(But that then leads me back to thinking that we shouldn't accept 8- and 16-bit types as <int> either.)
Comment 33 Xavier Claessens 2015-06-22 18:15:21 UTC
Thanks for the review !

(In reply to Dan Winship from comment #28)
> I think adding the serializing and deserializing parts separately is
> confusing. Just squash the first two commits together.

OK, will do.

> >-	soup-xmlrpc.c
> >+	soup-xmlrpc.c			\
> >+	soup-xmlrpc-variant.c
> 
> actually, we should make the new API be soup-xmlrpc.c, and move the old one
> to soup-xmlrpc-gvalue.c. (Maybe do that as part of the deprecation commit?)

libsoup doesn't seems to enforce single-include, isn't it an API break to rename the .h file? If we support only direct include of libsoup.h then it's fine.

> >+ * Copyright (C) 2007 Red Hat, Inc.
> >+ * Copyright (C) 2007 OpenedHand Ltd.
> >+ * Copyright (C) 2015 Collabora ltd.
> 
> remove the "(C)"s. (Yeah, I know soup-xmlrpc.c has them, but it's wrong, and
> we shouldn't copy it into new files.) (Likewise in the .h file.)

OK, will do.

> >+#define ERROR(fmt, ...)								\
> >+G_STMT_START{									\
> >+	g_set_error (error, SOUP_XMLRPC_ERROR, SOUP_XMLRPC_ERROR_ARGUMENTS,	\
> >+		     fmt, ##__VA_ARGS__);					\
> 
> __VA_ARGS__ is C99, which would break the compile on Windows.

Sad to be limited on a 16 years old feature, didn't know that was C99 and that libsoup builds with msvc. Will expand it everywhere then...

> Also, you should _() the error messages. (soup-xmlrpc doesn't because at the
> time that API was added libsoup didn't have any translated strings. But new
> GError-using APIs should be properly localized.)

Really? I always *HATED* translated GError, it generally doesn't make any sense to end user (app really should have their own msg for UI) and you end up having useless Chinese in debug logs that people post in bugzilla. But if you like wasting translator's time, I'll do.

> >+	case G_VARIANT_CLASS_HANDLE:
> 
> handles can't be serialized to XML-RPC

Hm, right, better refuse to send a useless file descriptor. For what it's worth, json-glib accepts it. Will fix.

> >+	case G_VARIANT_CLASS_OBJECT_PATH:
> >+	case G_VARIANT_CLASS_SIGNATURE:
> 
> and I don't think it makes sense to serialize these either, since XML-RPC
> isn't going to treat them as any different from strings

Not sure, I wanted to accept sending as much GVariants as possible, and json-glib accepts it as well. It doesn't make much sense to want to send them indeed, but doesn't hurt neither. Note that on the parser side, telling it's a signature/object-path make it check the format and raise error if they are not valid, so it has a small utility there. I don't have strong opinion, so if you prefer to drop it I'll fix.

> >+		type = g_variant_get_type_string (value);
> >+		if (type[1] == G_VARIANT_CLASS_BYTE) {
> 
> I'd say "if (g_variant_is_of_type (value, G_VARIANT_TYPE_BYTESTRING))"

Better indeed, will fix.

> >+	case G_VARIANT_CLASS_DICT_ENTRY: {
> 
> I'm pretty sure it should not be possible to reach this case, since it would
> imply a dict entry that was not inside a dict type, which GVariant wouldn't
> allow.

It is possible, even if pretty useless. A dict entry "{?*}" can be outside an array "a{?*}". For example: g_variant_new("{su}", "key", 42); In that case it's pretty much the same as a tuple tbh. 

> >+	case G_VARIANT_CLASS_UINT32:
> >+	case G_VARIANT_CLASS_INT64:
> >+	case G_VARIANT_CLASS_UINT64:
> >+	default:
> >+		ERROR ("Unsupported type: %s", g_variant_get_type_string (value));
> 
> Given that we accept INT8s and UINT16s and such as <int>s, it seems like we
> should accept UINT32s, INT64s, and UINT64s as well, and only return an error
> if the actual value is out of range for an <int>. ?

I decided to reject types that can overflow. My rational is that when you test your system and want to send uint32 you most likely have small values that will just work. Then when your system is on production, on the long run, it gets bigger values and would suddenly start complaining for out of range values. That's also why in later patch I opted for <i8> in the uint32 case, so you're sure the value will always fit, and if the peer doesn't support <i8> extension you'll get an error while testing and you'll notice you shouldn't be using uint32. Same rational for rejecting uint64 even when using <i8> extension.

> >+ * Limitations that will cause method to return %FALSE and set @error:
> 
> "Limitations" makes it sound like a problem with our implementation. Say
> something like "Trying to serialize data that does not correspond to any
> XML-RPC type will cause the method to return %FALSE and set @error" or
> something like that. Or else, expand the "special cases" section and just
> specify the complete GVariant->XML-RPC mapping, and state that anything
> other GVariants will result in an error. (Eg, given that you say that
> "tuples are serialized as <array>", it's not totally clear that arrays are
> also serialized as <array>.)

ok, will fix.

> >+ * Return value: (nullable): the text of the methodCall, or %NULL on
> >+ *  error.
> 
> (nullable) means it can return %NULL even if there *isn't* an error, which
> is not the case here. (Likewise for soup_xmlrpc_build_response())

ok, didn't pay much attention to annotations tbh, that's just copy/pasted from somewhere else. Will fix.

> >+gchar *
> >+soup_xmlrpc_build_request (const gchar *method_name,
> 
> oh, I should have noticed this earlier, but please don't use gint and gchar.
> (All other "g" types are fine, but gint and gchar are pointless IMHO and
> make the code bigger rather than smaller...)

As you wish, I'll sed that. When my brain thinks "char" my fingers type "gchar" automatically.

> >+	xmlrpc-variant-test
> 
> This could possibly go in the existing xmlrpc-test. Either way, you should
> port the existing xmlrpc-test tests as well.

I didn't want to port existing tests because even deprecated APIs must still work and thus be tested. I can merge files though if you prefer.

(In reply to Dan Winship from comment #29)
> I get the point of SoupXMLRPCParams now, but I wonder if it would be better
> to have something like "soup_xmlrpc_parse_method_name()" instead, so instead
> of doing
> 
> which saves a variable and a line of code, though at the expense of making
> us parse at least the beginning of the request twice.

I did that way to avoid parsing the xml twice indeed. I don't think it's a big deal to have an extra local variable. Note that there is soup_xmlrpc_parse_request_full() if you know the signature in advance (e.g. all your methods have same signature).

> Although my assumption would be that you don't *really* want a GVariant
> anyway, regardless of its signature. You just want the XML-RPC parameters
> parsed into local variables in a certain way... So we could just have
> parse_request() return a GVariant with some default parsing, and let the
> caller use g_variant_get() to extract values from it. Maybe add
> soup_xmlrpc_transform_variant() if you want a way to typecheck the variant
> and cast it to match a specific signature.
> 
> So, eg, if you have a call to "foobar" with args "<string>hello</string>",
> "<int>5</int>", and
> "<array><string>red</string><string>yellow</string><string>blue</string></
> array>" (simplifying the XML-RPC there a bit), the recipient does:
> 
>   if (soup_xmlrpc_parse_request (request, length, &method_name, &raw,
> &error)) {
> 
> and now @method_name is "foobar", and @params is a GVariant of type
> (si(sss)) (since arrays in XML-RPC are in general more like GVariant tuples
> than GVariant arrays). But then you could do
> 
>       params = soup_xmlrpc_transform_params (raw, "(syas)", &error);
>       if (params) {
>           const char *name;
>           guchar code;
>           char **labels;
> 
>           g_variant_get (params, "(&sy^as)", &name, &code, &labels);
>           ...
>       }
> 
> But I guess it would be good to see how the code is actually being used to
> see what would really make the most sense. (I realize now that
> soup_xmlrpc_extract_method_call() is actually pretty useless, since it
> requires you to know the call signature before you know the method name...)

Hmm, that seems quite complicated tbh, and extra GVariant conversion isn't always cheap. Note that you can always pass NULL for the signature and you'll get default types. If you give a signature that's because you know what to expect and want to typecheck and reject any call that doesn't comply.

(In reply to Dan Winship from comment #30)
> >Subject: [PATCH] xmlrpc: Add special GVariant representing a datatime
> 
> typo "datetime"

Will fix.

> OBJECT_PATH seems like a definite possibility though: we could have
> extension values be represented as "(oss)", where the 'o' is
> "/org/gnome/libsoup/ExtensionType", the 's' is the type name
> ("dateTime.iso8601"), and the 's' is the value. (This would only be slightly
> different from the code you already wrote then.)

Makes sense indeed, and it can easily be extended for any other types if needed. Also even if we decide to support object-path we are still technically using a path in libsoup's domain so it shouldn't ever be used by someone else. Will do that.

> >+/**
> >+ * soup_xmlrpc_new_datetime:
> >+ * @t: a unix timestamp
> >+ *
> >+ * Construct a specia #GVariant used to serialize a &lt;dateTime.iso8601&gt;
> 
> typo "specia"

Will fix.

> If we're doing the above, this should just be a wrapper around a generic
> "soup_xmlrpc_new_extension_type()" or something like that.

Having such API also allows custom user extensions, could potentially help interop with non-standard servers.

> >+soup_xmlrpc_new_datetime (gint64 t)
> 
> we use time_t for times elsewhere

ok, will fix.

> >+	str = g_time_val_to_iso8601 (&tv);
> 
> You should use soup_date_new_from_time_t() and soup_date_to_string() with
> format SOUP_DATE_ISO8601_XMLRPC; some implementations may be broken and not
> accept arbitrary ISO-8601 strings.

ok, will fix.

> >+/* Utils */
> >+GVariant *soup_xmlrpc_new_datetime (gint64 t);
> 
> Oh, belated, but everything needs "Since" tags in the gtk-doc comments, and
> SOUP_AVAILABLE_IN_ declarations in the .h file.

ok, will fix.
Comment 34 Dan Winship 2015-06-22 19:52:53 UTC
(In reply to Xavier Claessens from comment #33)
> libsoup doesn't seems to enforce single-include, isn't it an API break to
> rename the .h file? If we support only direct include of libsoup.h then it's
> fine.

oh, you're right. I forgot that I never got around to that.

Though we could still rename soup-xmlrpc.h -> soup-xmlrpc-gvalue.h and have the new soup-xmlrpc.h #include <libsoup/soup-xmlrpc-gvalue.h>

> > >+	case G_VARIANT_CLASS_DICT_ENTRY: {
> > 
> > I'm pretty sure it should not be possible to reach this case, since it would
> > imply a dict entry that was not inside a dict type, which GVariant wouldn't
> > allow.
> 
> It is possible, even if pretty useless. A dict entry "{?*}" can be outside
> an array "a{?*}". For example: g_variant_new("{su}", "key", 42); In that
> case it's pretty much the same as a tuple tbh.

Interesting. That's not allowed in D-Bus so I assumed it wasn't allowed in GVariant either. (But this is actually documented in the GVariant docs.)

> > >+	xmlrpc-variant-test
> > 
> > This could possibly go in the existing xmlrpc-test. Either way, you should
> > port the existing xmlrpc-test tests as well.
> 
> I didn't want to port existing tests because even deprecated APIs must still
> work and thus be tested. I can merge files though if you prefer.

Right, I didn't mean to get rid of the existing tests, I meant to duplicate them, so we test each of the XML-RPC calls using both the old API and the new.

> > But I guess it would be good to see how the code is actually being used to
> > see what would really make the most sense. (I realize now that
> > soup_xmlrpc_extract_method_call() is actually pretty useless, since it
> > requires you to know the call signature before you know the method name...)
> 
> Hmm, that seems quite complicated tbh...

Is your code that uses this somewhere public?
Comment 35 Xavier Claessens 2015-06-22 20:09:47 UTC
(In reply to Dan Winship from comment #31)
> Also deprecate all of soup-value-utils, which is only used by soup-xmlrpc
ok, will fix.

> >+/* This whole file is pretty much deprecated and replaced by
> >+ * soup-xmlrpc-variant.c */
> >+#ifdef G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> >+G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> >+#endif
> 
> You don't need to ifdef it. It's always defined, it's just that sometimes
> it's defined to nothing.
> 
> Oh, I see, you copied the existing code. I think that was to preserve compat
> with versions of GLib from before G_GNUC_BEGIN_IGNORE_DEPRECATIONS was
> introduced, but we presumably depend on a new-enough version of GLib that
> it's always there now.

ok, will fix.

(In reply to Dan Winship from comment #32)
> >Subject: [PATCH] xmlrpc: Add support for unstandard <i8>
> 
> "nonstandard"
>
> >uint32 can overflow a int32 so it is serialized as <i8>.
> >uint64 can overflox a int64 so it still can't be serialized.
> 
> typo "overflox"

ok, will fix.
 
> >For maximum flexibility on the parser side we accept all types
> >as long as the value is in range.
> 
> Hm... I guess if we're supporting <i8>, then probably what I said earlier
> about accepting within-range uint64s, etc, as <int> is wrong; a particular
> receiver might require either an <int> or an <i8> for a given method, and
> not accept the other, so the caller needs to be able to unambiguously
> indicate which one they meant...
> 
> (But that then leads me back to thinking that we shouldn't accept 8- and
> 16-bit types as <int> either.)

My rational here is if we transmit for example a TCP port number, it is uint16. It would be serialized into a <int> and the receiver can tell it expects a uint16 so a value bigger than G_MAXUINT16 would raise an error. The point is to have all checks while parsing arguments so the method can trust the GVariant it receives and don't have to do extra checks.
Comment 36 Xavier Claessens 2015-06-22 20:47:22 UTC
Created attachment 305862 [details] [review]
xmlrpc: Add GVariant (de)serializer
Comment 37 Xavier Claessens 2015-06-22 20:47:29 UTC
Created attachment 305863 [details] [review]
xmlrpc: Test parsing fault response
Comment 38 Xavier Claessens 2015-06-22 20:47:35 UTC
Created attachment 305864 [details] [review]
xmlrpc: Add special GVariant representing a datatime
Comment 39 Xavier Claessens 2015-06-22 20:47:47 UTC
Created attachment 305865 [details] [review]
xmlrpc: Add soup_xmlrpc_message_set_fault()

This is an alias for soup_xmlrpc_set_fault() for naming
consistency.
Comment 40 Xavier Claessens 2015-06-22 20:48:03 UTC
Created attachment 305866 [details] [review]
xmlrpc: Add support for nonstandard <i8>

uint32 can overflow a int32 so it is serialized as <i8>.
uint64 can overflow a int64 so it still can't be serialized.

For maximum flexibility on the parser side we accept all types
as long as the value is in range.
Comment 41 Xavier Claessens 2015-06-22 20:48:20 UTC
Created attachment 305867 [details] [review]
xmlrpc: Deprecate old API and fix doc
Comment 42 Xavier Claessens 2015-06-22 20:48:27 UTC
Created attachment 305868 [details] [review]
xmlrpc: s/gchar/char/ and s/gint/int/
Comment 43 Xavier Claessens 2015-06-22 20:48:34 UTC
Created attachment 305869 [details] [review]
xmlrpc: rename files
Comment 44 Xavier Claessens 2015-06-22 21:11:50 UTC
(In reply to Dan Winship from comment #34)
> Though we could still rename soup-xmlrpc.h -> soup-xmlrpc-gvalue.h and have
> the new soup-xmlrpc.h #include <libsoup/soup-xmlrpc-gvalue.h>

done.

> Right, I didn't mean to get rid of the existing tests, I meant to duplicate
> them, so we test each of the XML-RPC calls using both the old API and the
> new.

ok, will do

> > > But I guess it would be good to see how the code is actually being used to
> > > see what would really make the most sense. (I realize now that
> > > soup_xmlrpc_extract_method_call() is actually pretty useless, since it
> > > requires you to know the call signature before you know the method name...)
> > 
> > Hmm, that seems quite complicated tbh...
> 
> Is your code that uses this somewhere public?

Sadly it's proprietary code. But it's basically that pattern:

method_name = soup_xmlrpc_parse_request (data, len, &params, &error);
if (method_name == NULL)
  print_error_and_return;

if (g_str_equal (method_name, "Foo")) {
  const gchar *ip;
  guint16 port;

  args = soup_xmlrpc_params_parse (params, "(sq)", &error);
  if (args == NULL)
    print_error_and_return;
  g_variant_get (args, "(&sq)", &ip, port);

  do_foo (self, ip, port);
} else if (g_str_equal (method_name, "Bar")) {
  args = soup_xmlrpc_params_parse (params, "()", &error);
  if (args == NULL)
    print_error_and_return;

  do_bar (self);
} else {
  not_supported();
}

One possible nice improvement would be the merge the parse method with g_variant_get(). Like this:
 soup_xmlrpc_params_parse(params, "(&sq)", &ip, &port, &error);
Comment 45 Xavier Claessens 2015-06-23 14:48:54 UTC
Pushed my branch there for now: https://git.gnome.org/browse/libsoup/log/?h=wip/xclaesse/xmlrpc

I think it is only missing duplicating existing tests, and a decision on points I've argued above.
Comment 46 Xavier Claessens 2015-06-23 14:53:03 UTC
Ah, note that it's still using g_time_val_from_iso8601() instead of soup_date_new_from_string() because it doesn't seems to parse timezone and make my test fail. I changed to use soup_date_to_string() though.
Comment 47 Xavier Claessens 2015-06-23 20:22:29 UTC
(In reply to Xavier Claessens from comment #45)
> I think it is only missing duplicating existing tests, and a decision on
> points I've argued above.

Tests are now ported.

(In reply to Xavier Claessens from comment #46)
> Ah, note that it's still using g_time_val_from_iso8601() instead of
> soup_date_new_from_string() because it doesn't seems to parse timezone and
> make my test fail. I changed to use soup_date_to_string() though.

Changed that, because soup_date_new_from_string() assumes UTC if no timezone is defined, and g_time_val_from_iso8601() assumes local time, so that makes a difference. Better stay compatible with what libsoup was doing previously.
Comment 48 Xavier Claessens 2015-06-23 20:28:58 UTC
(In reply to Xavier Claessens from comment #44)
> One possible nice improvement would be the merge the parse method with
> g_variant_get(). Like this:
>  soup_xmlrpc_params_parse(params, "(&sq)", &ip, &port, &error);

Could add those later (I would like to not block the current patchset on it):
soup_xmlrpc_extract_response
soup_xmlrpc_params_extract
soup_xmlrpc_extract_request
Comment 49 Xavier Claessens 2015-07-27 20:43:59 UTC
Created attachment 308242 [details] [review]
Add SOUP_DEPRECATED_IN_2_52 macro
Comment 50 Xavier Claessens 2015-07-27 20:44:07 UTC
Created attachment 308243 [details] [review]
xmlrpc: Rename files
Comment 51 Xavier Claessens 2015-07-27 20:44:12 UTC
Created attachment 308244 [details] [review]
xmlrpc: Add GVariant (de)serializer
Comment 52 Xavier Claessens 2015-07-27 20:44:16 UTC
Created attachment 308245 [details] [review]
xmlrpc: Port soup_xmlrpc_build_fault() to GVariant
Comment 53 Xavier Claessens 2015-07-27 20:44:21 UTC
Created attachment 308246 [details] [review]
xmlrpc: Add unit tests for GVariant API
Comment 54 Xavier Claessens 2015-07-27 20:44:26 UTC
Created attachment 308247 [details] [review]
xmlrpc: Deprecate gvalue API
Comment 55 Xavier Claessens 2015-07-28 14:23:20 UTC
Created attachment 308299 [details] [review]
xmlrpc: Add unit tests for GVariant API
Comment 56 Dan Winship 2015-08-06 13:06:41 UTC
pushed some fixups to wip/xmlrpc-variant in git

Remaining questions/issues:

  - We build dateTimes from '(oss)' but parse them to 'x'

  - We build arbitrary extension types from '(oss)', but don't parse them
    at all

    Probably we should say that dateTime and extension values will be
    deserialized to 'v', and have reverse equivalents of
    soup_xmlrpc_new_datetime() and soup_xmlrpc_new_custom() to extract
    the values from those GVariants.

  - Related to that, I don't think we should accept random 'o' or 'g'
    values either when building or when parsing, since (a) there's no good
    reason to, and (b) '(oss)' for extensions is only unambiguous if
    'o' isn't allowed otherwise.

  - soup_xmlrpc_params_parse() should set an error if signature has
    unused characters at the end?

  - I don't think soup_xmlrpc_parse_request_full() is useful, since it
    requires you to know the method's signature without knowing its name.

  - soup_xmlrpc_new_custom() says "See soup_xmlrpc_build_request()", but
    the soup_xmlrpc_build_request() docs don't talk about custom values.

  - soup_xmlrpc_new_custom() and soup_xmlrpc_new_datetime() should have
    "value" or "variant" in their name somewhere
Comment 57 Xavier Claessens 2015-08-10 18:16:34 UTC
Regarding your fixup:

 - You added in soup_xmlrpc_build_response()'s doc that it shouldn't be a tuple, but tuples are serialized as arrays so it's actually fine. In DBus the return value can be a tuple, it's considered a single value. Just mentioning, I don't care much personally.

 - Why do you remove all old gvalue APIs from doc? Deprecated stuff should be still documented IMO.


(In reply to Dan Winship from comment #56)
>   - We build dateTimes from '(oss)' but parse them to 'x'

Is that a problem? The format in which we build dateTime GVariant is internal implementation detail, not a public API guarantee.

>   - We build arbitrary extension types from '(oss)', but don't parse them
>     at all
> 
>     Probably we should say that dateTime and extension values will be
>     deserialized to 'v', and have reverse equivalents of
>     soup_xmlrpc_new_datetime() and soup_xmlrpc_new_custom() to extract
>     the values from those GVariants.

tbh, maybe I should just remove soup_xmlrpc_new_custom() from public API, because that's something that was not possible with previous API anyway. If someone needs it later for compat with some services we can still add it. I would keep only the "(oss)" trick for dateTime as internal implementation detail.

>   - Related to that, I don't think we should accept random 'o' or 'g'
>     values either when building or when parsing, since (a) there's no good
>     reason to, and (b) '(oss)' for extensions is only unambiguous if
>     'o' isn't allowed otherwise.

tbh, I don't care much either way. I though it would be nicer to support as much of GVariant types as possible. It's not really ambiguous since the object path is in libsoup namespace. So it won't clash with any reasonable GVariant user wants to serialize, unless he explicitly want to fire a bullet in its foots. But if you've got strong opinion against, I'll just remove them, I don't care for my use cases.

>   - soup_xmlrpc_params_parse() should set an error if signature has
>     unused characters at the end?

Good point, that seems to be a bug in json-glib implementation I've followed (and diverted a lot since actually) as well.

>   - I don't think soup_xmlrpc_parse_request_full() is useful, since it
>     requires you to know the method's signature without knowing its name.

It can be used if all your methods have same signature, which is actually the case in one of my project' services. But I could remove it if you think it's important.

>   - soup_xmlrpc_new_custom() says "See soup_xmlrpc_build_request()", but
>     the soup_xmlrpc_build_request() docs don't talk about custom values.

Indeed, I would just remove soup_xmlrpc_new_custom() from API if it's fine with you.

>   - soup_xmlrpc_new_custom() and soup_xmlrpc_new_datetime() should have
>     "value" or "variant" in their name somewhere

soup_xmlrpc_variant_new_datetime() to follow g_variant_new_foo() pattern but in another namespace?
Comment 58 Dan Winship 2015-08-10 18:49:05 UTC
(In reply to Xavier Claessens from comment #57)
>  - You added in soup_xmlrpc_build_response()'s doc that it shouldn't be a
> tuple, but tuples are serialized as arrays so it's actually fine. In DBus
> the return value can be a tuple, it's considered a single value. Just
> mentioning, I don't care much personally.

I said "should not be a tuple here (unless the return value is an array)". I was trying to say that you just pass the value itself, not the value wrapped inside something. Unlike soup_xmlrpc_build_request() (or g_dbus_method_invocation_return_value()), where to pass a single value, you have to pass a tuple containing that single value rather than the single value itself.

But clearly I didn't express that well, so it needs to be rewritten a bit more.

>  - Why do you remove all old gvalue APIs from doc? Deprecated stuff should
> be still documented IMO.

Yeah, but I've generally removed deprecated docs in the past, to keep the docs from getting too cluttered. https://developer.gnome.org/libsoup/ keeps all the old versions of the docs, so people can also just read the old docs there if they want to use the old functions.

> >     Probably we should say that dateTime and extension values will be
> >     deserialized to 'v', and have reverse equivalents of
> >     soup_xmlrpc_new_datetime() and soup_xmlrpc_new_custom() to extract
> >     the values from those GVariants.
> 
> tbh, maybe I should just remove soup_xmlrpc_new_custom() from public API,
> because that's something that was not possible with previous API anyway. If
> someone needs it later for compat with some services we can still add it. I
> would keep only the "(oss)" trick for dateTime as internal implementation
> detail.

I'm fine with removing custom, but I think I'd still prefer to deserialize datetimes to 'v' like above.

> But if you've got strong opinion against, I'll just remove
> them, I don't care for my use cases.

Yeah, remove them ('o' and 'g' variants) please

> >   - I don't think soup_xmlrpc_parse_request_full() is useful, since it
> >     requires you to know the method's signature without knowing its name.
> 
> It can be used if all your methods have same signature, which is actually
> the case in one of my project' services. But I could remove it if you think
> it's important.

That's an edge case. I'd say remove it.

> >   - soup_xmlrpc_new_custom() and soup_xmlrpc_new_datetime() should have
> >     "value" or "variant" in their name somewhere
> 
> soup_xmlrpc_variant_new_datetime() to follow g_variant_new_foo() pattern but
> in another namespace?

sounds right
Comment 59 Xavier Claessens 2015-08-10 19:07:59 UTC
(In reply to Xavier Claessens from comment #57)
> (In reply to Dan Winship from comment #56)
> >   - soup_xmlrpc_params_parse() should set an error if signature has
> >     unused characters at the end?
> 
> Good point, that seems to be a bug in json-glib implementation I've followed
> (and diverted a lot since actually) as well.

Actually, no. I already check the signature with g_variant_type_string_is_valid(), so things like "ii" are forbidden, it must be "(ii)". parse_array() already check for too few/many values for a tuple.
Comment 60 Xavier Claessens 2015-08-10 19:51:58 UTC
Added more commits in wip/xmlrpc-variant.

(In reply to Dan Winship from comment #58)
> I'm fine with removing custom, but I think I'd still prefer to deserialize
> datetimes to 'v' like above.

I don't understand how is that useful. You would have to doc what that 'v' actually contains otherwise user cannot do anything with it. It could be deserialized to 's' and let the user parse it with soup_date_new_from_string() if you don't like the unix timestamp approach. I could also change soup_xmlrpc_variant_new_datetime() to take a SoupDate or string arg instead of a timestamp as well.
Comment 61 Dan Winship 2015-08-11 14:16:16 UTC
(In reply to Xavier Claessens from comment #60)
> (In reply to Dan Winship from comment #58)
> > I'm fine with removing custom, but I think I'd still prefer to deserialize
> > datetimes to 'v' like above.
> 
> I don't understand how is that useful. You would have to doc what that 'v'
> actually contains otherwise user cannot do anything with it.

No, we document that you get back a 'v', and you have to call soup_xmlrpc_variant_get_datetime() on it to extract the value. Exactly the inverse of how you need to use soup_xmlrpc_variant_new_datetime() to create a datetime value.

I guess maybe as a convenience we could allow it to be deserialized to 's' or 'x' too in addition to the canonical 'v' deserialization. Although doing 's' would be sketchy since you'd lose typechecking (the response might have had an actual <string> value there instead of a <dateTime.iso8601> value, and then your code might break if the string isn't a parseable datetime).

> It could be
> deserialized to 's' and let the user parse it with
> soup_date_new_from_string() if you don't like the unix timestamp approach. I
> could also change soup_xmlrpc_variant_new_datetime() to take a SoupDate or
> string arg instead of a timestamp as well.

Yeah, I can't quite make up my mind on this point actually; using time_t values is not quite right, because XML-RPC datetimes have no explicitly associated time zone, so it's better to give back a representation where the program can convert it to UTC or local time or whatever depending on the semantics of the XML-RPC API in question. But OTOH, SoupDate is mostly redundant with GDateTime now, so I'm not sure I want to use it in new APIs. Although on the other other hand, GDateTime can't represent "floating" (zone-less) times... so maybe using SoupDate here *would* be best. (Though I should probably add SoupDate<->GDateTime APIs at some point.)

We could have

  GVariant *soup_xmlrpc_variant_new_datetime             (SoupDate   *datetime);
  GVariant *soup_xmlrpc_variant_new_datetime_from_string (const char *datetime);
  GVariant *soup_xmlrpc_variant_new_datetime_from_time_t (time_t      datetime);

  SoupDate *soup_xmlrpc_variant_get_datetime             (GVariant   *var);
  char     *soup_xmlrpc_variant_get_datetime_as_string   (GVariant   *var);
  time_t    soup_xmlrpc_variant_get_datetime_as_time_t   (GVariant   *var):
Comment 62 Xavier Claessens 2015-08-11 20:19:59 UTC
(In reply to Dan Winship from comment #61)
>   GVariant *soup_xmlrpc_variant_new_datetime             (SoupDate  
> *datetime);
>   SoupDate *soup_xmlrpc_variant_get_datetime             (GVariant   *var);

Did that, other variants can be added later if/when needed.

Note that using the generic "v" in the signature has the downside that it can't be type checked with the signature anymore. For example if your server expects a datetime and you give "(v)" to soup_xmlrpc_params_parse()' signature, if client sends you something else it will still accept it. That's why I've added a GError arg to _get_datetime(), type checking is done there.
Comment 63 Xavier Claessens 2015-08-11 20:21:08 UTC
(wip/xmlrpc-variant updated)
Comment 64 Dan Winship 2015-08-13 14:04:48 UTC
OK, please squash the new fixup commits in with the original ones. Also, two of the commits ended up with my name on them instead of yours... you can use "git commit --amend --reset-author" to fix that.

Other than that, it looks good to commit, so let's get it in for next week's release! Thanks for your work on this.
Comment 65 Xavier Claessens 2015-08-13 15:34:29 UTC
Awesome, squashed and pushed. Thanks!
Comment 66 Xavier Claessens 2015-08-13 15:35:43 UTC
Created attachment 309213 [details] [review]
Add SOUP_DEPRECATED_IN_2_52 macro
Comment 67 Xavier Claessens 2015-08-13 15:35:56 UTC
Created attachment 309214 [details] [review]
xmlrpc: Rename files
Comment 68 Xavier Claessens 2015-08-13 15:36:05 UTC
Created attachment 309215 [details] [review]
xmlrpc: Add GVariant (de)serializer
Comment 69 Xavier Claessens 2015-08-13 15:36:16 UTC
Created attachment 309216 [details] [review]
xmlrpc: Deprecate gvalue API