391697 - add escapeStringForLIKE to mozIStorageStatement

Status: RESOLVED FIXED

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

Description

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

add EscapeForLIKE to mozIStorageStatement

attaching a patch and a test case. 

sdwilsh wrote that he'd like to make this return a UTF8String, but I'm not sure that is ideal.

I'd prefer to keep it an AString, to avoid conversions.  I've fixed my C++ in history autocomplete from:

  rv = mDBAutoCompleteQuery->BindUTF8StringParameter(2, NS_ConvertUTF16toUTF8(escapedSearchString));
  NS_ENSURE_SUCCESS(rv, rv);

to

  rv = mDBAutoCompleteQuery->BindUTF8StringParameter(2, escapedSearchString);
  NS_ENSURE_SUCCESS(rv, rv);

Comment 1

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

Attachment: patch

Comment 2

[Shawn Wilsher :sdwilsh]

So, I'm thinking that letting the consumer specify what they want the escape character to be would be a very good thing.

Also, for consistency with the interface, let's call it |escapeStringForLIKE|.

I'm also wondering if, for C++ consumers, we want to have an |escapeUTF8StringForLIKE| so they do not have to convert.

Lastly, can you indicate in the javadoc header that consumers will have to add the ESCAPE {their character} in as well?

Comment 3

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

Attachment: patch

shawn, I did not add the UTF8 version, as the current consumer in C++ doesn't need it.

Sample consumer code:

  nsString escapedSearchString;
  rv = mDBAutoCompleteQuery->EscapeStringForLIKE(mCurrentSearchString, PRUnichar('/'), escapedSearchString);
  NS_ENSURE_SUCCESS(rv, rv);

  // prepend and append with % for "contains"
  rv = mDBAutoCompleteQuery->BindStringParameter(2, NS_LITERAL_STRING("%") + escapedSearchString + NS_LITERAL_STRING("%"));
  NS_ENSURE_SUCCESS(rv, rv);

Comment 4

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

Attachment: improve comments in idl

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 276171 [details] [diff] [review]
improve comments in idl

>+/* AString escapeStringForLIKE(in AString aValue, in char aEscapeChar); */
>+NS_IMETHODIMP
>+mozStorageStatement::EscapeStringForLIKE(const nsAString & aValue, const PRUnichar aEscapeChar, nsAString &aEscapedValue)
nit: 80 char line wrapping
nit: s/aEscapedValue/escapedString/

>+{
>+    const PRUnichar MATCH_ALL('%');
>+    const PRUnichar MATCH_ONE('_');
>+
>+    aEscapedValue.Truncate(0);
>+
>+    PRInt32 i;
>+    for (i = 0; i < aValue.Length(); i++) {
nit: declare i inside the for loop

>+      if (aValue[i] == aEscapeChar || aValue[i] == MATCH_ALL || aValue[i] == MATCH_ONE)
nit: 80 char line wrapping

>+        aEscapedValue += aEscapeChar;
nit: indentation is 4 spaces in this file, not 2

r=sdwilsh, with nits fixed.

Comment 6

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

Attachment: patch as checked in

Comment 7

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

Attachment: patch as checked in

Comment 8

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

fixed.

Checking in public/mozIStorageStatement.idl;
/cvsroot/mozilla/storage/public/mozIStorageStatement.idl,v  <--  mozIStorageStat
ement.idl
new revision: 1.9; previous revision: 1.8
done
Checking in src/mozStorageStatement.cpp;
/cvsroot/mozilla/storage/src/mozStorageStatement.cpp,v  <--  mozStorageStatement
.cpp
new revision: 1.25; previous revision: 1.24
done
RCS file: /cvsroot/mozilla/storage/test/unit/test_like_escape.js,v
done
Checking in test/unit/test_like_escape.js;
/cvsroot/mozilla/storage/test/unit/test_like_escape.js,v  <--  test_like_escape.
js
initial revision: 1.1
done

Comment 9

[Eric Shepherd [:sheppy]]

Removing doc needed keyword in favor of a new bug that covers the fact that in general we actually need proper documentation for Storage; that bug (bug 399535) is set as blocked by this one.