search

w3c / IndexedDB

Indexed Database API
https://w3c.github.io/IndexedDB/
Other
237 stars 62 forks source link

Clarify the handling of +0 and -0 in keys #375

Open abacabadabacaba opened 2 years ago

abacabadabacaba commented 2 years ago

Currently, according to the spec, both +0 and -0 are valid keys. So, when a value is stored with the key -0, a later call to getAllKeys should return -0 as the key. However, some implementations instead are replacing -0 with +0.

Also, given that -0 == +0, an object store or a unique index can have either -0 or +0 as a key, but not both.

The value +0 or -0 can be the whole key, or a (possibly nested) array element of the key.

I think that the spec should be clarified to note the existence of equal but different keys, and to provide examples of correct behavior (e.g. put replacing an entry with a different key).

inexorabletash commented 2 years ago

Ooof, I can't believe we overlooked this.

I'd be in favor of saying that keys should be normalized to +0 but we should survey implementations and see what they are all doing.

@abacabadabacaba do you have details about what each implementation does?

abacabadabacaba commented 2 years ago

Firefox is normalizing the keys, Chromium doesn't. I reported the Firefox behavior here as violating the spec.

asutherland commented 2 years ago

Thanks so much for the spec bug and Firefox bug with easy reproduction! Firefox was not intentionally normalizing here, bit manipulations just went wrong when trying to create a binary encoding that sorted sensibly.

That said, normalizing to +0 intentionally does seem desirable because of the uniqueness problem (because of the language around key equal to).

inexorabletash commented 2 years ago

I put together a test & normalization fix for Blink. The WPT is here:

https://chromium-review.googlesource.com/c/chromium/src/+/3700521/2/third_party/blink/web_tests/external/wpt/IndexedDB/key_negative_zero.any.js

Without a code change, as expected these are the failures:

FAIL put(v, -0) key should normalize to +0 assert_equals: key should be normalized to +0 expected 0 but got -0 FAIL put(v, +0) / getKey(v, -0) normalization assert_equals: getKey(-0) should see +0 expected 0 but got -0 FAIL put(v, -0) / getKey(v, -0) normalization assert_equals: getKey(-0) should see +0 expected 0 but got -0 FAIL put(v, -0) / getAllKeys() normalization assert_equals: key should be normalized to +0 expected 0 but got -0

Anyone want to take a look at the WPT and see if I missed any important cases? One that comes to mind is IDBKeyRange.bound(0, 10).includes(-0) and other permutations with open/closed bounds.

inexorabletash commented 2 years ago

And a spec PR at https://github.com/w3c/IndexedDB/pull/386 but maybe we want to say more. Feedback welcome!

inexorabletash commented 1 year ago

@marcoscaceres - know anyone on WebKit that would interested in reviewing the WebKit behavior and/or spec/WPTs here?

@asutherland - same question, but for Gecko?

marcoscaceres commented 1 year ago

I'll try to find someone...

szewai commented 1 year ago

WebKit does not normalize negative 0, so for the proposed test, here are the failed tests: FAIL put(v, -0) key should normalize to +0 assert_equals: key should be normalized to +0 expected 0 but got -0 FAIL put(v, -0) / getKey(v, +0) normalization assert_equals: getKey(+0) should see +0 expected 0 but got -0 FAIL put(v, -0) / getKey(v, -0) normalization assert_equals: getKey(-0) should see +0 expected 0 but got -0 FAIL put(v, -0) / getAllKeys() normalization assert_equals: key should be normalized to +0 expected 0 but got -0

Looks like we have a different case than Blink in the no-normalization implementation.

Generally I think the normalization would simplify the spec and implementation, i.e. we can always assume no -0 in keys. But with existing implementation we have, that probably means we need to convert -0 in database to 0 after the spec change. How is Blink going to deal with this? @inexorabletash

For the test, I think we can add test for extracting key with key path from value (e.g. ensure -0 is converted to 0 in key, but not in value).

inexorabletash commented 1 year ago

Apologies for the delay, I've been OOO for a bit.

Generally I think the normalization would simplify the spec and implementation, i.e. we can always assume no -0 in keys. But with existing implementation we have, that probably means we need to convert -0 in database to 0 after the spec change. How is Blink going to deal with this? @inexorabletash

@szewai - In Blink, all internal key comparisons treat -0 and 0 the same. The patch just ensures that when doing a key-to-JS conversion any -0 is mapped to +0. Whatever was previously stored in the database gets "fixed" on retrieval. This only happens to keys, not values. I can't think of a case that doesn't cover, but I may be missing something.

inexorabletash commented 1 year ago

@szewai - hey, sorry for the long delay here - any further thoughts about making this change in WebKit? What do you think of the WPT and spec PR https://github.com/w3c/IndexedDB/pull/386 ?

This is obviously pretty minor, but it'd be nice to get it off the radar if we've got consensus.

szewai commented 1 year ago

@szewai - hey, sorry for the long delay here - any further thoughts about making this change in WebKit? What do you think of the WPT and spec PR #386 ?

This is obviously pretty minor, but it'd be nice to get it off the radar if we've got consensus.

Hello! This sounds good to me. I filed https://bugs.webkit.org/show_bug.cgi?id=258053.