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

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.


> > Chrome's current behavior in this case is that the put (etc) call returns
> > without raising an error, but an error event is raised against the
> request
> > indicating that the value could not be applied. This would imply having
> the
> > algorithm below return a success/failure indicator and having the steps
> in
> > 5.1 abort if the set fails.
> >
> > Thoughts?
>
> First off, I absolutely agree that we need to write an algorithm to
> exactly define how it works when a keyPath is used to modify a value.
> There are lots of edge cases here and it doesn't surprise me that the
> different implementations have ended up doing different things.
>
> But first, there seems to be at least two misconceptions in this thread.
>
> First off, modifying a value to insert a keyPath can never run into
> the situation when a value already exists. Consider the following:
>
> store1 = db.createObjectStore("mystore1", { keyPath: "a.b",
> autoIncrement: true });
> store1.put({ a: { b: 12 }});
> store2 = db.createObjectStore("mystore2", { keyPath: "length",
> autoIncrement: true });
> store2.put([1,2,3]);
>
> The first .put call will insert an entry with key 12 since the key
> already exists. So no modification will even be attempted, i.e. we'll
> never invoke the algorithm to modify a value using a keyPath. Same
> thing in the second .put call. Here a value already exists on the
> keyPath "length" and so an entry will be inserted with key 3. Again,
> we don't need to even invoke the steps for modifying a value using a
> keyPath.
>
> Please let me know if I'm missing something
>

Nope, totally clear.


> The second issue is how to modify a value if the keyPath used for
> modifying is the empty string. This situation can no longer occur
> since the change in bug 14985 [1]. Modifying values using keyPaths
> only happen when you use autoIncrement, and you can no longer use
> autoIncrement together with an empty-string keyPath since that is
> basically useless.
>

Also clear.

>
> 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.

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.


> 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.


> 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.


> 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.


> In general I like Firefox behavior (except for the above mentioned
> blob-case bug). It means that we always figure out if we have a key
> before we send off data to the database thread and return from the
> .put/.add function. While in theory you could off-load the work to
> evaluate the keyPath to the database-thread in the case when
> autoIncrement is true, it seems somewhat inconsistent to do this since
> it can't be done when autoIncrement is false.
>
> This also means that when there is a keyPath we consistently know that
> the primary key for the value can be found at the keyPath. This is
> useful for example in the .update function since you there need to
> check that the primary key hasn't changed value.
>
> The way we do this is that we have a specialized keyPath-evaluation
> code path which returns an error if any of the values that we go
> through is not an object.
>
> But I'm certainly open to other behaviors.
>
>
> I'm curious to hear what the IE and Chrome implementations do in these
> various edge cases. It sounds like Chrome throws for some of the edge
> cases described above, for some asynchronously fire an error event,
> and for some simply don't modify the stored value, but it was a bit
> unclear to me in which situations you do what.
>

Should be detailed above, but let me know if I missed any cases.




> [1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=14985
>
> / Jonas
>