Skip to content

Remove "cyclic" references #2969

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
Apr 23, 2020
Merged

Remove "cyclic" references #2969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
71bdf7e
Remove cyclic references
schmidt-sebastian Apr 22, 2020
ba13820
Validation
schmidt-sebastian Apr 23, 2020
e447326
Cleanup
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
50 changes: 23 additions & 27 deletions packages/firestore/src/api/database.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ import {
validateStringEnum,
valueDescription
} from '../util/input_validation';
import { logError, setLogLevel, LogLevel, getLogLevel } from '../util/log';
import { getLogLevel, logError, LogLevel, setLogLevel } from '../util/log';
import { AutoId } from '../util/misc';
import { Deferred, Rejecter, Resolver } from '../util/promise';
import { FieldPath as ExternalFieldPath } from './field_path';
Expand Down Expand Up @@ -836,7 +836,7 @@ export class WriteBatch implements firestore.WriteBatch {
convertedValue
);
this._mutations = this._mutations.concat(
parsed.toMutations(ref._key, Precondition.NONE)
parsed.toMutations(ref._key, Precondition.none())
);
return this;
}
Expand Down Expand Up @@ -906,7 +906,7 @@ export class WriteBatch implements firestore.WriteBatch {
this._firestore
);
this._mutations = this._mutations.concat(
new DeleteMutation(ref._key, Precondition.NONE)
new DeleteMutation(ref._key, Precondition.none())
);
return this;
}
Expand Down Expand Up @@ -1031,7 +1031,7 @@ export class DocumentReference<T = firestore.DocumentData>
)
: this.firestore._dataReader.parseSetData(functionName, convertedValue);
return this._firestoreClient.write(
parsed.toMutations(this._key, Precondition.NONE)
parsed.toMutations(this._key, Precondition.none())
);
}

Expand Down Expand Up @@ -1075,7 +1075,7 @@ export class DocumentReference<T = firestore.DocumentData>
delete(): Promise<void> {
validateExactNumberOfArgs('DocumentReference.delete', arguments, 0);
return this._firestoreClient.write([
new DeleteMutation(this._key, Precondition.NONE)
new DeleteMutation(this._key, Precondition.none())
]);
}

Expand Down Expand Up @@ -1447,32 +1447,31 @@ export class Query<T = firestore.DocumentData> implements firestore.Query<T> {

// Enumerated from the WhereFilterOp type in index.d.ts.
const whereFilterOpEnums = [
'<',
'<=',
'==',
'>=',
'>',
'array-contains',
'in',
'array-contains-any'
Operator.LESS_THAN,
Operator.LESS_THAN_OR_EQUAL,
Operator.EQUAL,
Operator.GREATER_THAN_OR_EQUAL,
Operator.GREATER_THAN,
Operator.ARRAY_CONTAINS,
Operator.IN,
Operator.ARRAY_CONTAINS_ANY
];
validateStringEnum('Query.where', whereFilterOpEnums, 2, opStr);
const op = validateStringEnum('Query.where', whereFilterOpEnums, 2, opStr);

let fieldValue: api.Value;
const fieldPath = fieldPathFromArgument('Query.where', field);
const operator = Operator.fromString(opStr);
if (fieldPath.isKeyField()) {
if (
operator === Operator.ARRAY_CONTAINS ||
operator === Operator.ARRAY_CONTAINS_ANY
op === Operator.ARRAY_CONTAINS ||
op === Operator.ARRAY_CONTAINS_ANY
) {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
`Invalid Query. You can't perform '${operator.toString()}' ` +
`Invalid Query. You can't perform '${op}' ` +
'queries on FieldPath.documentId().'
);
} else if (operator === Operator.IN) {
this.validateDisjunctiveFilterElements(value, operator);
} else if (op === Operator.IN) {
this.validateDisjunctiveFilterElements(value, op);
const referenceList: api.Value[] = [];
for (const arrayValue of value as api.Value[]) {
referenceList.push(this.parseDocumentIdValue(arrayValue));
Expand All @@ -1482,20 +1481,17 @@ export class Query<T = firestore.DocumentData> implements firestore.Query<T> {
fieldValue = this.parseDocumentIdValue(value);
}
} else {
if (
operator === Operator.IN ||
operator === Operator.ARRAY_CONTAINS_ANY
) {
this.validateDisjunctiveFilterElements(value, operator);
if (op === Operator.IN || op === Operator.ARRAY_CONTAINS_ANY) {
this.validateDisjunctiveFilterElements(value, op);
}
fieldValue = this.firestore._dataReader.parseQueryValue(
'Query.where',
value,
// We only allow nested arrays for IN queries.
/** allowArrays = */ operator === Operator.IN ? true : false
/** allowArrays = */ op === Operator.IN
);
}
const filter = FieldFilter.create(fieldPath, operator, fieldValue);
const filter = FieldFilter.create(fieldPath, op, fieldValue);
this.validateNewFilter(filter);
return new Query(
this._query.addFilter(filter),
Expand Down
90 changes: 21 additions & 69 deletions packages/firestore/src/core/query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,28 +80,27 @@ export class Query {

get orderBy(): OrderBy[] {
if (this.memoizedOrderBy === null) {
this.memoizedOrderBy = [];

const inequalityField = this.getInequalityFilterField();
const firstOrderByField = this.getFirstOrderByField();
if (inequalityField !== null && firstOrderByField === null) {
// In order to implicitly add key ordering, we must also add the
// inequality filter field for it to be a valid query.
// Note that the default inequality field and key ordering is ascending.
if (inequalityField.isKeyField()) {
this.memoizedOrderBy = [KEY_ORDERING_ASC];
} else {
this.memoizedOrderBy = [
new OrderBy(inequalityField),
KEY_ORDERING_ASC
];
if (!inequalityField.isKeyField()) {
this.memoizedOrderBy.push(new OrderBy(inequalityField));
}
this.memoizedOrderBy.push(
new OrderBy(FieldPath.keyField(), Direction.ASCENDING)
);
} else {
debugAssert(
inequalityField === null ||
(firstOrderByField !== null &&
inequalityField.isEqual(firstOrderByField)),
'First orderBy should match inequality field.'
);
this.memoizedOrderBy = [];
let foundKeyOrdering = false;
for (const orderBy of this.explicitOrderBy) {
this.memoizedOrderBy.push(orderBy);
Expand All @@ -117,9 +116,7 @@ export class Query {
? this.explicitOrderBy[this.explicitOrderBy.length - 1].dir
: Direction.ASCENDING;
this.memoizedOrderBy.push(
lastDirection === Direction.ASCENDING
? KEY_ORDERING_ASC
: KEY_ORDERING_DESC
new OrderBy(FieldPath.keyField(), lastDirection)
);
}
}
Expand Down Expand Up @@ -468,48 +465,15 @@ export abstract class Filter {
abstract isEqual(filter: Filter): boolean;
}

export class Operator {
static LESS_THAN = new Operator('<');
static LESS_THAN_OR_EQUAL = new Operator('<=');
static EQUAL = new Operator('==');
static GREATER_THAN = new Operator('>');
static GREATER_THAN_OR_EQUAL = new Operator('>=');
static ARRAY_CONTAINS = new Operator('array-contains');
static IN = new Operator('in');
static ARRAY_CONTAINS_ANY = new Operator('array-contains-any');

static fromString(op: string): Operator {
switch (op) {
case '<':
return Operator.LESS_THAN;
case '<=':
return Operator.LESS_THAN_OR_EQUAL;
case '==':
return Operator.EQUAL;
case '>=':
return Operator.GREATER_THAN_OR_EQUAL;
case '>':
return Operator.GREATER_THAN;
case 'array-contains':
return Operator.ARRAY_CONTAINS;
case 'in':
return Operator.IN;
case 'array-contains-any':
return Operator.ARRAY_CONTAINS_ANY;
default:
return fail('Unknown FieldFilter operator: ' + op);
}
}

constructor(public name: string) {}

toString(): string {
return this.name;
}

isEqual(other: Operator): boolean {
return this.name === other.name;
}
export const enum Operator {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a function to convert from string to Operator in lieu of using the as operator at call sites. This function should invoke fail() if conversion fails. This would restore parity with the old code since the following logic from the deleted fromString() method has been lost:

default:
  return fail('Unknown FieldFilter operator: ' + op);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we are using "const enums" the value of the operator is actually a string in the code. Hence, there is no conversion. We already validate that the input is one of the specific strings before we convert to the Operator type (via validateStringEnum). I changed validateStringEnum to do the casting on behalf of the caller, which means that the validation and the conversion now happens in the same place.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks great!

LESS_THAN = '<',
LESS_THAN_OR_EQUAL = '<=',
EQUAL = '==',
GREATER_THAN = '>',
GREATER_THAN_OR_EQUAL = '>=',
ARRAY_CONTAINS = 'array-contains',
IN = 'in',
ARRAY_CONTAINS_ANY = 'array-contains-any'
}

export class FieldFilter extends Filter {
Expand Down Expand Up @@ -635,7 +599,7 @@ export class FieldFilter extends Filter {
isEqual(other: Filter): boolean {
if (other instanceof FieldFilter) {
return (
this.op.isEqual(other.op) &&
this.op === other.op &&
this.field.isEqual(other.field) &&
valueEquals(this.value, other.value)
);
Expand Down Expand Up @@ -737,15 +701,9 @@ export class ArrayContainsAnyFilter extends FieldFilter {
/**
* The direction of sorting in an order by.
*/
export class Direction {
static ASCENDING = new Direction('asc');
static DESCENDING = new Direction('desc');

private constructor(public name: string) {}

toString(): string {
return this.name;
}
export const enum Direction {
ASCENDING = 'asc',
DESCENDING = 'desc'
}

/**
Expand Down Expand Up @@ -875,9 +833,3 @@ export class OrderBy {
return this.dir === other.dir && this.field.isEqual(other.field);
}
}

const KEY_ORDERING_ASC = new OrderBy(FieldPath.keyField(), Direction.ASCENDING);
const KEY_ORDERING_DESC = new OrderBy(
FieldPath.keyField(),
Direction.DESCENDING
);
6 changes: 2 additions & 4 deletions packages/firestore/src/core/snapshot_version.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,12 @@ import { Timestamp } from '../api/timestamp';
* timestamp, such as update_time or read_time.
*/
export class SnapshotVersion {
static readonly MIN = new SnapshotVersion(new Timestamp(0, 0));

static fromTimestamp(value: Timestamp): SnapshotVersion {
return new SnapshotVersion(value);
}

static forDeletedDoc(): SnapshotVersion {
return SnapshotVersion.MIN;
static min(): SnapshotVersion {
return new SnapshotVersion(new Timestamp(0, 0));
}

private constructor(private timestamp: Timestamp) {}
Expand Down
8 changes: 4 additions & 4 deletions packages/firestore/src/core/sync_engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import {
import { MaybeDocument, NoDocument } from '../model/document';
import { DocumentKey } from '../model/document_key';
import { Mutation } from '../model/mutation';
import { MutationBatchResult, BATCHID_UNKNOWN } from '../model/mutation_batch';
import { BATCHID_UNKNOWN, MutationBatchResult } from '../model/mutation_batch';
import { RemoteEvent, TargetChange } from '../remote/remote_event';
import { RemoteStore } from '../remote/remote_store';
import { RemoteSyncer } from '../remote/remote_syncer';
Expand All @@ -52,7 +52,7 @@ import {
} from '../local/shared_client_state_syncer';
import { SortedSet } from '../util/sorted_set';
import { ListenSequence } from './listen_sequence';
import { Query, LimitType } from './query';
import { LimitType, Query } from './query';
import { SnapshotVersion } from './snapshot_version';
import { Target } from './target';
import { TargetIdGenerator } from './target_id_generator';
Expand Down Expand Up @@ -504,11 +504,11 @@ export class SyncEngine implements RemoteSyncer {
);
documentUpdates = documentUpdates.insert(
limboKey,
new NoDocument(limboKey, SnapshotVersion.forDeletedDoc())
new NoDocument(limboKey, SnapshotVersion.min())
);
const resolvedLimboDocuments = documentKeySet().add(limboKey);
const event = new RemoteEvent(
SnapshotVersion.MIN,
SnapshotVersion.min(),
/* targetChanges= */ new Map<TargetId, TargetChange>(),
/* targetMismatches= */ new SortedSet<TargetId>(primitiveComparator),
documentUpdates,
Expand Down
10 changes: 5 additions & 5 deletions packages/firestore/src/core/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

import { ParsedSetData, ParsedUpdateData } from '../api/user_data_reader';
import { documentVersionMap } from '../model/collections';
import { Document, NoDocument, MaybeDocument } from '../model/document';
import { Document, MaybeDocument, NoDocument } from '../model/document';

import { DocumentKey } from '../model/document_key';
import {
Expand All @@ -27,7 +27,7 @@ import {
VerifyMutation
} from '../model/mutation';
import { Datastore } from '../remote/datastore';
import { fail, debugAssert } from '../util/assert';
import { debugAssert, fail } from '../util/assert';
import { Code, FirestoreError } from '../util/error';
import { SnapshotVersion } from './snapshot_version';

Expand Down Expand Up @@ -123,7 +123,7 @@ export class Transaction {
docVersion = doc.version;
} else if (doc instanceof NoDocument) {
// For deleted docs, we must use baseVersion 0 when we overwrite them.
docVersion = SnapshotVersion.forDeletedDoc();
docVersion = SnapshotVersion.min();
} else {
throw fail('Document in a transaction was a ' + doc.constructor.name);
}
Expand Down Expand Up @@ -151,7 +151,7 @@ export class Transaction {
if (!this.writtenDocs.has(key) && version) {
return Precondition.updateTime(version);
} else {
return Precondition.NONE;
return Precondition.none();
}
}

Expand All @@ -163,7 +163,7 @@ export class Transaction {
// The first time a document is written, we want to take into account the
// read time and existence
if (!this.writtenDocs.has(key) && version) {
if (version.isEqual(SnapshotVersion.forDeletedDoc())) {
if (version.isEqual(SnapshotVersion.min())) {
// The document doesn't exist, so fail the transaction.

// This has to be validated locally because you can't send a
Expand Down
4 changes: 2 additions & 2 deletions packages/firestore/src/local/index_free_query_engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export class IndexFreeQueryEngine implements QueryEngine {

// Queries that have never seen a snapshot without limbo free documents
// should also be run as a full collection scan.
if (lastLimboFreeSnapshotVersion.isEqual(SnapshotVersion.MIN)) {
if (lastLimboFreeSnapshotVersion.isEqual(SnapshotVersion.min())) {
return this.executeFullCollectionScan(transaction, query);
}

Expand Down Expand Up @@ -204,7 +204,7 @@ export class IndexFreeQueryEngine implements QueryEngine {
return this.localDocumentsView!.getDocumentsMatchingQuery(
transaction,
query,
SnapshotVersion.MIN
SnapshotVersion.min()
);
}
}
Loading