490867 - Variant fixes: coerce null to empty string, GetIsNull should check correct type, variant base type should be void

Status: RESOLVED FIXED

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

Description

[Drew Willcoxon :adw]

The sync API returns the empty string when a string getter is called on a null value:

http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageStatement.cpp#744
http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageStatement.cpp#767

Places code as it exists right now depends on this behavior.

However, the async API returns NS_ERROR_CANNOT_CONVERT_DATA when the same thing is attempted.  At least the two APIs should be consistent.  After talking with Shawn, I'll attempt to make the async consistent with the sync so that nulls are returned as the empty string.

Comment 1

[Drew Willcoxon :adw]

Attachment: v1

Adds a NullVariant subclass of Variant_base.

For the record, talked again with Shawn, who wisely says that just because the sync API acts one way doesn't mean it's right. :)

Comment 2

[Shawn Wilsher :sdwilsh]

Comment on attachment 375219 [details] [diff] [review]
v1

>+  // Return empty string
nit 2x: comment inside method, end in punctuation, and you are actually returning a void string, not an empty string.

Comment 3

[Shawn Wilsher :sdwilsh]

This will be difficult to test until we expose Variant publicly.

Comment 4

[Drew Willcoxon :adw]

Attachment: for checkin

Addressed comment 3.

Comment 5

[Drew Willcoxon :adw]

Attachment: return VTYPE_VOID datatype

Actually the null variant's type ought to be nsIDataType::VTYPE_VOID, not nsIDataType::VTYPE_EMPTY, no?  mozStorageRow::GetIsNull() is wrong otherwise...

Comment 6

[Shawn Wilsher :sdwilsh]

No, we want VTYPE_VOID (http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsVariant.cpp#1583), so GetIsNull is wrong there.

Comment 7

[Drew Willcoxon :adw]

I'm confused.  Did you mean we want VTYPE_EMPTY?

Comment 8

[Shawn Wilsher :sdwilsh]

yeah, I meant to say we do not want VTYPE_VOID.

Comment 9

[Drew Willcoxon :adw]

Changing bug name to better reflect scope.  If we should break it up into three separate bugs, let me know.  Will post new patch.

Comment 10

[Drew Willcoxon :adw]

Attachment: v4

Comment 11

[Drew Willcoxon :adw]

Attachment: v5

NullVariant::GetDataType() from previous patches no longer needed.

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 375346 [details] [diff] [review]
v5

>+    // Return void string.
nit: return a void string.  articles are fun

Also, please add a test for getIsNull here for all data types:
http://mxr.mozilla.org/mozilla-central/source/storage/test/unit/test_storage_statement_executeAsync.js#156

r=sdwilsh

Comment 13

[Shawn Wilsher :sdwilsh]

And, also, we should keep the base variant as a VTYPE_VOID.  VOID maps to undefined in JS, which is exactly what the base should be.  VTYPE_EMPTY maps to null in JS.

Comment 14

[Drew Willcoxon :adw]

(In reply to comment #13)
By keep you mean "change" right? :) Would it be appropriate to handle that in this bug?  I could broaden its scope again.  NullVariant will need GetDataType() defined.

Comment 15

[Shawn Wilsher :sdwilsh]

eh, yeah, that's what I meant.  It's early still...

Comment 16

[Drew Willcoxon :adw]

Attachment: v6

Requesting review just to be safe.  I added a few NS_ENSURE_ARG_POINTER checks.

Comment 17

[Shawn Wilsher :sdwilsh]

Comment on attachment 375375 [details] [diff] [review]
v6

Use do_check_true or do_check_false in the test please

r=sdwilsh

Comment 18

[Drew Willcoxon :adw]

Attachment: for checkin v2

Comment 19

[Drew Willcoxon :adw]

Attachment: for checkin v2 (unbitrotted)

Comment 20

[Drew Willcoxon :adw]

Attachment: for checkin v3 (unbitrotted)

Shawn, if we're going to take this at all, could we do it soon so it doesn't get bitrotted again?

Comment 21

[Shawn Wilsher :sdwilsh]

oh come on - you don't like to unbitrot things?

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 384678 [details] [diff] [review]
for checkin v3 (unbitrotted)


{
storage/test/unit/test_storage_statement_executeAsync.js not tracked!
}

Comment 23

[Drew Willcoxon :adw]

Attachment: for checkin v4 (unbitrotted)

Comment 24

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/074a69d4dcce