391156 - mozStorage doesn't handle unicode in LIKE, UPPER, or LOWER functions

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[(not reading, please use [e-mail] instead)]

url bar autocomplete not finding matches due to non-ascii case sensitivity with SQLite

spun out from bug #389491

sqlite's LIKE has problems with non-ascii and case sensitivity:

The LIKE operator is not case sensitive and will match upper case characters on
one side against lower case characters on the other. (A bug: SQLite only
understands upper/lower case for 7-bit Latin characters. Hence the LIKE
operator is case sensitive for 8-bit iso8859 characters or UTF-8 characters.
For example, the expression 'a' LIKE 'A' is TRUE but 'æ' LIKE 'Æ' is FALSE.).

but there is an extenstion that can help.  See
http://www.mail-archive.com/[e-mail]/msg26040.html

http://www.sqlite.org/cvstrac/fileview?f=sqlite/ext/icu/README.txt&v=1.2

note, the download manager will aso need the icu extension.

Comment 1

[Shawn Wilsher :sdwilsh]

Should this be a storage bug to get icu?

Comment 2

[(not reading, please use [e-mail] instead)]

steps to reproduce this bug:

1)  visit http://en.wikipedia.org/wiki/%C3%86
2)  in your url bar, type Æ (you will get a title match: "Æ - Wikipedia, the free encyclopedia")
3)  in your url bar, type æ

expected result:  like Æ, you will get a title match: "Æ - Wikipedia, the free encyclopedia"

actual results:  you won't get a match

Comment 3

[(not reading, please use [e-mail] instead)]

note, this debug depends on bug #389491 landing.

Comment 4

[Shawn Wilsher :sdwilsh]

So, we cannot actually use the icu sqlite extension because the icu library is huge and we have our own unicode library...

Comment 5

[(not reading, please use [e-mail] instead)]

shawn, thanks for looking into this.

to clarify, the problem is that icu.c includes the ICU headers:

/* Include ICU headers */
#include <unicode/utypes.h>
#include <unicode/uregex.h>
#include <unicode/ustring.h>
#include <unicode/ucol.h>

and we don't want to include the icu library.

going forward, it looks like we'll have to take icu.c and map functions (such as icuLikeCompare) to our existing code?

Comment 6

[(not reading, please use [e-mail] instead)]

or, will we need our own extension, similar to icu.c, to register our own "like" (and possibly, "lower", "upper", etc) with sqlite?

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This doesn't actually work - we are failing the test cases.  I'm not sure why though.  Seth, would you mind taking a look at this?

Comment 8

[(not reading, please use [e-mail] instead)]

> Seth, would you mind taking a look at this?

applying and looking now.

Comment 9

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

fixes a minor bug in caseFunction

Comment 10

[(not reading, please use [e-mail] instead)]

shawn, try this:

    } else {
      // CASE 4
      if (ToUpperCase(*aStringItr) != ToUpperCase(*aPatternItr)) {
        // If we've hit a point where the strings don't match, there is no match
        return 0;
      }
      aStringItr++;
      aPatternItr++;
      lastWasEscape = PR_FALSE;
    }

I'll apply your latest patch, make that change, and upload a new version.

Comment 11

[(not reading, please use [e-mail] instead)]

question for shawn:

our idl has:

mozIStorageStatement createStatement(in AUTF8String aSQLStatement);

but in your patch, we are treating the statement text as utf-16.

I wonder if your these tests, we need to use bindStringParameter()?

trying that now.

Comment 12

[(not reading, please use [e-mail] instead)]

Attachment: v1.2

Comment 13

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

Comment 14

[(not reading, please use [e-mail] instead)]

Attachment: v1.4

after much pain, shawn and I figured out why this stuff works from C++ but not our js unit tests.

../../_tests/xpcshell-simple/test_storage/unit/test_unicode.js: PASS

Comment 15

[(not reading, please use [e-mail] instead)]

Attachment: v1.5 (removing xFree)

Comment 16

[(not reading, please use [e-mail] instead)]

Attachment: v1.6 (more tests, some of these don't pass)

Comment 17

[(not reading, please use [e-mail] instead)]

note to shawn:  test_escape_for_like_ascii will fail (and I'm sure test_escape_for_like_non_ascii would, too)

while the non-ascii part of likeCompare() appears to work, the handling of escaped terms needs a little work.

we should also add more tests for MATCH_ONE and MATCH_ALL.

Perhaps we should convert parts of sqlite-3.4.1\test\like.test to javascript

Comment 18

[(not reading, please use [e-mail] instead)]

Attachment: v1.7, more tests (again, some of these don't pass)

Comment 19

[(not reading, please use [e-mail] instead)]

open questions:

1) non-ascii GLOB, MATCH, REGEX support? (currently, no, only ascii)
2) "PRAGMA case_sensitive_like=on;" support?  (after this patch, no)

todo:

1) fix our new LIKE implementation to pass these tests (and the tests in test_unicode.js)
2) add more LIKE and unicode tests

Comment 20

[Shawn Wilsher :sdwilsh]

ha!  found the issue.  We weren't increasing the pattern iterator :(

New patch to come once I fix up a few more things.

Comment 21

[Shawn Wilsher :sdwilsh]

Also, I think it's best if we do the escapeLike stuff in a separate bug.

Comment 22

[Shawn Wilsher :sdwilsh]

(In reply to comment #19)
> 1) non-ascii GLOB, MATCH, REGEX support? (currently, no, only ascii)
follow-up

> 2) "PRAGMA case_sensitive_like=on;" support?  (after this patch, no)
ug - let's do that in a follow-up as well

Comment 23

[Shawn Wilsher :sdwilsh]

Attachment: v1.8

I think that our test coverage is sufficient now, but we should probably import all of sqlite's tests into our own tree at some point.

Comment 24

[(not reading, please use [e-mail] instead)]

Comment on attachment 276003 [details] [diff] [review]
v1.8

r=sspitzer, thanks shawn!

Comment 25

[(not reading, please use [e-mail] instead)]

I'm going to land for shawn, add the credit to test_like.js (which comes from like.test in SQLite code base), and log the spin off bugs mentioned in this bug, and prepare a patch for escapeLike for review.

thanks again, shawn for working on this!

Comment 26

[(not reading, please use [e-mail] instead)]

fixed.  I added a note about how some of the code was based on icu.c and test.like before checking in.

Checking in storage/build/Makefile.in;
/cvsroot/mozilla/storage/build/Makefile.in,v  <--  Makefile.in
new revision: 1.12; previous revision: 1.11
done
Checking in storage/src/Makefile.in;
/cvsroot/mozilla/storage/src/Makefile.in,v  <--  Makefile.in
new revision: 1.7; previous revision: 1.6
done
Checking in storage/src/mozStorageConnection.cpp;
/cvsroot/mozilla/storage/src/mozStorageConnection.cpp,v  <--  mozStorageConnecti
on.cpp
new revision: 1.28; previous revision: 1.27
done
RCS file: /cvsroot/mozilla/storage/src/mozStorageUnicodeFunctions.cpp,v
done
Checking in storage/src/mozStorageUnicodeFunctions.cpp;
/cvsroot/mozilla/storage/src/mozStorageUnicodeFunctions.cpp,v  <--  mozStorageUn
icodeFunctions.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/storage/src/mozStorageUnicodeFunctions.h,v
done
Checking in storage/src/mozStorageUnicodeFunctions.h;
/cvsroot/mozilla/storage/src/mozStorageUnicodeFunctions.h,v  <--  mozStorageUnic
odeFunctions.h
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/storage/test/unit/test_like.js,v
done
Checking in storage/test/unit/test_like.js;
/cvsroot/mozilla/storage/test/unit/test_like.js,v  <--  test_like.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/storage/test/unit/test_unicode.js,v
done
Checking in storage/test/unit/test_unicode.js;
/cvsroot/mozilla/storage/test/unit/test_unicode.js,v  <--  test_unicode.js
initial revision: 1.1
done

Comment 27

[(not reading, please use [e-mail] instead)]

s/test.like/like.test

Comment 28

[Eli Friedman]

Some minor string nits:

1. You don't need to use PromiseFlatString on nsAutoStrings; just calling get()
is sufficient.

2. In likeFunction, you should use an nsDependentString rather than an
nsAutoString to avoid an unnecessary string copy.

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

+  NS_ASSERTION(B.Length() != 0, "LIKE string must not be null!");

check IsEmpty() instead

Comment 30

[(not reading, please use [e-mail] instead)]

eli / christian:  will follow up with a patch to address those string suggestions.

Comment 31

[Shawn Wilsher :sdwilsh]

Attachment: v1.8.1

hey, this should make our LIKE a slight bit faster :)

Comment 32

[(not reading, please use [e-mail] instead)]

Attachment: supplimental string changes, per elif and biesi

Comment 33

[(not reading, please use [e-mail] instead)]

Comment on attachment 276138 [details] [diff] [review]
v1.8.1

r=sspitzer, thanks shawn

Comment 34

[Shawn Wilsher :sdwilsh]

Attachment: v1.8.2

I missed biesi's comment

Comment 35

[Shawn Wilsher :sdwilsh]

Checking in storage/src/mozStorageUnicodeFunctions.cpp;
new revision: 1.2; previous revision: 1.1