GNOME Bugzilla – Bug 790671
A required property of a <entry/gContact:website> element (@href) was not present.
Last modified: 2017-11-27 19:12:35 UTC
I cannot read my contacts using libgdata git master at commit 83cce228, because when I let it refresh my address book from evolution libgdata aborts parsing of the feed during this entry returned from the server: > <entry gd:etag=""xxx""> > <id>http://www.google.com/m8/feeds/contacts/user/base/xxx</id> > <updated>2017-10-11T13:25:30.093Z</updated> > <app:edited xmlns:app="http://www.w3.org/2007/app">2017-10-11T13:25:30.093Z</app:edited> > <category scheme="http://schemas.google.com/g/2005#kind" term="http://schemas.google.com/contact/2008#contact"/> > <title>new contact from evo</title> > <link rel="http://schemas.google.com/contacts/2008/rel#photo" type="image/*" href="https://www.google.com/m8/feeds/photos/media/user/xxx"/> > <link rel="self" type="application/atom+xml" href="https://www.google.com/m8/feeds/contacts/user/full/xxx"/> > <link rel="edit" type="application/atom+xml" href="https://www.google.com/m8/feeds/contacts/user/full/xxx"/> > <gd:name> > <gd:fullName>new contact from evo</gd:fullName> > <gd:givenName>new</gd:givenName> > <gd:additionalName>contact</gd:additionalName> > <gd:familyName>from evo</gd:familyName> > </gd:name> > <gd:organization rel="http://schemas.google.com/g/2005#other"/> > <gd:email rel="http://schemas.google.com/g/2005#work" address="new-evo@no.where"/> > <gd:email rel="http://schemas.google.com/g/2005#home" address="changing1@no.where"/> > <gContact:website href="" rel="other"/> > <gContact:groupMembershipInfo deleted="false" href="http://www.google.com/m8/feeds/groups/user/base/6"/> > </entry> As long as the server lets me save empty website value, the libgdata should not abort parsing (it didn't abort the process, but the outcome is quite similar, I cannot access any contact in my address book).
Created attachment 364124 [details] [review] proposed patch A naive solution, allow empty @href for gContact:website.
Hmm, that’s a bit of a pain. I can’t see how a GDataGContactWebsite can be useful with an empty or missing href. I think I’d prefer the fix for this to be in parse_xml() in gdata-contacts-contact.c: it should ignore failure parsing a gContact:website element, and not try to add the object for it to ->websites. Could you update your patch to do that? Thanks!
Review of attachment 364124 [details] [review]: (see comment above)
I did think of that too, but all that hiding behind parsers and parseables and whatsoever pretty much confused me that I came with that naive solution instead. The difference is significant, because without the fix I do not see any contact, or I receive an empty website in some contacts (I have more of those here). And as I said, as long as the Google server allows it, you should too. When you hide the website objects with the empty URL to the caller, will it be able to remove such empty part, or they will just pile up during the time?
(In reply to Milan Crha from comment #4) > And as I said, as long as the Google server allows it, you should too. When > you hide the website objects with the empty URL to the caller, will it be > able to remove such empty part, or they will just pile up during the time? They will be dropped from the XML, so if you modify that contact locally and upload the modified XML, the empty website would be missing from the upload.
Created attachment 364283 [details] [review] proposed patch ][ Say something like this? Note, I: a) didn't want to change the whole if() statement in gdata-contacts-contact.c b) this way the new enum value can be reused by other properties, if needed.
Review of attachment 364283 [details] [review]: This looks like a good approach to me. I have a few comments, but they’re mostly about improving the structure of the code (and some of the problems were there in the code already, so aren’t your fault — but we might as well improve things while we’re touching this bit of code). ::: gdata/gdata-parser.c @@ -636,1 @@ Can you add a `g_return_val_if_fail (!(options & P_REQUIRED) || !(options & P_IGNORE_ERROR), FALSE)` please? @@ +648,3 @@ if (options & P_REQUIRED && parsable == NULL) { /* The error has already been set by _gdata_parsable_new_from_xml_node() */ + g_warn_if_fail (local_error != NULL); This can be a g_assert(), since we control all the code paths which could be setting it. Actually, it could be moved to just after the _gdata_parsable_new_from_xml_node() call, outside the big `if` statement: parsable = _gdata_parsable_new_from_xml_node (…, &local_error); g_assert ((parsable == NULL) == (local_error != NULL)); if (options & P_REQUIRED && parsable == NULL) { … @@ +650,3 @@ + g_warn_if_fail (local_error != NULL); + if (local_error) + g_propagate_error (error, local_error); Nitpick: Use g_steal_error() as below. @@ +657,3 @@ + *success = TRUE; + return TRUE; + } else if (local_error) { Nitpick: `else if (local_error != NULL)` (please make NULL comparisons explicit) @@ +658,3 @@ + return TRUE; + } else if (local_error) { + g_warn_if_fail (parsable == NULL); Similarly, this can be dropped in favour of the g_assert() above. @@ +659,3 @@ + } else if (local_error) { + g_warn_if_fail (parsable == NULL); + g_propagate_error (error, local_error); Nitpick: `g_propagate_error (error, g_steal_pointer (&local_error))` to make the ownership transfer explicit (though it doesn’t actually change the behaviour here at all). ::: gdata/services/contacts/gdata-contacts-contact.c @@ +896,3 @@ gdata_parser_object_from_element_setter (node, "event", P_REQUIRED, GDATA_TYPE_GCONTACT_EVENT, gdata_contacts_contact_add_event, self, &success, error) == TRUE || + gdata_parser_object_from_element_setter (node, "website", P_IGNORE_ERROR, GDATA_TYPE_GCONTACT_WEBSITE, Could you split this out into a separate patch from the core changes please? It’s possible that the GDataContactsContact change might be reverted in future if Google fix their servers. It would be easier to revert if separate from the core changes.
Review of attachment 364283 [details] [review]: I've just few questions before touching the change again. ::: gdata/gdata-parser.c @@ -636,1 @@ Is that to make sure they both are not set together? You have other mutually exclusive options where I didn't notice any such check for them (though I also didn't search for it). @@ +648,3 @@ if (options & P_REQUIRED && parsable == NULL) { /* The error has already been set by _gdata_parsable_new_from_xml_node() */ + g_warn_if_fail (local_error != NULL); I'm not a fan of g_assert(). Of course, if you control all paths to the place, then it can be done, but then it also means that your unit tests catch all those paths with all the options, which is pretty hard to achieve for any nontrivial library. @@ +650,3 @@ + g_warn_if_fail (local_error != NULL); + if (local_error) + g_propagate_error (error, local_error); g_steal_pointer(), okay @@ +657,3 @@ + *success = TRUE; + return TRUE; + } else if (local_error) { Right, I noticed this coding style in libgdata (comparing to TRUE is quite brave), but missed it here, I'm sorry (I prefer not to do such compares, because it can mean performance hits, due to reading data from memory; while it might not be a problem in this particular place, it's a good habit to minimize possible performance hits, from my point of view). ::: gdata/services/contacts/gdata-contacts-contact.c @@ +896,3 @@ gdata_parser_object_from_element_setter (node, "event", P_REQUIRED, GDATA_TYPE_GCONTACT_EVENT, gdata_contacts_contact_add_event, self, &success, error) == TRUE || + gdata_parser_object_from_element_setter (node, "website", P_IGNORE_ERROR, GDATA_TYPE_GCONTACT_WEBSITE, I probably can. I only do not like many patches attached into bugzilla. Working with them, while they all are related to one issue, is a pita. And having true revert of a change, well, I use git reverts very rarely, only for things which had been added erroneously, not for things which had been valid in time of the issue being fixed. It can be just different habits though.
(In reply to Milan Crha from comment #8) > ::: gdata/gdata-parser.c > @@ -636,1 @@ > > > Is that to make sure they both are not set together? You have other mutually > exclusive options where I didn't notice any such check for them (though I > also didn't search for it). Yes, this is to ensure they are both not set together. It’s a programming style I’ve developed since originally writing this code, which is why the other options don’t have g_assert()s. If you want to add other g_assert()s, that would be appreciated, but is not strictly necessary for this bug. > @@ +648,3 @@ > if (options & P_REQUIRED && parsable == NULL) { > /* The error has already been set by _gdata_parsable_new_from_xml_node() > */ > + g_warn_if_fail (local_error != NULL); > > I'm not a fan of g_assert(). Of course, if you control all paths to the > place, then it can be done, but then it also means that your unit tests > catch all those paths with all the options, which is pretty hard to achieve > for any nontrivial library. We control all paths to this code. > @@ +657,3 @@ > + *success = TRUE; > + return TRUE; > + } else if (local_error) { > > Right, I noticed this coding style in libgdata (comparing to TRUE is quite > brave), but missed it here, I'm sorry (I prefer not to do such compares, > because it can mean performance hits, due to reading data from memory; while > it might not be a problem in this particular place, it's a good habit to > minimize possible performance hits, from my point of view). The coding style of comparing to TRUE is something which I’m dropping from libgdata. But explicitly comparing to NULL is something which is still in place (to differentiate those comparisons from comparing to TRUE, so it’s obvious you’re dealing with a pointer type rather than a boolean type). How can comparing to NULL be a performance hit? > ::: gdata/services/contacts/gdata-contacts-contact.c > @@ +896,3 @@ > gdata_parser_object_from_element_setter (node, "event", P_REQUIRED, > GDATA_TYPE_GCONTACT_EVENT, > > gdata_contacts_contact_add_event, self, &success, error) == TRUE || > + gdata_parser_object_from_element_setter (node, "website", > P_IGNORE_ERROR, GDATA_TYPE_GCONTACT_WEBSITE, > > I probably can. I only do not like many patches attached into bugzilla. > Working with them, while they all are related to one issue, is a pita. And > having true revert of a change, well, I use git reverts very rarely, only > for things which had been added erroneously, not for things which had been > valid in time of the issue being fixed. It can be just different habits > though. I’d much prefer it if the patches were separated out. Are you using git-bz? I find it makes dealing with multiple patches on Bugzilla a lot easier. In any case, I expect to be approving these patches as soon as you attach them, since there are unlikely to be any more review issues. Thanks.
(In reply to Philip Withnall from comment #9) > How can comparing to NULL be a performance hit? Okay, I do not know anything about the compiler internals, but I guess a comparison against certain static value requires the value to be stored in the memory somewhere, it surely cannot be stored in a register for the whole time. And the NULL is usually defined as "((void *)0)". That means that to make the comparison, the processor needs to read the value from the memory first. And it costs a bit. One might not notice doing it once, but do it million times and it'll show itself (avoid caching and it'll be even more). Of course, smart enough compiler optimizations can turn such particular compares from "a == NULL" to simple "a" expression (I often compile with -O0, to have complete backtraces). Anyway, this is just my habit, which can be even wrong (by the way, I use NULL compares in g_warn_if/g_return_if macros for readability on the console), and you've different habits, which I'm fine with. I want to use the coding style the project is developed with, I only make mistakes when it is different from the style I use myself. :) > Are you using git-bz? No, I'm not, but I'll do something similar.
(In reply to Milan Crha from comment #10) > (In reply to Philip Withnall from comment #9) > > How can comparing to NULL be a performance hit? > > Okay, I do not know anything about the compiler internals, but I guess a > comparison against certain static value requires the value to be stored in > the memory somewhere, it surely cannot be stored in a register for the whole > time. And the NULL is usually defined as "((void *)0)". That means that to > make the comparison, the processor needs to read the value from the memory > first. And it costs a bit. One might not notice doing it once, but do it > million times and it'll show itself (avoid caching and it'll be even more). > Of course, smart enough compiler optimizations can turn such particular > compares from "a == NULL" to simple "a" expression (I often compile with > -O0, to have complete backtraces). Anyway, this is just my habit, which can > be even wrong (by the way, I use NULL compares in g_warn_if/g_return_if > macros for readability on the console), and you've different habits, which > I'm fine with. I’m pretty sure any modern compiler will know how to optimise comparisons against a constant NULL/TRUE/FALSE. > I want to use the coding style the project is developed with, I only make > mistakes when it is different from the style I use myself. :) Thanks. > > Are you using git-bz? > > No, I'm not, but I'll do something similar. https://blog.fishsoup.net/2008/11/16/git-bz-bugzilla-subcommand-for-git/. It’s really useful.
Created attachment 364512 [details] [review] proposed patch ]I[ (a) The first part of the patch. I hope I didn't overlook anything.
Created attachment 364513 [details] [review] proposed patch ]I[ (b) The second part, one chunk from gdata/services/contacts/gdata-contacts-contact.
By the way, I do not have commitable ckeckout of libgdata, neither I'm sure I'd be able to write proper commit message (note of the overlook of the "!= NULL" compare in such a small patch).
Thanks for the patches. I’ve added some test cases and commit messages and pushed them to master. :-)