Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
- Add support for `dataAccessMode` in Firestore database creation. This allows choosing between `FIRESTORE_NATIVE` and `MONGODB_COMPATIBLE` for Enterprise edition databases.
- Updated Firestore Emulator to v1.20.4, which includes minor bug fixes for Firestore Native Mode.
- Added `apptesting:execute` command to run App Testing agent tests from YAML files.
- Updated Data Connect emulator to v3.3.0:
Expand Down
7 changes: 7 additions & 0 deletions schema/firebase-config.json
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,13 @@
"FirestoreSingle": {
"additionalProperties": false,
"properties": {
"dataAccessMode": {
"enum": [
"FIRESTORE_NATIVE",
"MONGODB_COMPATIBLE"
],
"type": "string"
},
"database": {
"type": "string"
},
Expand Down
160 changes: 160 additions & 0 deletions src/deploy/firestore/prepare.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
import { expect } from "chai";
import * as sinon from "sinon";
import prepare from "./prepare";
import { FirestoreApi } from "../../firestore/api";
import * as types from "../../firestore/api-types";
import { FirebaseError } from "../../error";
import { Options } from "../../options";
import * as ensureApiEnabled from "../../ensureApiEnabled";
import * as fsConfig from "../../firestore/fsConfig";
import * as loadCJSON from "../../loadCJSON";
import { RulesDeploy } from "../../rulesDeploy";

describe("firestore prepare", () => {
let sandbox: sinon.SinonSandbox;
let getDatabaseStub: sinon.SinonStub;
let createDatabaseStub: sinon.SinonStub;

beforeEach(() => {
sandbox = sinon.createSandbox();
getDatabaseStub = sandbox.stub(FirestoreApi.prototype, "getDatabase");
createDatabaseStub = sandbox.stub(FirestoreApi.prototype, "createDatabase");
sandbox.stub(ensureApiEnabled, "ensure").resolves();
sandbox.stub(loadCJSON, "loadCJSON").returns({});
sandbox.stub(RulesDeploy.prototype, "addFile").returns();
sandbox.stub(RulesDeploy.prototype, "compile").resolves();
sandbox.stub(fsConfig, "getFirestoreConfig").returns([
{
database: "test-db",
rules: "firestore.rules",
indexes: "firestore.indexes.json",
},
]);
});

afterEach(() => {
sandbox.restore();
});

describe("createDatabase", () => {
const projectId = "test-project";
const options = {
projectId,
config: {
path: (p: string) => p,
data: {
firestore: {
database: "test-db",
},
},
},
} as unknown as Options;
Comment on lines +41 to +51
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The style guide (line 38) advises against using unknown as an escape hatch. Casting with as unknown as Options can make tests brittle and hide type errors. For new test files, it's a good practice to establish a pattern of creating more robust mocks. Consider creating a test utility to generate mock Options objects, or at a minimum, define the mock object with Partial<Options> and then cast to Options to make the partial nature of the mock more explicit.

References
  1. The style guide states: "Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards." (link)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm a little worried about this since this kind of removes type safety

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Gonna leave this for now since it is a test - however, we do have a mockOptions function definite in src/emulator/controller.spec.ts that could be turned into a general utility. Making a note to do so in a follow up PR (since we'll use it in other places too)


it("should create a database with default settings when dataAccessMode is missing", async () => {
getDatabaseStub.rejects({ status: 404 });
createDatabaseStub.resolves();

// We need to call the default export which calls createDatabase internally
await prepare({ projectId }, options);

expect(createDatabaseStub.calledOnce).to.be.true;
const args = createDatabaseStub.firstCall.args[0];

Check warning on line 61 in src/deploy/firestore/prepare.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe assignment of an `any` value
expect(args.firestoreDataAccessMode).to.be.undefined;

Check warning on line 62 in src/deploy/firestore/prepare.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .firestoreDataAccessMode on an `any` value
expect(args.mongodbCompatibleDataAccessMode).to.be.undefined;

Check warning on line 63 in src/deploy/firestore/prepare.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .mongodbCompatibleDataAccessMode on an `any` value
expect(args.databaseEdition).to.equal(types.DatabaseEdition.STANDARD);

Check warning on line 64 in src/deploy/firestore/prepare.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .databaseEdition on an `any` value
});

it("should create a database with FIRESTORE_NATIVE when specified on enterprise edition", async () => {
const enterpriseOptions = {
projectId,
config: {
path: (p: string) => p,
data: {
firestore: {
database: "test-db",
edition: "enterprise",
dataAccessMode: "FIRESTORE_NATIVE",
},
},
},
} as unknown as Options;
getDatabaseStub.rejects({ status: 404 });
createDatabaseStub.resolves();

await prepare({ projectId }, enterpriseOptions);

expect(createDatabaseStub.calledOnce).to.be.true;
const args = createDatabaseStub.firstCall.args[0];

Check warning on line 87 in src/deploy/firestore/prepare.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe assignment of an `any` value
expect(args.firestoreDataAccessMode).to.equal(types.DataAccessMode.ENABLED);

Check warning on line 88 in src/deploy/firestore/prepare.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .firestoreDataAccessMode on an `any` value
expect(args.mongodbCompatibleDataAccessMode).to.equal(types.DataAccessMode.DISABLED);

Check warning on line 89 in src/deploy/firestore/prepare.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .mongodbCompatibleDataAccessMode on an `any` value
expect(args.databaseEdition).to.equal(types.DatabaseEdition.ENTERPRISE);

Check warning on line 90 in src/deploy/firestore/prepare.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .databaseEdition on an `any` value
});

it("should create a database with MONGODB_COMPATIBLE when specified on enterprise edition", async () => {
const enterpriseOptions = {
projectId,
config: {
path: (p: string) => p,
data: {
firestore: {
database: "test-db",
edition: "enterprise",
dataAccessMode: "MONGODB_COMPATIBLE",
},
},
},
} as unknown as Options;
getDatabaseStub.rejects({ status: 404 });
createDatabaseStub.resolves();

await prepare({ projectId }, enterpriseOptions);

expect(createDatabaseStub.calledOnce).to.be.true;
const args = createDatabaseStub.firstCall.args[0];

Check warning on line 113 in src/deploy/firestore/prepare.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe assignment of an `any` value
expect(args.firestoreDataAccessMode).to.equal(types.DataAccessMode.DISABLED);

Check warning on line 114 in src/deploy/firestore/prepare.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .firestoreDataAccessMode on an `any` value
expect(args.mongodbCompatibleDataAccessMode).to.equal(types.DataAccessMode.ENABLED);
expect(args.databaseEdition).to.equal(types.DatabaseEdition.ENTERPRISE);
});

it("should throw an error when dataAccessMode is specified on standard edition", async () => {
const standardOptions = {
projectId,
config: {
data: {
firestore: {
database: "test-db",
edition: "standard",
dataAccessMode: "MONGODB_COMPATIBLE",
},
},
},
} as unknown as Options;
getDatabaseStub.rejects({ status: 404 });

await expect(prepare({ projectId }, standardOptions)).to.be.rejectedWith(
FirebaseError,
"dataAccessMode can only be specified for enterprise edition databases.",
);
});

it("should throw an error when dataAccessMode is specified without edition (defaults to standard)", async () => {
const defaultOptions = {
projectId,
config: {
data: {
firestore: {
database: "test-db",
dataAccessMode: "MONGODB_COMPATIBLE",
},
},
},
} as unknown as Options;
getDatabaseStub.rejects({ status: 404 });

await expect(prepare({ projectId }, defaultOptions)).to.be.rejectedWith(
FirebaseError,
"dataAccessMode can only be specified for enterprise edition databases.",
);
});
Comment thread
joehan marked this conversation as resolved.
});
});
19 changes: 19 additions & 0 deletions src/deploy/firestore/prepare.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,23 @@ async function createDatabase(context: any, options: Options): Promise<void> {
edition = upperEdition as types.DatabaseEdition;
}

let firestoreDataAccessMode: types.DataAccessMode | undefined;
let mongodbCompatibleDataAccessMode: types.DataAccessMode | undefined;
if (firestoreCfg.dataAccessMode) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We will need two dataAccessMode (firestoreDataAccessMode and mongoDataAccessMode - consistent with API design). Firestore will add support for both modes in the future (interoperability).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I was thinking that we could simplify this into a single field dataAccessMode, and turn it into an array once we add interoperability - ie:

// Just Mongo
dataAccessMode: MONGODB_COMPATIBLE

// Just Native

dataAccessMode: FIRESTORE_NATIVE

// Interop mode
dataAccessMode: [ MONGODB_COMPATIBLE, FIRESTORE_NATIVE]

This seemed more ergonomic to me (especially before interop is available), but I don't feel too strongly about this tho, so happy to match the backend API design if you prefer

if (edition !== types.DatabaseEdition.ENTERPRISE) {
throw new FirebaseError(
"dataAccessMode can only be specified for enterprise edition databases.",
);
}
if (firestoreCfg.dataAccessMode === "FIRESTORE_NATIVE") {
firestoreDataAccessMode = types.DataAccessMode.ENABLED;
mongodbCompatibleDataAccessMode = types.DataAccessMode.DISABLED;
} else if (firestoreCfg.dataAccessMode === "MONGODB_COMPATIBLE") {
firestoreDataAccessMode = types.DataAccessMode.DISABLED;
mongodbCompatibleDataAccessMode = types.DataAccessMode.ENABLED;
}
Comment thread
joehan marked this conversation as resolved.
Comment on lines +111 to +117
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

This if/else if block can be made more concise and less repetitive by using a boolean variable to determine the mode and then using ternary operators for the assignments. This improves readability and aligns with the style guide's preference for simpler branching logic.

    const isNative = firestoreCfg.dataAccessMode === "FIRESTORE_NATIVE";
    firestoreDataAccessMode = isNative ? types.DataAccessMode.ENABLED : types.DataAccessMode.DISABLED;
    mongodbCompatibleDataAccessMode = isNative ? types.DataAccessMode.DISABLED : types.DataAccessMode.ENABLED;
References
  1. The style guide encourages reducing nesting and considering helper functions to encapsulate branching. While not reducing nesting, this change simplifies the branching logic, which is in the spirit of the rule. (link)

}

const api = new FirestoreApi();
try {
await api.getDatabase(options.projectId, firestoreCfg.database);
Expand All @@ -118,6 +135,8 @@ async function createDatabase(context: any, options: Options): Promise<void> {
databaseEdition: edition,
deleteProtectionState: types.DatabaseDeleteProtectionState.DISABLED,
pointInTimeRecoveryEnablement: types.PointInTimeRecoveryEnablement.DISABLED,
firestoreDataAccessMode,
mongodbCompatibleDataAccessMode,
};
await api.createDatabase(createDatabaseReq);
}
Expand Down
3 changes: 3 additions & 0 deletions src/firebaseConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,13 @@ type DatabaseMultiple = ({
}> &
Deployable)[];

type DataAccessMode = "MONGODB_COMPATIBLE" | "FIRESTORE_NATIVE";

type FirestoreSingle = {
database?: string;
location?: string;
edition?: string;
dataAccessMode?: DataAccessMode;
rules?: string;
indexes?: string;
} & Deployable;
Expand Down
Loading