Skip to content

Ensure consistent JSON Serialization of DocumentKeys #1596

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 13, 2019
Merged

Ensure consistent JSON Serialization of DocumentKeys #1596

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d608480
Ensure consistent JSON Serialization of DocumentKeys
schmidt-sebastian Mar 12, 2019
a92a5c8
Add Changelog entry
schmidt-sebastian Mar 12, 2019
73f3398
Remove JSON.stringify
schmidt-sebastian Mar 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/firestore/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
While this feature is not yet available, all schema changes are included
in this release. Once you upgrade, you will not be able to use an older version
of the Firestore SDK with persistence enabled.
- [fixed] Fixed an issue with IndexedDb persistence that triggered an internal
assert for Queries that use nested DocumentReferences in where() clauses
(#1524, #1596).

# 1.0.4
- [fixed] Fixed an uncaught promise error occurring when `enablePersistence()`
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/src/core/sync_engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {

const queryView = this.queryViewsByQuery.get(query);
if (queryView) {
// PORTING NOTE: With Mult-Tab Web, it is possible that a query view
// PORTING NOTE: With Multi-Tab Web, it is possible that a query view
// already exists when EventManager calls us for the first time. This
// happens when the primary tab is already listening to this query on
// behalf of another tab and the user of the primary also starts listening
Expand Down
4 changes: 4 additions & 0 deletions packages/firestore/src/model/path.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ export abstract class Path {
return this.len;
}

toJSON(): string[] {
return this.segments.slice(this.offset, this.limit());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. RelationFilter.canonicalId() calls FieldValue.toString().
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and note that ArrayValue will need the same fix as it relies on JSON.stringify() too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


isEqual(other: Path): boolean {
return Path.comparator(this, other) === 0;
}
Expand Down
35 changes: 34 additions & 1 deletion packages/firestore/test/integration/api/query.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,10 @@ import {
withTestDb
} from '../util/helpers';

const Timestamp = firebase.firestore!.Timestamp;
const Blob = firebase.firestore!.Blob;
const FieldPath = firebase.firestore!.FieldPath;
const GeoPoint = firebase.firestore!.GeoPoint;
const Timestamp = firebase.firestore!.Timestamp;

// TODO(b/116617988): Use public API.
interface FirestoreInternal extends firestore.FirebaseFirestore {
Expand Down Expand Up @@ -694,4 +696,35 @@ apiDescribe('Queries', persistence => {
expect(querySnapshot.docs.map(d => d.id)).to.deep.equal(['cg-doc2']);
});
});

it('can query custom types', () => {
return withTestCollection(persistence, {}, async ref => {
const data = {
ref: ref.firestore.doc('f/c'),
geoPoint: new GeoPoint(0, 0),
buffer: Blob.fromBase64String('Zm9v'),
time: Timestamp.now()
};
await ref.add({ data });

// In https://github.com/firebase/firebase-js-sdk/issues/1524, a
// customer was not able to unlisten from a query that contained a
// nested object with a DocumentReference. The cause of it was that our
// serialization of nested references via JSON.stringify() was different
// for Queries created via the API layer versus Queries read from
// persistence. To simulate this issue, we have to listen and unlisten
// to the same query twice.
const query = ref.where('data', '==', data);

for (let i = 0; i < 2; ++i) {
const deferred = new Deferred();
const unsubscribe = query.onSnapshot(snapshot => {
expect(snapshot.size).to.equal(1);
deferred.resolve();
});
await deferred.promise;
unsubscribe();
}
});
});
});
6 changes: 6 additions & 0 deletions packages/firestore/test/unit/model/path.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,12 @@ describe('Path', () => {
expect(abc.isPrefixOf(ba)).to.equal(false);
});

it('respects offset during JSON serialization', () => {
const path1 = new ResourcePath(['c']);
const path2 = new ResourcePath(['a', 'b', 'c'], 2);
expect(JSON.stringify(path1)).to.equal(JSON.stringify(path2));
});

it('can be constructed from field path.', () => {
const path = FieldPath.fromServerFormat('foo\\..bar\\\\.baz');
expect(path.toArray()).to.deep.equal(['foo.', 'bar\\', 'baz']);
Expand Down