Created attachment 284609 [details] [review]
Migrate sync_data from EBookBackendSqliteDB to EBookSqlite

Created attachment 284610 [details] [review]
test-book-client-custom-summary: Add 'exists' tests for email and X509 certificates

Created attachment 284611 [details] [review]
EBookSqlite: Support 'exists' queries from summary

Created attachment 284612 [details] [review]
EBookSqlite: Add support for storing x509Cert in a boolean summary field

Created attachment 284613 [details] [review]
EBookSqlite: Enable x509Cert summary field by default

Comment on attachment 284609 [details] [review]
Migrate sync_data from EBookBackendSqliteDB to EBookSqlite

Looks good, please commit

Comment on attachment 284608 [details] [review]
Allow EBookSqlite to be used without transactions

Thanks for the fix, it was intended to be usable this way but hasn't been tested enough. Please commit to master.

Comment on attachment 284611 [details] [review]
EBookSqlite: Support 'exists' queries from summary

Thanks for fixing this.

Comment on attachment 284612 [details] [review]
EBookSqlite: Add support for storing x509Cert in a boolean summary field

Just a note on the overall patch since this one deserves some attention...

At first I didn't like the approach because I feel the right approach would
be to adjust EBookSqlite to allow storing the "existence" of fields in the
summary and without mandating that a normalized copy will be held in the
summary, which would be instead explicit with E_BOOK_INDEX_PREFIX.

Now it doesn't look so special-casey anymore, and the way I see it what
this patch does is simply:
  "Add limited search capabilities to EBookSqlite for EContactCert fields"

Where "limited" means, only support for the "exists" queries.

So overall I'm fine with the patch, I'd like you to make a couple of
small changes before committing this, though:

>@@ -4536,6 +4550,17 @@ query_preflight_check (PreflightContext *context,
> 
> 		switch (field_test) {
> 		case E_BOOK_QUERY_IS:
>+			if (test->field && test->field->type == E_TYPE_CONTACT_CERT) {
>+			no_cert_content:
>+				context->status = MAX (context->status, PREFLIGHT_NOT_SUMMARIZED);
>+				EBSQL_NOTE (
>+					PREFLIGHT,
>+					g_printerr (
>+						"PREFLIGHT CHECK: "
>+						"Refusing content check on X.509 cert field '%s', new status: %s\n",
>+						EBSQL_FIELD_ID_STR (test->field_id),
>+						EBSQL_STATUS_STR (context->status)));
>+			}
> 			break;
> 
> 		case BOOK_QUERY_EXISTS:
>@@ -4545,11 +4570,15 @@ query_preflight_check (PreflightContext *context,
> 		case E_BOOK_QUERY_REGEX_NORMAL:
> 
> 			/* All of these queries can only apply to string fields,
>-			 * or fields which hold multiple strings 
>+			 * or fields which hold multiple strings. Except for
>+			 * BOOK_QUERY_EXISTS which can be used for the cert field.
> 			 */
> 			if (test->field) {
>-				if (test->field->type != G_TYPE_STRING &&
>-				    test->field->type != E_TYPE_CONTACT_ATTR_LIST) {
>+				if (test->field->type == E_TYPE_CONTACT_CERT) {
>+					if (field_test != BOOK_QUERY_EXISTS)
>+						goto no_cert_content;
>+				} else if (test->field->type != G_TYPE_STRING &&
>+					   test->field->type != E_TYPE_CONTACT_ATTR_LIST) {


This whole area of code is just to check that the parsed arguments are safe
to use to build a sane query from the parsed S-Expression input, I think that
since the EContactCert field really is the exception here, it might be cleaner
to give it a special case outside of the switch() statement, and thus avoiding
the goto statement (and avoiding duplicating the large EBSQL_NOTE statement).

Another thing which needs to be done here is that now that we allow parsing
of S-Expression statements with only one argument, it's possible to have
field tests that don't include values.

So basically we now need to check:
   (test->value == NULL && field_test != BOOK_QUERY_EXISTS)

and bump the preflight status to PREFLIGHT_INVALID here.

Comment on attachment 284610 [details] [review]
test-book-client-custom-summary: Add 'exists' tests for email and X509 certificates

Looks good, please commit.

Comment on attachment 284613 [details] [review]
EBookSqlite: Enable x509Cert summary field by default

Looks good. I wasn't sure you needed the explicit bit where you append the new summary field, I suppose that was needed.

Comment on attachment 284613 [details] [review]
EBookSqlite: Enable x509Cert summary field by default

Indeed. Without that bit, I get a database claiming to be schema version 9 but still lacking the x509Cert summary field.

Comment on attachment 284608 [details] [review]
Allow EBookSqlite to be used without transactions

I'm going to hold off on this one. It makes lockless operation work when that's all that's happening, but in that case you must *never* use locks.

The problem with that is that the read code does appear to use locks. So if I access the EWS GAL while it's updating, I get a crash.

So if we want to support "lockless" operation I suspect we might want to implicitly take and release the overall lock in the operations that would want it.

Comment on attachment 284609 [details] [review]
Migrate sync_data from EBookBackendSqliteDB to EBookSqlite

Pushed to master and also to 3.12 because we *really* don't want databases getting migrated and losing data.

(In reply to comment #9)
> So basically we now need to check:
>    (test->value == NULL && field_test != BOOK_QUERY_EXISTS)
> 
> and bump the preflight status to PREFLIGHT_INVALID here.

Hm, really? Elsewhere in the function we have an explicit check for NULL with a comment that suggests it's not just defensive: "If we search for a NULL or zero length string..."

Is that incorrect? I can't sde how we'd *really* end up with NULL there. Even when test-book-client-custom-summary.c does a check on the behaviour of e_book_query_any_field_contains(NULL) that ends up in query_preflight_check() as the empty string "" and not NULL — because it's parsed from a string with e_sexp_parse().

If you confirm that we really can't have NULL, I'll fix at least the comment there too.

Testing shows that this definitely 'breaks' the behaviour of a query string
(contains "x-evolution-any-field")

That used to just end up as 'not summarized'. But now it's marked as invalid. Perhaps correctly so, but I'd like confirmation of that before I go ahead and break it... and we should actually make it fail *consistently* rather than just in EBookSqlite, shouldn't we?

(In reply to comment #9)
> So overall I'm fine with the patch, I'd like you to make a couple of
> small changes before committing this, though:
> 
> This whole area of code is just to check that the parsed arguments are safe
> to use to build a sane query from the parsed S-Expression input, I think that
> since the EContactCert field really is the exception here, it might be cleaner
> to give it a special case outside of the switch() statement, and thus avoiding
> the goto statement (and avoiding duplicating the large EBSQL_NOTE statement).

Done. It's now got a 'return' to avoid going into the switch statement, and I'm not convinced it's any less ugly. But OK :)

> Another thing which needs to be done here is that now that we allow parsing
> of S-Expression statements with only one argument, it's possible to have
> field tests that don't include values.
> 
> So basically we now need to check:
>    (test->value == NULL && field_test != BOOK_QUERY_EXISTS)
> 
> and bump the preflight status to PREFLIGHT_INVALID here.

Yeah, this is a larger can of worms, as discussed. I'm uncomfortable returning PREFLIGHT_INVALID *only* in EBookSqlite for a query which actually does seem to work differently elsewhere, so I've gone for the simple option of limiting my own change (in commit b8c18cfd) so it doesn't affect the behaviour for any other types of query:

+       if (argc == 1 && query_type == BOOK_QUERY_EXISTS &&
+           argv[0]->type == ESEXP_RES_STRING) {
...

(In reply to comment #13)
> (From update of attachment 284608 [details] [review])
> I'm going to hold off on this one. It makes lockless operation work when that's
> all that's happening, but in that case you must *never* use locks.
> 
> The problem with that is that the read code does appear to use locks. So if I
> access the EWS GAL while it's updating, I get a crash.
> 
> So if we want to support "lockless" operation I suspect we might want to
> implicitly take and release the overall lock in the operations that would want
> it.

Indeed, that locking stuff was changed a little too quickly while adding
last minute support for GCancellable.

I think the result is as you say,
  o You can use e_book_sqlite_lock()/unlock() to make multiple
    calls to EBookSqlite an atomic transaction, and hold an explicit
    mutex.

  o You can use EBookSqlite without _lock()/_unlock() at all

  o Trying to use _lock()/_unlock() sometimes but not always
    can result in the not "locked" calls to be interleaved
    into what might be an ongoing "locked" transaction in another
    thread.

Actually, it looks like at least I documented it to work that way...

From e_book_sqlite_lock():
  Aside from ensuring atomicity of transactions, this function 
  will hold a mutex which will cause further calls to e_book_sqlite_lock()
  to block. If you are accessing @ebsql from multiple threads, then any 
  interactions with @ebsql should be nested in calls to
  e_book_sqlite_lock() and e_book_sqlite_unlock().

Sorry I didn't remember this earlier.

However, it is supposed to allow you to not specify a GCancellable at all,
so I think this issue notwithstanding, the patch to allow the NULL cancellables
should be fine.

(In reply to comment #15)
> (In reply to comment #9)
> > So basically we now need to check:
> >    (test->value == NULL && field_test != BOOK_QUERY_EXISTS)
> > 
> > and bump the preflight status to PREFLIGHT_INVALID here.
> 
> Hm, really? Elsewhere in the function we have an explicit check for NULL with a
> comment that suggests it's not just defensive: "If we search for a NULL or zero
> length string..."
> 
> Is that incorrect? I can't sde how we'd *really* end up with NULL there. Even
> when test-book-client-custom-summary.c does a check on the behaviour of
> e_book_query_any_field_contains(NULL) that ends up in query_preflight_check()
> as the empty string "" and not NULL — because it's parsed from a string with
> e_sexp_parse().
> 
> If you confirm that we really can't have NULL, I'll fix at least the comment
> there too.

Sorry I was off base on this, as we discussed on IRC we should probably
amend the (already existing) table of information about query types to
include more information for the purpose of this input validation.

In any case this is orthogonal to your patches...

(In reply to comment #17)
[...]
> Yeah, this is a larger can of worms, as discussed. I'm uncomfortable returning
> PREFLIGHT_INVALID *only* in EBookSqlite for a query which actually does seem to
> work differently elsewhere, so I've gone for the simple option of limiting my
> own change (in commit b8c18cfd) so it doesn't affect the behaviour for any
> other types of query:
> 
> +       if (argc == 1 && query_type == BOOK_QUERY_EXISTS &&
> +           argv[0]->type == ESEXP_RES_STRING) {
> ...

Alright let's commit and close this then :)

EWS master is now ported to EBookSqlite and supports cursors.