Skip to content
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

Jump to bottom

Untangle ObjectValue and ObjectValueBuilder #2970

Merged
merged 5 commits into from
Apr 23, 2020
Merged

Untangle ObjectValue and ObjectValueBuilder #2970

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a379eed
Untangle ObjectValue and ObjectValueBuilder
schmidt-sebastian Apr 22, 2020
78a75f8
Review
schmidt-sebastian Apr 23, 2020
5a64786
Cleanup
schmidt-sebastian Apr 23, 2020
5da72cd
Merge branch 'master' into mrschmidt/untangle
schmidt-sebastian Apr 23, 2020
2501720
Lint fix
schmidt-sebastian Apr 23, 2020
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
6 changes: 3 additions & 3 deletions packages/firestore/src/api/user_data_reader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import { debugAssert, fail } from '../util/assert';
import { Code, FirestoreError } from '../util/error';
import { isPlainObject, valueDescription } from '../util/input_validation';
import { Dict, forEach, isEmpty } from '../util/obj';
import { ObjectValue } from '../model/field_value';
import { ObjectValue, ObjectValueBuilder } from '../model/field_value';
import {
ArrayRemoveTransformOperation,
ArrayUnionTransformOperation,
Expand Down Expand Up @@ -390,7 +390,7 @@ export class UserDataReader {
validatePlainObject('Data must be an object, but it was:', context, input);

let fieldMaskPaths = new SortedSet<FieldPath>(FieldPath.comparator);
const updateData = ObjectValue.newBuilder();
const updateData = new ObjectValueBuilder();
forEach(input as Dict<unknown>, (key, value) => {
const path = fieldPathFromDotSeparatedString(methodName, key);

Expand Down Expand Up @@ -450,7 +450,7 @@ export class UserDataReader {
}

let fieldMaskPaths = new SortedSet<FieldPath>(FieldPath.comparator);
const updateData = ObjectValue.newBuilder();
const updateData = new ObjectValueBuilder();

for (let i = 0; i < keys.length; ++i) {
const path = keys[i];
Expand Down
3 changes: 2 additions & 1 deletion packages/firestore/src/local/local_store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ import { IndexedDbPersistence } from './indexeddb_persistence';
import { IndexedDbMutationQueue } from './indexeddb_mutation_queue';
import { IndexedDbRemoteDocumentCache } from './indexeddb_remote_document_cache';
import { IndexedDbTargetCache } from './indexeddb_target_cache';
import { extractFieldMask } from '../model/field_value';

const LOG_TAG = 'LocalStore';

Expand Down Expand Up @@ -339,7 +340,7 @@ export class LocalStore {
new PatchMutation(
mutation.key,
baseValue,
baseValue.fieldMask(),
extractFieldMask(baseValue.proto.mapValue!),
Precondition.exists(true)
)
);
Expand Down
75 changes: 32 additions & 43 deletions packages/firestore/src/model/field_value.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,8 @@ export class ObjectValue {
);
}

/** Returns a new Builder instance that is based on an empty object. */
static newBuilder(): ObjectValueBuilder {
return ObjectValue.EMPTY.toBuilder();
static empty(): ObjectValue {
return new ObjectValue({ mapValue: {} });
}

/**
Expand Down Expand Up @@ -90,48 +89,9 @@ export class ObjectValue {
}
}

/**
* Returns a FieldMask built from all FieldPaths starting from this
* ObjectValue, including paths from nested objects.
*/
fieldMask(): FieldMask {
return this.extractFieldMask(this.proto.mapValue!);
}

private extractFieldMask(value: api.MapValue): FieldMask {
let fields = new SortedSet<FieldPath>(FieldPath.comparator);
forEach(value.fields || {}, (key, value) => {
const currentPath = new FieldPath([key]);
if (typeOrder(value) === TypeOrder.ObjectValue) {
const nestedMask = this.extractFieldMask(value.mapValue!);
const nestedFields = nestedMask.fields;
if (nestedFields.isEmpty()) {
// Preserve the empty map by adding it to the FieldMask.
fields = fields.add(currentPath);
} else {
// For nested and non-empty ObjectValues, add the FieldPath of the
// leaf nodes.
nestedFields.forEach(nestedPath => {
fields = fields.add(currentPath.child(nestedPath));
});
}
} else {
// For nested and non-empty ObjectValues, add the FieldPath of the leaf
// nodes.
fields = fields.add(currentPath);
}
});
return FieldMask.fromSet(fields);
}

isEqual(other: ObjectValue): boolean {
return valueEquals(this.proto, other.proto);
}

/** Creates a ObjectValueBuilder instance that is based on the current value. */
toBuilder(): ObjectValueBuilder {
return new ObjectValueBuilder(this);
}
}

/**
Expand All @@ -152,7 +112,7 @@ export class ObjectValueBuilder {
/**
* @param baseObject The object to mutate.
*/
constructor(private readonly baseObject: ObjectValue) {}
constructor(private readonly baseObject: ObjectValue = ObjectValue.empty()) {}

/**
* Sets the field to the provided value.
Expand Down Expand Up @@ -278,3 +238,32 @@ export class ObjectValueBuilder {
return modified ? { mapValue: { fields: resultAtPath } } : null;
}
}

/**
* Returns a FieldMask built from all fields in an MapValue.
*/
export function extractFieldMask(value: api.MapValue): FieldMask {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider changing the argument of extractFieldMask() from api.MapValue to ObjectValue. With this change, the call sites become more readable. For example,

extractFieldMask(objValue.proto.mapValue!);

becomes

extractFieldMask(objValue);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was how the original call worked, but it used one level of indirection. The method is called recursively and only the first level uses an ObjectValue. I debated passing in just an api.Value, which would act as a sort of compromise. It however hides the fact that the method only works on a MapValue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh I see. Feel free to disregard this comment then. The original way you implemented it makes sense.

let fields = new SortedSet<FieldPath>(FieldPath.comparator);
forEach(value.fields || {}, (key, value) => {
const currentPath = new FieldPath([key]);
if (typeOrder(value) === TypeOrder.ObjectValue) {
const nestedMask = extractFieldMask(value.mapValue!);
const nestedFields = nestedMask.fields;
if (nestedFields.isEmpty()) {
// Preserve the empty map by adding it to the FieldMask.
fields = fields.add(currentPath);
} else {
// For nested and non-empty ObjectValues, add the FieldPath of the
// leaf nodes.
nestedFields.forEach(nestedPath => {
fields = fields.add(currentPath.child(nestedPath));
});
}
} else {
// For nested and non-empty ObjectValues, add the FieldPath of the leaf
// nodes.
fields = fields.add(currentPath);
}
});
return FieldMask.fromSet(fields);
}
8 changes: 4 additions & 4 deletions packages/firestore/src/model/mutation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,13 +485,13 @@ export class PatchMutation extends Mutation {
if (maybeDoc instanceof Document) {
data = maybeDoc.data();
} else {
data = ObjectValue.EMPTY;
data = ObjectValue.empty();
}
return this.patchObject(data);
}

private patchObject(data: ObjectValue): ObjectValue {
const builder = data.toBuilder();
const builder = new ObjectValueBuilder(data);
this.fieldMask.fields.forEach(fieldPath => {
if (!fieldPath.isEmpty()) {
const newValue = this.data.field(fieldPath);
Expand Down Expand Up @@ -598,7 +598,7 @@ export class TransformMutation extends Mutation {

if (coercedValue != null) {
if (baseObject == null) {
baseObject = ObjectValue.newBuilder().set(
baseObject = new ObjectValueBuilder().set(
fieldTransform.field,
coercedValue
);
Expand Down Expand Up @@ -726,7 +726,7 @@ export class TransformMutation extends Mutation {
'TransformResults length mismatch.'
);

const builder = data.toBuilder();
const builder = new ObjectValueBuilder(data);
for (let i = 0; i < this.fieldTransforms.length; i++) {
const fieldTransform = this.fieldTransforms[i];
const fieldPath = fieldTransform.field;
Expand Down
36 changes: 15 additions & 21 deletions packages/firestore/test/unit/model/field_value.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,12 @@
import * as api from '../../../src/protos/firestore_proto_api';

import { expect } from 'chai';
import { ObjectValue, TypeOrder } from '../../../src/model/field_value';
import {
extractFieldMask,
ObjectValue,
ObjectValueBuilder,
TypeOrder
} from '../../../src/model/field_value';
import { typeOrder } from '../../../src/model/values';
import { wrap, wrapObject, field, mask } from '../../util/helpers';

Expand Down Expand Up @@ -79,13 +84,11 @@ describe('FieldValue', () => {
});

it('can add multiple new fields', () => {
let objValue = ObjectValue.EMPTY;
objValue = objValue
.toBuilder()
let objValue = ObjectValue.empty();
objValue = new ObjectValueBuilder(objValue)
.set(field('a'), wrap('a'))
.build();
objValue = objValue
.toBuilder()
objValue = new ObjectValueBuilder(objValue)
.set(field('b'), wrap('b'))
.set(field('c'), wrap('c'))
.build();
Expand Down Expand Up @@ -154,8 +157,7 @@ describe('FieldValue', () => {
it('can delete added keys', () => {
let objValue = wrapObject({});

objValue = objValue
.toBuilder()
objValue = new ObjectValueBuilder(objValue)
.set(field('a'), wrap('a'))
.delete(field('a'))
.build();
Expand Down Expand Up @@ -189,12 +191,8 @@ describe('FieldValue', () => {
it('can delete multiple fields', () => {
let objValue = wrapObject({ a: 'a', b: 'a', c: 'c' });

objValue = objValue
.toBuilder()
.delete(field('a'))
.build();
objValue = objValue
.toBuilder()
objValue = new ObjectValueBuilder(objValue).delete(field('a')).build();
objValue = new ObjectValueBuilder(objValue)
.delete(field('b'))
.delete(field('c'))
.build();
Expand All @@ -216,7 +214,7 @@ describe('FieldValue', () => {
'map.nested.d',
'emptymap'
);
const actualMask = objValue.fieldMask();
const actualMask = extractFieldMask(objValue.proto.mapValue!);
expect(actualMask.isEqual(expectedMask)).to.be.true;
});

Expand All @@ -225,8 +223,7 @@ describe('FieldValue', () => {
fieldPath: string,
value: api.Value
): ObjectValue {
return objectValue
.toBuilder()
return new ObjectValueBuilder(objectValue)
.set(field(fieldPath), value)
.build();
}
Expand All @@ -235,10 +232,7 @@ describe('FieldValue', () => {
objectValue: ObjectValue,
fieldPath: string
): ObjectValue {
return objectValue
.toBuilder()
.delete(field(fieldPath))
.build();
return new ObjectValueBuilder(objectValue).delete(field(fieldPath)).build();
}

function assertObjectEquals(
Expand Down
12 changes: 7 additions & 5 deletions packages/firestore/test/unit/model/mutation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import {
wrap,
wrapObject
} from '../../util/helpers';
import { ObjectValueBuilder } from '../../../src/model/field_value';

describe('Mutation', () => {
addEqualityMatcher();
Expand Down Expand Up @@ -191,11 +192,12 @@ describe('Mutation', () => {
);

// Server timestamps aren't parsed, so we manually insert it.
const data = wrapObject({
foo: { bar: '<server-timestamp>' },
baz: 'baz-value'
})
.toBuilder()
const data = new ObjectValueBuilder(
wrapObject({
foo: { bar: '<server-timestamp>' },
baz: 'baz-value'
})
)
.set(field('foo.bar'), serverTimestamp(timestamp, null))
.build();
const expectedDoc = new Document(key('collection/key'), version(0), data, {
Expand Down
Loading