Ensure consistent JSON Serialization of DocumentKeys #1596
Conversation
schmidt-sebastian
force-pushed
the
mrschmidt-unlisten
branch
from
c4f71b1 to
d608480
Compare
packages/firestore/src/model/path.ts
Outdated
| @@ -73,6 +73,10 @@ export abstract class Path { | |||
| return this.len; | |||
| } | |||
|
|
|||
| toJSON(): string[] { | |||
| return this.segments.slice(this.offset, this.limit()); | |||
| } | |||
There was a problem hiding this comment.
Oh, wow. I had to set a breakpoint on this code and run your test to understand why this fixes it... It looks like it's because:
- RelationFilter.canonicalId() calls FieldValue.toString().
- ObjectValue.toString() uses JSON.stringify()
So while your fix works, I'm concerned it's not holistic. Using .toString() as part of our canonicalId() implementation is perhaps questionable, and using JSON.stringify() is almost certainly problematic, since 1) JSON.stringify() doesn't provide any guarantees as to order, etc. and 2) we have made no effort to implement sane toJSON() methods on any of our types. The one you fixed here may not be the only issue!
So I think at best, this fix is a workaround. If we're in a hurry to get this fixed, we could check it in but open a bug to fix this properly.
A better fix would be to port what we do on Android in ObjectValue.toString() which calls .toString() on the underlying SortedMap. This at least guarantees a consistent ordering and makes sure we are relying on the FieldValue.toString() method (as opposed to the largely undefined JSON representation). Unfortunately it looks like the JS SortedMap doesn't have .toString(), but you could either add it or else just do the traversal in ObjectValue.toString() manually.
Ultimately I think it's dangerous to be using toString() this freely in our construction of canonicalString(). It imposes a non-obvious requirement on our .toString() implementations. So a complete fix would be to add canonicalString() to FieldValue and use that instead of .toString(). But that'd need to be back-ported to the other platforms, so perhaps a project for another day.
There was a problem hiding this comment.
Oh, and note that ArrayValue will need the same fix as it relies on JSON.stringify() too.
There was a problem hiding this comment.
By the way, another approach for solving this issue would be to change RelationFilter to generate a simpler but more collision-prone canonicalID. e.g. if the FieldValue is an ObjectValue just use "[object]" as the canonicalId representation. This will generate collisions, but it avoids any chance for mismatches.
This probably isn't my top choice though.
There was a problem hiding this comment.
Hm, looks like someone really doesn't like JSON.stringify():)
What you suggested feels right to me, but some of it does feels a little out of scope for this bug fix. I did drop JSON.stringify and added a toString() on SortedMap, which is relatively safe and straightforward fix. As you pointed out, that should already a big step in the right direction. I would hope that we can leave the rest as is for now, and rely on the fact that our toString() implementations are deterministic. That ResourcePath.toString() already works as is a good indication.
schmidt-sebastian
force-pushed
the
mrschmidt-unlisten
branch
from
15f2090 to
73f3398
Compare
Fixes #1524
The DocumentKey serialization to canonical Query IDs is different once a Query is round-tripped through IndexedDb. After the round-trip, we include the project and database ID in the JSON serialized canonical ID. Since we use this ID as a Map key in SyncEngine, we need to ensure that these IDs are consistent.
This is technically a minor breaking change since it means that existing Queries that filter by DocumentReferences will no longer be read back from persistence. Instead, we will treat them as new queries and GC the old query.