Skip to content

Commit

Permalink
Slightly better error messages (#451)
Browse files Browse the repository at this point in the history
  • Loading branch information
@schmidt-sebastian
schmidt-sebastian authored Nov 13, 2018
1 parent 0fac51b commit 4657723
Show file tree
Hide file tree
Showing 17 changed files with 276 additions and 214 deletions.
5 changes: 2 additions & 3 deletions dev/src/document.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import {AnyDuringMigration, AnyJs, ApiMapValue, DocumentData, UpdateData, UserIn

import api = google.firestore.v1beta1;


/**
* Returns a builder for DocumentSnapshot and QueryDocumentSnapshot instances.
* Invoke `.build()' to assemble the final snapshot.
Expand Down Expand Up @@ -785,8 +784,8 @@ export class DocumentMask {
const result = applyDocumentMask(data);

if (result.remainingPaths.length !== 0) {
throw new Error(`Input data is missing for field '${
result.remainingPaths[0].toString()}'.`);
throw new Error(`Input data is missing for field "${
result.remainingPaths[0].toString()}".`);
}

return result.filteredData;
Expand Down
64 changes: 41 additions & 23 deletions dev/src/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,8 +289,9 @@ export class Firestore {
*/
constructor(settings?: Settings) {
this._validator = new Validator({
ArrayElement: (name, value) =>
validateFieldValue(name, value, /* depth */ 0, /*inArray=*/true),
ArrayElement: (name, value) => validateFieldValue(
name, value, /*path=*/undefined, /*level=*/0,
/*inArray=*/true),
DeletePrecondition: precondition =>
validatePrecondition(precondition, /* allowExists= */ true),
Document: validateDocumentData,
Expand Down Expand Up @@ -511,8 +512,8 @@ export class Firestore {
convertDocument = convert.documentFromJson;
} else {
throw new Error(
`Unsupported encoding format. Expected 'json' or 'protobufJS', ` +
`but was '${encoding}'.`);
`Unsupported encoding format. Expected "json" or "protobufJS", ` +
`but was "${encoding}".`);
}

const document = new DocumentSnapshotBuilder();
Expand Down Expand Up @@ -1261,51 +1262,67 @@ follow these steps, YOUR APP MAY BREAK.`);
*
* @private
* @param val JavaScript value to validate.
* @param path The field path to validate.
* @param options Validation options
* @param depth The current depth of the traversal.
* @param level The current depth of the traversal. This is used to decide
* whether deletes are allowed in conjunction with `allowDeletes: root`.
* @param inArray Whether we are inside an array.
* @returns 'true' when the object is valid.
* @throws when the object is invalid.
*/
function validateFieldValue(
val: AnyJs, options: ValidationOptions, depth?: number,
val: AnyJs, options: ValidationOptions, path?: FieldPath, level?: number,
inArray?: boolean): boolean {
if (!depth) {
depth = 1;
} else if (depth > MAX_DEPTH) {
if (path && path.size > MAX_DEPTH) {
throw new Error(
`Input object is deeper than ${MAX_DEPTH} levels or contains a cycle.`);
}

level = level || 0;
inArray = inArray || false;

if (is.array(val)) {
const fieldPathMessage = path ? ` (found in field ${path.toString()})` : '';

if (Array.isArray(val)) {
const arr = val as AnyDuringMigration[];
for (const prop of arr) {
validateFieldValue(arr[prop]!, options, depth! + 1, /* inArray= */ true);
for (let i = 0; i < arr.length; ++i) {
validateFieldValue(
arr[i]!, options,
path ? path.append(String(i)) : new FieldPath(String(i)), level + 1,
/* inArray= */ true);
}
} else if (isPlainObject(val)) {
const obj = val as object;
for (const prop in obj) {
if (obj.hasOwnProperty(prop)) {
validateFieldValue(obj[prop]!, options, depth! + 1, inArray);
validateFieldValue(
obj[prop]!, options,
path ? path.append(new FieldPath(prop)) : new FieldPath(prop),
level + 1, inArray);
}
}
} else if (val === undefined) {
throw new Error(
`Cannot use "undefined" as a Firestore value${fieldPathMessage}.`);
} else if (val instanceof DeleteTransform) {
if (inArray) {
throw new Error(`${val.methodName}() cannot be used inside of an array.`);
throw new Error(`${val.methodName}() cannot be used inside of an array${
fieldPathMessage}.`);
} else if (
(options.allowDeletes === 'root' && depth > 1) ||
(options.allowDeletes === 'root' && level !== 0) ||
options.allowDeletes === 'none') {
throw new Error(`${
val.methodName}() must appear at the top-level and can only be used in update() or set() with {merge:true}.`);
val.methodName}() must appear at the top-level and can only be used in update() or set() with {merge:true}${
fieldPathMessage}.`);
}
} else if (val instanceof FieldTransform) {
if (inArray) {
throw new Error(`${val.methodName}() cannot be used inside of an array.`);
throw new Error(`${val.methodName}() cannot be used inside of an array${
fieldPathMessage}.`);
} else if (!options.allowTransforms) {
throw new Error(`${
val.methodName}() can only be used in set(), create() or update().`);
throw new Error(
`${val.methodName}() can only be used in set(), create() or update()${
fieldPathMessage}.`);
}
} else if (val instanceof DocumentReference) {
return true;
Expand All @@ -1315,9 +1332,10 @@ function validateFieldValue(
return true;
} else if (val instanceof FieldPath) {
throw new Error(
'Cannot use object of type "FieldPath" as a Firestore value.');
`Cannot use object of type "FieldPath" as a Firestore value${
fieldPathMessage}.`);
} else if (is.object(val)) {
throw customObjectError(val);
throw customObjectError(val, path);
}

return true;
Expand All @@ -1335,7 +1353,7 @@ function validateFieldValue(
function validateDocumentData(
obj: DocumentData, options: ValidationOptions): boolean {
if (!isPlainObject(obj)) {
throw new Error('Input is not a plain JavaScript object.');
throw customObjectError(obj);
}

options = options || {};
Expand All @@ -1345,7 +1363,7 @@ function validateDocumentData(
for (const prop in obj) {
if (obj.hasOwnProperty(prop)) {
isEmpty = false;
validateFieldValue(obj[prop], options, /* depth= */ 1);
validateFieldValue(obj[prop], options, new FieldPath(prop));
}
}

Expand Down
58 changes: 36 additions & 22 deletions dev/src/path.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,20 +65,28 @@ abstract class Path<T> {
*
* @private
* @hideconstructor
* @param {string[]} segments Sequence of parts of a path.
* @param segments Sequence of parts of a path.
*/
constructor(protected readonly segments: string[]) {}

/**
* String representation as expected by the proto API.
*
* @private
* @type {string}
*/
get formattedName(): string {
return this.canonicalString()!;
}

/**
* Returns the number of segments of this field path.
*
* @private
*/
get size(): number {
return this.segments.length;
}

abstract construct(segments: string[]|string): T;
abstract canonicalString(): string;
abstract split(relativePath: string): string[];
Expand All @@ -87,9 +95,8 @@ abstract class Path<T> {
* Create a child path beneath the current level.
*
* @private
* @param {string|T} relativePath Relative path to append to the current path.
* @returns {T} The new path.
* @template T
* @param relativePath Relative path to append to the current path.
* @returns The new path.
*/
append(relativePath: Path<T>|string): T {
if (relativePath instanceof Path) {
Expand All @@ -102,9 +109,7 @@ abstract class Path<T> {
* Returns the path of the parent node.
*
* @private
* @returns {T|null} The new path or null if we are already at the root.
* @returns {T} The new path.
* @template T
* @returns The new path or null if we are already at the root.
*/
parent(): T|null {
if (this.segments.length === 0) {
Expand All @@ -119,8 +124,7 @@ abstract class Path<T> {
*
* @private
* @param other The path to check against.
* @returns 'true' iff the current path is a prefix match with
* 'other'.
* @returns 'true' iff the current path is a prefix match with 'other'.
*/
isPrefixOf(other: Path<T>): boolean {
if (other.segments.length < this.segments.length) {
Expand All @@ -140,7 +144,7 @@ abstract class Path<T> {
* Returns a string representation of this path.
*
* @private
* @returns {string} A string representing this path.
* @returns A string representing this path.
*/
toString(): string {
return this.formattedName;
Expand Down Expand Up @@ -176,7 +180,7 @@ abstract class Path<T> {
* Returns a copy of the underlying segments.
*
* @private
* @returns {Array.<string>} A copy of the segments that make up this path.
* @returns A copy of the segments that make up this path.
*/
toArray(): string[] {
return this.segments.slice();
Expand All @@ -186,8 +190,8 @@ abstract class Path<T> {
* Returns true if this `Path` is equal to the provided value.
*
* @private
* @param {*} other The value to compare against.
* @return {boolean} true if this `Path` is equal to the provided value.
* @param other The value to compare against.
* @return true if this `Path` is equal to the provided value.
*/
isEqual(other: Path<T>): boolean {
return (
Expand Down Expand Up @@ -450,7 +454,7 @@ export class FieldPath extends Path<FieldPath> {
for (let i = 0; i < elements.length; ++i) {
validate.isString(i, elements[i]);
if (elements[i].length === 0) {
throw new Error(`Argument at index ${i} should not be empty.`);
throw new Error(`Element at index ${i} should not be an empty string.`);
}
}

Expand All @@ -471,26 +475,36 @@ export class FieldPath extends Path<FieldPath> {
* Returns true if the provided value can be used as a field path argument.
*
* @private
* @param {string|FieldPath} fieldPath The value to verify.
* @param fieldPath The value to verify.
* @throws if the string can't be used as a field path.
* @returns {boolean} 'true' when the path is valid.
* @returns 'true' when the path is valid.
*/
static validateFieldPath(fieldPath: string|FieldPath) {
static validateFieldPath(fieldPath: unknown): boolean {
if (!(fieldPath instanceof FieldPath)) {
if (!is.string(fieldPath)) {
if (fieldPath === undefined) {
throw new Error('Path cannot be omitted.');
}

if (is.object(fieldPath) &&
(fieldPath as object).constructor.name === 'FieldPath') {
throw customObjectError(fieldPath);
}

if (typeof fieldPath !== 'string') {
throw new Error(
'Paths can only be specified as strings or via a FieldPath object.');
}

if (fieldPath.indexOf('..') >= 0) {
throw new Error(`Paths must not contain '..' in them.`);
throw new Error(`Paths must not contain ".." in them.`);
}

if (fieldPath.startsWith('.') || fieldPath.endsWith('.')) {
throw new Error(`Paths must not start or end with '.'.`);
throw new Error(`Paths must not start or end with ".".`);
}

if (!FIELD_PATH_RE.test(fieldPath)) {
throw new Error(`Paths can't be empty and must not contain '*~/[]'.`);
throw new Error(`Paths can't be empty and must not contain "*~/[]".`);
}
}

Expand Down
12 changes: 6 additions & 6 deletions dev/src/reference.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -940,11 +940,11 @@ export class Query {
const fieldValue = documentSnapshot.get(fieldOrder.field);
if (fieldValue === undefined) {
throw new Error(
`Field '${
`Field "${
fieldOrder
.field}' is missing in the provided DocumentSnapshot. Please provide a ` +
'document that contains values for all specified orderBy() and ' +
'where() constraints.');
.field}" is missing in the provided DocumentSnapshot. ` +
'Please provide a document that contains values for all specified ' +
'orderBy() and where() constraints.');
} else {
fieldValues.push(fieldValue);
}
Expand Down Expand Up @@ -1319,7 +1319,7 @@ export class Query {
} else if (reference instanceof DocumentReference) {
if (!this._path.isPrefixOf(reference._path)) {
throw new Error(
`'${reference.path}' is not part of the query result set and ` +
`"${reference.path}" is not part of the query result set and ` +
'cannot be used as a query boundary.');
}
} else {
Expand All @@ -1331,7 +1331,7 @@ export class Query {
if (reference._path.parent()!.compareTo(this._path) !== 0) {
throw new Error(
'Only a direct child can be used as a query boundary. ' +
`Found: '${reference.path}'.`);
`Found: "${reference.path}".`);
}
return reference;
}
Expand Down
Loading

0 comments on commit 4657723

Please sign in to comment.