Re: [Bug 15434] New: [IndexedDB] Detail steps for assigning a key to a value

On Tue, Jan 24, 2012 at 8:43 AM, Joshua Bell <jsbell@chromium.org> wrote:
> On Tue, Jan 24, 2012 at 2:21 AM, Jonas Sicking <jonas@sicking.cc> wrote:
>>
>> On Mon, Jan 23, 2012 at 5:34 PM, Joshua Bell <jsbell@chromium.org> wrote:
>> > There's another edge case here - what happens on a put (etc) request to
>> > an
>> > object store with a key generator when the object store's key path does
>> > not
>> > yield a value, yet the algorithm below exits without changing the value.
>> >
>> > Sample:
>> >
>> > store = db.createObjectStore("my-store", {keyPath: "a.b", autoIncrement:
>> > true});
>> > request = store.put("value");
>> >
>> > 3.2.5 for "put" has this error case "The object store uses in-line keys
>> > and
>> > the result of evaluating the object store's key path yields a value and
>> > that
>> > value is not a valid key." resulting in a DataError.
>>
>> The intent here was for something like:
>>
>> store = db.createObjectStore("my-store", {keyPath: "a.b", autoIncrement:
>> true});
>> request = store.put({ a: { b: { hello: "world" } });
>>
>> In this case "4.7 Steps for extracting a key from a value using a key
>> path" will return the { hello: "world" } object which is not a valid
>> key and hence a DataError is thrown.
>>
>> > In this case, "4.7
>> > Steps for extracting a key from a value using a key path" says "no value
>> > is
>> > returned", so that error case doesn't apply.
>>
>> Yes, in your example that error is not applicable.
>>
>> > "5.1 Object Store Storage Operation" step 2 is: "If store uses a key
>> > generator and key is undefined, set key to the next generated key. If
>> > store
>> > also uses in-line keys, then set the property in value pointed to by
>> > store's
>> > key path to the new value for key."
>> >
>> > Per the algorithm below, the value would not change. (Another example
>> > would
>> > be a keyPath of "length" and putting [1,2,3])
>> >
>
>
> Although it's unimportant to the discussion below, I realized after the fact
> that my Array/length example was lousy since |length| is of course
> assignable.

Just to be perfectly clear and avoid any misunderstanding, the same
thing happens for non-assignable properties. For example:

store1 = db.createObjectStore("store1", { keyPath: "a.length",
autoIncrement: true});
store1.put({ a: "str" }); // stores an entry with key 3
store2 = db.createObjectStore("store2", { keyPath: "a.size",
autoIncrement: true});
store2.put({ a: my10KbBlob }); // stores an entry with key 10240

>> So, with that in mind we still need to figure out the various edge
>> cases and write a detailed set of steps for modifying a value using a
>> keyPath. In all these examples i'll assume that the key 1 is
>> generated. I've included the Firefox behavior in all cases, not
>> because I think it's obviously correct, but as a data point. I'm
>> curious to hear what you guys do too.
>>
>> What happens if a there are missing objects "higher up" in the keyPath:
>> store = db.createObjectStore("os", { keyPath: "a.b.c", autoIncrement: true
>> });
>> store.put({ x: "str" });
>> Here there is nowhere to directly store the new value since there is
>> no "a" property.
>> What we do in Firefox is to insert objects as needed. In this case
>> we'd modify the value such that we get the following:
>> { x: "str", a: { b: { c: 1 } } }
>> Same thing goes if part of the "object chain" is there:
>> store = db.createObjectStore("os", { keyPath: "a.b.c", autoIncrement: true
>> });
>> store.put({ x: "str", a: {} });
>> Here Firefox will again store { x: "str", a: { b: { c: 1 } } }
>>
>
> Per this thread/bug, I've landed a patch in Chromium to follow this
> behavior. Should be in Chrome Canary already and show up in 18.

Cool.

>> What happens if a value "higher up" in the keyPath is not an object:
>> store = db.createObjectStore("os", { keyPath: "a.b.c", autoIncrement: true
>> });
>> store.put({ a: "str" });
>> Here there not only is nowhere to directly store the new value. We
>> also can't simply insert the missing objects since we can't add a "b"
>> property to the value "str". Exact same scenario appears if you
>> replace "str" with a 1 or null.
>> What we do in Firefox is to throw a DataError exception.
>> Another example of this is simply
>> store = db.createObjectStore("os", { keyPath: "a", autoIncrement: true });
>> store.put("str");
>
> Chrome currently defers setting the new value until the transaction executes
> the asynchronous request, and thus doesn't raise an exception but fails the
> request. I agree that doing this at the time of the call makes more sense
> and is more consistent and predictable. If there's consensus here I'll file
> a bug against Chromium.

Awesome!

>> What happens if a value "higher up" in the keyPath is a host object:
>> store = db.createObjectStore("os", { keyPath: "a.b", autoIncrement: true
>> });
>> store.put({ a: myBlob });
>> While we can set a 'b' property on the blob, the structured clone
>> algorithm wouldn't copy this property and so it'd be pretty useless.
>> The exact same question applies if a is set to a Date or a RegExp too.
>> In Firefox I actually think that we set a useless 'b' property on the
>> blob which I would say is a bug.
>
> I believe Chrome does the same as Firefox here - assign a property which
> will then fail to be serialized.
>
>>
>> Whatever we decide here, we should probably be consistent with the
>> previous case of "a" being a string rather than a blob.
>
>
> ... and I agree that being consistent with the "string" case. If the value
> is not going to be serialized, we should probably fail.

Awesome.

>> What happens if a value "higher up" in the keyPath is an Array object:
>> store = db.createObjectStore("os", { keyPath: "a.b", autoIncrement: true
>> });
>> store.put({ a: [1, 2] });
>> We can simply set a 'b' property on the array since that property
>> should get copied during the structured clone. But since it's
>> non-obvious I wanted to point it out.
>> In Firefox we set a 'b' property on the array.
>
>
> Good example. I'll have to test, but I'm pretty sure Chrome will behave the
> same as Firefox here.

Sounds good.

>> There's also the somewhat ambiguous situation where a keyPath points
>> to a property which does exist, but where the property has the value
>> 'undefined':
>> store = db.createObjectStore("os", { keyPath: "a", autoIncrement: true });
>> store.put({ a: undefined });
>> I know I above said that the steps for modifying a value doesn't even
>> run if evaluating "4.7 Steps for extracting a key from a value using a
>> key path" had yielded a value. The question is if yielding 'undefined'
>> counts as yielding a value.
>> If it does count as yielding a value then the above code throws since
>> undefined isn't a valid key. I.e. we hit the "The object store uses
>> in-line keys and the result of evaluating the object store's key path
>> yields a value and that value is not a valid key." case which throws a
>> DataError.
>> In Firefox we do count it as yielding a value and so we throw an
>> exception.
>
>
> I'm in the process of landing a patch which brings Chrome's behavior on
> put/add calls in line with with the spec (hence the thread!); the current
> state of the patch give the same behavior as you describe for Firefox:
> |undefined| is considered a value, and so an exception is thrown.

I'm very much unsure what the better behavior is here. And note that
this situation also appears in the following scenario:

store = db.createObjectStore("os", { keyPath: "a.b", autoIncrement: true });
store.put({ a: undefined });

Considering undefined a value, and thus throwing, is the more
conservative choice and does let us change the behavior later if we
want to later.

Based on this (pending details from microsoft of course) I suggest the
following set of steps:

1. If /keyPath/ is the empty string, skip the remaining steps and
/value/ is not modified.
2. Let /remainingKeypath/ be /keyPath/ and /object/ be /value/.
3. if /object/ is not an Object object or an Array object (see
structured clone algorithm), then throw a DOMException of type
DataError.
4. If /remainingKeypath/ has a period in it, assign /remainingKeypath/
to be everything after the first period and assign /attribute/ to be
everything before that first period. Otherwise, go to step 8.
5. If /object/ does not have an attribute named /attribute/, then
create the attribute and assign it an empty object.
6. Assign /object/ to be the value of the attribute named /attribute/
on /object/.
7. Go to step 3.
8. NOTE: The steps leading here ensure that /remainingKeyPath/ is a
single attribute name (i.e. string without periods) by this step. They
also ensure that /object/ is an Object or an Array, and not a Date,
RegExp, Blob etc.
9. Let /attribute/ be /remainingKeyPath/
10. Set an attribute named /attribute/ on /object/ with the value /key/.

Note that the intent is that these steps are only executed if
evaluating the keyPath did not yield a value. I.e. if we know that we
need to modify the stored value. Because of this we know that at step
10 the object does not have an attribute with name /attribute/.

Let me know if these steps sound ok?

/ Jonas

Received on Tuesday, 24 January 2012 19:39:18 UTC