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.

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

Fix composite index testing build bug #7682

Merged
merged 12 commits into from
Oct 13, 2023
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -97,4 +97,5 @@ toc/
.terraform/*
.terraform.lock.hcl
*.tfstate
*.tfstate.*
*.tfstate.*
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this for new jsons generated by terraform? The one in config/ is already gitignored in line 6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a guideline for local configuring, and instructed to add "project.json" file of their target firebase project inside Firestore, where the terraform is. It makes more sense to keep everything needed in one place, instead of updating the "project.json" in config instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on a second thought, maybe there is no big difference.

*/project.json
1 change: 1 addition & 0 deletions integration/firestore/gulpfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ function copyTests() {
.src(
[
testBase + '/integration/api/*.ts',
testBase + '/integration/util/composite_index_test_helper.ts',
testBase + '/integration/util/events_accumulator.ts',
testBase + '/integration/util/helpers.ts',
testBase + '/integration/util/settings.ts',
Expand Down
1 change: 1 addition & 0 deletions packages/firestore/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ export { FieldPath as _FieldPath } from './model/path';
export type { ResourcePath as _ResourcePath } from './model/path';
export { ByteString as _ByteString } from './util/byte_string';
export { logWarn as _logWarn } from './util/log';
export { AutoId as _AutoId } from './util/misc';
export type {
AuthTokenFactory,
FirstPartyCredentialsSettings
Expand Down
6 changes: 6 additions & 0 deletions packages/firestore/src/util/misc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ export interface Indexable {
[k: string]: unknown;
}

/**
* A utility class for generating unique alphanumeric IDs of a specified length.
*
* @internal
* Exported internally for testing purposes.
*/
export class AutoId {
static newId(): string {
// Alphanumeric characters
Expand Down
18 changes: 18 additions & 0 deletions packages/firestore/test/integration/api/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,21 @@ put in test/integration/api_internal instead.
The line "import * as firebaseExport from '../util/firebase_export';" is
replaced via the gulpfile in 'integration/firestore' and should not be
modified.


## Testing composite index query against production

### Setting Up the Environment:
1. Create a `project.json` file in the `firebase-js-sdk/packages/firestore` directory. This file should contain your target Firebase project's configuration.
2. If not already logged in, authenticate with your Google Cloud Platform (GCP) account using `gcloud auth application-default login`. You can check your logged-in accounts by running `gcloud auth list`.
3. Navigate to the `firebase-js-sdk/packages/firestore` directory, run:
```
terraform init
terraform apply -var-file=project.json -auto-approve
```
Note: If the index creation encounters issues, such as concurrent operations, consider running the index creation process again. Error messages indicating that indexes have already been created can be safely disregarded.


### Adding new composite index query tests
1. To create a new composite index for local development, click on the provided link in the test error message, which will direct you to the Firebase Console.
2. Add the newly created composite index to the `firestore_index_config.tf` file. The "__name__" field is not required to be explicitly added to the file, as the index creation will auto complete it on behalf.
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,10 @@ import { apiDescribe } from '../util/helpers';
* and setting test documents and running queries with ease, ensuring proper data
* isolation and query construction.
*
* Please remember to update the main index configuration file (firestore_index_config.tf)
* with any new composite indexes needed for the tests. This ensures synchronization with
* other testing environments, including CI. You can generate the required index link by
* clicking on the Firebase console link in the error message while running tests locally.
* To get started, please refer to the instructions provided in the README file. This will guide you
* through setting up your local testing environment and updating the Terraform configuration with
* any new composite indexes required for your testing scenarios.
*/

apiDescribe('Composite Index Queries', persistence => {
// OR Query tests only run when the SDK's local cache is configured to use
// LRU garbage collection (rather than eager garbage collection) because
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,6 @@
* limitations under the License.
*/

import { and } from '../../../src/lite-api/query';
import { AutoId } from '../../../src/util/misc';
import { field } from '../../util/helpers';

import {
query as internalQuery,
CollectionReference,
Expand All @@ -41,7 +37,10 @@ import {
getDocs as getDocuments,
QuerySnapshot,
deleteDoc as deleteDocument,
doc
doc,
and,
_AutoId,
_FieldPath
} from './firebase_export';
import {
batchCommitDocsToCollection,
Expand Down Expand Up @@ -72,7 +71,7 @@ export class CompositeIndexTestHelper {
// Creates a new instance of the CompositeIndexTestHelper class, with a unique test
// identifier for data isolation.
constructor() {
this.testId = 'test-id-' + AutoId.newId();
this.testId = 'test-id-' + _AutoId.newId();
}

// Runs a test with specified documents in the COMPOSITE_INDEX_TEST_COLLECTION.
Expand Down Expand Up @@ -113,8 +112,8 @@ export class CompositeIndexTestHelper {

// Remove test-specific fields from a document, including the testId and expiration date.
private removeTestSpecificFieldsFromDoc(doc: DocumentData): void {
doc._document?.data?.delete(field(this.TTL_FIELD));
doc._document?.data?.delete(field(this.TEST_ID_FIELD));
doc._document?.data?.delete(new _FieldPath([this.TEST_ID_FIELD]));
doc._document?.data?.delete(new _FieldPath([this.TEST_ID_FIELD]));
}

// Helper method to hash document keys and add test-specific fields for the provided documents.
Expand Down
7 changes: 3 additions & 4 deletions packages/firestore/test/integration/util/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
import { isIndexedDBAvailable } from '@firebase/util';
import { expect } from 'chai';

import { AutoId } from '../../../src/util/misc';

import {
clearIndexedDbPersistence,
collection,
Expand All @@ -45,7 +43,8 @@ import {
writeBatch,
Query,
getDocsFromServer,
getDocsFromCache
getDocsFromCache,
_AutoId
} from './firebase_export';
import {
ALT_PROJECT_ID,
Expand Down Expand Up @@ -448,7 +447,7 @@ export function withTestCollectionSettings<T>(
docs: { [key: string]: DocumentData },
fn: (collection: CollectionReference, db: Firestore) => Promise<T>
): Promise<T> {
const collectionId = AutoId.newId();
const collectionId = _AutoId.newId();
return batchCommitDocsToCollection(
persistence,
settings,
Expand Down
Loading