Skip to content

fix(firestore): Change asserts to throw argumentError #17302

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.

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 9 commits into from
Apr 24, 2025
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,13 @@ class _JsonCollectionReference extends _JsonQuery
@override
DocumentReference<Map<String, dynamic>> doc([String? path]) {
if (path != null) {
assert(path.isNotEmpty, 'a document path must be a non-empty string');
assert(!path.contains('//'), 'a document path must not contain "//"');
assert(path != '/', 'a document path must point to a valid document');
if (path.isEmpty) {
throw ArgumentError('A document path must be a non-empty string');
} else if (path.contains('//')) {
throw ArgumentError('A document path must not contain "//"');
} else if (path == '/') {
throw ArgumentError('A document path must point to a valid document');
}
}

return _JsonDocumentReference(firestore, _delegate.doc(path));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,18 +120,15 @@ class _JsonDocumentReference

@override
CollectionReference<Map<String, dynamic>> collection(String collectionPath) {
assert(
collectionPath.isNotEmpty,
'a collectionPath path must be a non-empty string',
);
assert(
!collectionPath.contains('//'),
'a collection path must not contain "//"',
);
assert(
isValidCollectionPath(collectionPath),
'a collection path must point to a valid collection.',
);
if (collectionPath.isEmpty) {
throw ArgumentError('A collectionPath must be a non-empty string.');
} else if (collectionPath.contains('//')) {
throw ArgumentError('A collection path must not contain "//".');
} else if (!isValidCollectionPath(collectionPath)) {
throw ArgumentError(
'A collection path must point to a valid collection.',
);
}

return _JsonCollectionReference(
firestore,
Expand Down
55 changes: 23 additions & 32 deletions packages/cloud_firestore/cloud_firestore/lib/src/firestore.dart
Original file line number Diff line number Diff line change
Expand Up @@ -88,18 +88,15 @@ class FirebaseFirestore extends FirebasePluginPlatform {

/// Gets a [CollectionReference] for the specified Firestore path.
CollectionReference<Map<String, dynamic>> collection(String collectionPath) {
assert(
collectionPath.isNotEmpty,
'a collectionPath path must be a non-empty string',
);
assert(
!collectionPath.contains('//'),
'a collection path must not contain "//"',
);
assert(
isValidCollectionPath(collectionPath),
'a collection path must point to a valid collection.',
);
if (collectionPath.isEmpty) {
throw ArgumentError('A collection path must be a non-empty string.');
} else if (collectionPath.contains('//')) {
throw ArgumentError('A collection path must not contain "//".');
} else if (!isValidCollectionPath(collectionPath)) {
throw ArgumentError(
'A collection path must point to a valid collection.',
);
}

return _JsonCollectionReference(this, _delegate.collection(collectionPath));
}
Expand Down Expand Up @@ -214,14 +211,13 @@ class FirebaseFirestore extends FirebasePluginPlatform {

/// Gets a [Query] for the specified collection group.
Query<Map<String, dynamic>> collectionGroup(String collectionPath) {
assert(
collectionPath.isNotEmpty,
'a collection path must be a non-empty string',
);
assert(
!collectionPath.contains('/'),
'a collection path passed to collectionGroup() cannot contain "/"',
);
if (collectionPath.isEmpty) {
throw ArgumentError('A collection path must be a non-empty string.');
} else if (collectionPath.contains('/')) {
throw ArgumentError(
'A collection path passed to collectionGroup() cannot contain "/".',
);
}

return _JsonQuery(this, _delegate.collectionGroup(collectionPath));
}
Expand All @@ -237,18 +233,13 @@ class FirebaseFirestore extends FirebasePluginPlatform {

/// Gets a [DocumentReference] for the specified Firestore path.
DocumentReference<Map<String, dynamic>> doc(String documentPath) {
assert(
documentPath.isNotEmpty,
'a document path must be a non-empty string',
);
assert(
!documentPath.contains('//'),
'a collection path must not contain "//"',
);
assert(
isValidDocumentPath(documentPath),
'a document path must point to a valid document.',
);
if (documentPath.isEmpty) {
throw ArgumentError('A document path must be a non-empty string.');
} else if (documentPath.contains('//')) {
throw ArgumentError('A document path must not contain "//".');
} else if (!isValidDocumentPath(documentPath)) {
throw ArgumentError('A document path must point to a valid document.');
}

return _JsonDocumentReference(this, _delegate.doc(documentPath));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,12 @@ void main() {
});

test('does not expect an empty path', () {
expect(() => firestore!.collection(''), throwsAssertionError);
expect(() => firestore!.collection(''), throwsArgumentError);
});

test('does accept an invalid path', () {
// 'foo/bar' points to a document
expect(() => firestore!.collection('foo/bar'), throwsAssertionError);
expect(() => firestore!.collection('foo/bar'), throwsArgumentError);
});
});

Expand All @@ -96,13 +96,13 @@ void main() {
});

test('does not expect an empty path', () {
expect(() => firestore!.collectionGroup(''), throwsAssertionError);
expect(() => firestore!.collectionGroup(''), throwsArgumentError);
});

test('does accept a path containing "/"', () {
expect(
() => firestore!.collectionGroup('foo/bar/baz'),
throwsAssertionError,
throwsArgumentError,
);
});
});
Expand All @@ -113,12 +113,12 @@ void main() {
});

test('does not expect an empty path', () {
expect(() => firestore!.doc(''), throwsAssertionError);
expect(() => firestore!.doc(''), throwsArgumentError);
});

test('does accept an invalid path', () {
// 'foo' points to a collection
expect(() => firestore!.doc('bar'), throwsAssertionError);
expect(() => firestore!.doc('bar'), throwsArgumentError);
});
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,19 +98,19 @@ void main() {

test('path must be non-empty strings', () {
DocumentReference docRef = firestore.doc('foo/bar');
expect(() => firestore.collection(''), throwsAssertionError);
expect(() => docRef.collection(''), throwsAssertionError);
expect(() => firestore.collection(''), throwsArgumentError);
expect(() => docRef.collection(''), throwsArgumentError);
});

test('path must be odd length', () {
DocumentReference docRef = firestore.doc('foo/bar');
expect(() => firestore.collection('foo/bar'), throwsAssertionError);
expect(() => firestore.collection('foo/bar'), throwsArgumentError);
expect(
() => firestore.collection('foo/bar/baz/quu'),
throwsAssertionError,
throwsArgumentError,
);
expect(() => docRef.collection('foo/bar'), throwsAssertionError);
expect(() => docRef.collection('foo/bar/baz/quu'), throwsAssertionError);
expect(() => docRef.collection('foo/bar'), throwsArgumentError);
expect(() => docRef.collection('foo/bar/baz/quu'), throwsArgumentError);
});

test('must not have empty segments', () {
Expand All @@ -124,31 +124,31 @@ void main() {
DocumentReference docRef = colRef.doc('test-document');

for (final path in badPaths) {
expect(() => firestore.collection(path), throwsAssertionError);
expect(() => firestore.doc(path), throwsAssertionError);
expect(() => colRef.doc(path), throwsAssertionError);
expect(() => docRef.collection(path), throwsAssertionError);
expect(() => firestore.collection(path), throwsArgumentError);
expect(() => firestore.doc(path), throwsArgumentError);
expect(() => colRef.doc(path), throwsArgumentError);
expect(() => docRef.collection(path), throwsArgumentError);
}
});

group('validate', () {
test('path must be non-empty strings', () {
DocumentReference docRef = firestore.doc('foo/bar');
expect(() => firestore.collection(''), throwsAssertionError);
expect(() => docRef.collection(''), throwsAssertionError);
expect(() => firestore.collection(''), throwsArgumentError);
expect(() => docRef.collection(''), throwsArgumentError);
});

test('path must be odd length', () {
DocumentReference docRef = firestore.doc('foo/bar');
expect(() => firestore.collection('foo/bar'), throwsAssertionError);
expect(() => firestore.collection('foo/bar'), throwsArgumentError);
expect(
() => firestore.collection('foo/bar/baz/quu'),
throwsAssertionError,
throwsArgumentError,
);
expect(() => docRef.collection('foo/bar'), throwsAssertionError);
expect(() => docRef.collection('foo/bar'), throwsArgumentError);
expect(
() => docRef.collection('foo/bar/baz/quu'),
throwsAssertionError,
throwsArgumentError,
);
});

Expand All @@ -163,10 +163,10 @@ void main() {
DocumentReference docRef = colRef.doc('test-document');

for (final String path in badPaths) {
expect(() => firestore.collection(path), throwsAssertionError);
expect(() => firestore.doc(path), throwsAssertionError);
expect(() => colRef.doc(path), throwsAssertionError);
expect(() => docRef.collection(path), throwsAssertionError);
expect(() => firestore.collection(path), throwsArgumentError);
expect(() => firestore.doc(path), throwsArgumentError);
expect(() => colRef.doc(path), throwsArgumentError);
expect(() => docRef.collection(path), throwsArgumentError);
}
});
});
Expand Down
Loading