Closed Bug 490867 Opened 11 years ago Closed 11 years ago

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

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: adw, Assigned: adw)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 9 obsolete files)

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.
Flags: in-testsuite?
Attached patch v1 (obsolete) — Splinter Review
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. :)
Attachment #375219 - Flags: review?(sdwilsh)
Summary: Async API should coerce the null variant to empty string → Variant should coerce the null variant to empty string
Target Milestone: --- → mozilla1.9.2a1
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.
Attachment #375219 - Flags: review?(sdwilsh) → review+
This will be difficult to test until we expose Variant publicly.
Attached patch for checkin (obsolete) — Splinter Review
Addressed comment 3.
Attachment #375219 - Attachment is obsolete: true
Keywords: checkin-needed
Keywords: checkin-needed
Attached patch return VTYPE_VOID datatype (obsolete) — Splinter Review
Actually the null variant's type ought to be nsIDataType::VTYPE_VOID, not nsIDataType::VTYPE_EMPTY, no?  mozStorageRow::GetIsNull() is wrong otherwise...
Attachment #375286 - Flags: review?(sdwilsh)
No, we want VTYPE_VOID (http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsVariant.cpp#1583), so GetIsNull is wrong there.
I'm confused.  Did you mean we want VTYPE_EMPTY?
yeah, I meant to say we do not want VTYPE_VOID.
Changing bug name to better reflect scope.  If we should break it up into three separate bugs, let me know.  Will post new patch.
Summary: Variant should coerce the null variant to empty string → Null variant fixes: coerce to empty string; return correct type; GetIsNull should check correct type
Attached patch v4 (obsolete) — Splinter Review
Attachment #375248 - Attachment is obsolete: true
Attachment #375286 - Attachment is obsolete: true
Attachment #375345 - Flags: review?(sdwilsh)
Attachment #375286 - Flags: review?(sdwilsh)
Attached patch v5 (obsolete) — Splinter Review
NullVariant::GetDataType() from previous patches no longer needed.
Attachment #375345 - Attachment is obsolete: true
Attachment #375346 - Flags: review?(sdwilsh)
Attachment #375345 - Flags: review?(sdwilsh)
Summary: Null variant fixes: coerce to empty string; return correct type; GetIsNull should check correct type → Null variant fixes: coerce to empty string, GetIsNull should check correct type
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
Attachment #375346 - Flags: review?(sdwilsh) → review+
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.
(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.
eh, yeah, that's what I meant.  It's early still...
Summary: Null variant fixes: coerce to empty string, GetIsNull should check correct type → Variant fixes: coerce null to empty string, GetIsNull should check correct type, variant base type should be void
Attached patch v6 (obsolete) — Splinter Review
Requesting review just to be safe.  I added a few NS_ENSURE_ARG_POINTER checks.
Attachment #375346 - Attachment is obsolete: true
Attachment #375375 - Flags: review?(sdwilsh)
Attachment #375375 - Flags: review?(sdwilsh) → review+
Comment on attachment 375375 [details] [diff] [review]
v6

Use do_check_true or do_check_false in the test please

r=sdwilsh
Attached patch for checkin v2 (obsolete) — Splinter Review
Attachment #375375 - Attachment is obsolete: true
Keywords: checkin-needed
Attached patch for checkin v2 (unbitrotted) (obsolete) — Splinter Review
Attachment #375438 - Attachment is obsolete: true
Attached patch for checkin v3 (unbitrotted) (obsolete) — Splinter Review
Shawn, if we're going to take this at all, could we do it soon so it doesn't get bitrotted again?
Attachment #378448 - Attachment is obsolete: true
oh come on - you don't like to unbitrot things?
Comment on attachment 384678 [details] [diff] [review]
for checkin v3 (unbitrotted)


{
storage/test/unit/test_storage_statement_executeAsync.js not tracked!
}
Keywords: checkin-needed
Whiteboard: [c-n: sorry, v4 needed]
Attachment #384678 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [c-n: sorry, v4 needed]
http://hg.mozilla.org/mozilla-central/rev/074a69d4dcce
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.