diff --git a/crates/firestore-database/src/database.rs b/crates/firestore-database/src/database.rs index cd187fe..e3e1047 100644 --- a/crates/firestore-database/src/database.rs +++ b/crates/firestore-database/src/database.rs @@ -129,19 +129,12 @@ impl FirestoreDatabase { consistency: ReadConsistency, ) -> Result>> { if let ReadConsistency::Transaction(id) = consistency { - Ok(Some(self.get_txn(id).await?)) + Ok(Some(self.transactions.get(id).await?)) } else { Ok(None) } } - pub async fn get_txn( - &self, - id: TransactionId, - ) -> Result, GenericDatabaseError> { - self.transactions.get(id).await - } - /// Get all the collections that reside directly under the given parent. This means that: /// - the IDs will not contain a `/` /// - the result will be empty if `parent` is a [`Ref::Collection`]. @@ -320,12 +313,7 @@ impl FirestoreDatabase { transaction: TransactionId, ) -> Result<(Timestamp, Vec)> { let txn = self.transactions.get(transaction).await?; - let txn = &txn.as_read_write().ok_or_else(|| { - GenericDatabaseError::invalid_argument(format!( - "Transaction {transaction:?} is read-only" - )) - })?; - let time = Timestamp::now(); + let rw_txn = txn.as_read_write(); let mut write_results = vec![]; let mut updates = HashMap::new(); @@ -335,11 +323,20 @@ impl FirestoreDatabase { for write in &writes { let name = get_doc_name_from_write(write)?; if let Entry::Vacant(entry) = write_guard_cache.entry(name.clone()) { + let txn = rw_txn.as_ref().ok_or_else(|| { + GenericDatabaseError::invalid_argument( + "Cannot modify entities in a read-only transaction", + ) + })?; entry.insert(txn.take_write_guard(&name).await?); } } - txn.drop_remaining_guards().await; + if let Some(txn) = rw_txn { + txn.drop_remaining_guards().await; + } + + let time = Timestamp::now(); for write in writes { let name = get_doc_name_from_write(&write)?; @@ -351,7 +348,7 @@ impl FirestoreDatabase { } } - self.transactions.stop(transaction).await?; + self.transactions.remove(transaction).await?; self.send_event(DatabaseEvent { database: Arc::downgrade(self), @@ -457,7 +454,7 @@ impl FirestoreDatabase { } pub async fn rollback(&self, transaction: TransactionId) -> Result<()> { - self.transactions.stop(transaction).await?; + self.transactions.remove(transaction).await?; Ok(()) } diff --git a/crates/firestore-database/src/database/transaction.rs b/crates/firestore-database/src/database/transaction.rs index ac92103..9bde0a5 100644 --- a/crates/firestore-database/src/database/transaction.rs +++ b/crates/firestore-database/src/database/transaction.rs @@ -77,29 +77,31 @@ impl RunningTransactions { } } - pub(crate) async fn stop(&self, id: TransactionId) -> Result<()> { + pub(crate) async fn remove(&self, id: TransactionId) -> Result> { self.txns.write().await.remove(&id).ok_or_else(|| { GenericDatabaseError::invalid_argument(format!("invalid transaction ID: {}", id.0)) - })?; - Ok(()) + }) } } #[derive(Debug)] -pub enum Transaction { +pub(crate) enum Transaction { ReadWrite(ReadWriteTransaction), ReadOnly(ReadOnlyTransaction), } impl Transaction { - pub async fn read_doc(&self, name: &DocumentRef) -> Result>> { + pub(crate) async fn read_doc( + &self, + name: &DocumentRef, + ) -> Result>> { match self { Transaction::ReadWrite(txn) => txn.read_doc(name).await, Transaction::ReadOnly(txn) => txn.read_doc(name).await, } } - pub fn as_read_write(&self) -> Option<&ReadWriteTransaction> { + pub(crate) fn as_read_write(&self) -> Option<&ReadWriteTransaction> { if let Self::ReadWrite(v) = self { Some(v) } else { @@ -109,10 +111,10 @@ impl Transaction { } #[derive(Debug)] -pub struct ReadWriteTransaction { - pub id: TransactionId, +pub(crate) struct ReadWriteTransaction { + pub(crate) id: TransactionId, database: Weak, - guards: Mutex>>, + guards: Mutex>>, } impl ReadWriteTransaction { @@ -124,7 +126,10 @@ impl ReadWriteTransaction { } } - pub async fn read_doc(&self, name: &DocumentRef) -> Result>> { + pub(crate) async fn read_doc( + &self, + name: &DocumentRef, + ) -> Result>> { Ok(self .read_guard(name) .await? @@ -132,11 +137,11 @@ impl ReadWriteTransaction { .cloned()) } - pub async fn drop_remaining_guards(&self) { + pub(crate) async fn drop_remaining_guards(&self) { self.guards.lock().await.clear(); } - pub async fn take_write_guard( + pub(crate) async fn take_write_guard( &self, name: &DocumentRef, ) -> Result { @@ -173,14 +178,15 @@ impl ReadWriteTransaction { } #[derive(Debug)] -pub struct ReadOnlyTransaction { - pub id: TransactionId, - pub database: Weak, - pub consistency: ReadConsistency, +pub(crate) struct ReadOnlyTransaction { + #[allow(dead_code)] // Only useful in logging + pub(crate) id: TransactionId, + pub(crate) database: Weak, + pub(crate) consistency: ReadConsistency, } impl ReadOnlyTransaction { - pub fn new( + pub(crate) fn new( id: TransactionId, database: Weak, consistency: ReadConsistency, diff --git a/test-suite/tests/3-basic-transaction.test.ts b/test-suite/tests/3-basic-transaction.test.ts index 610350b..4019eaf 100644 --- a/test-suite/tests/3-basic-transaction.test.ts +++ b/test-suite/tests/3-basic-transaction.test.ts @@ -5,43 +5,40 @@ import { AsyncLocalStorage } from 'async_hooks'; import { range } from 'lodash'; import { setTimeout as time } from 'timers/promises'; import { fs } from './utils'; -import { writeData } from './utils/firestore'; describe('concurrent tests', () => { // no concurrent tests with the Java Emulator.. - if (fs.connection === 'JAVA EMULATOR') { - test.concurrent = test; - } + const concurrent = fs.connection === 'JAVA EMULATOR' ? test : test.concurrent; - test.concurrent('simple txn', async () => { + concurrent('simple txn', async () => { const [docRef1] = refs(); await fs.firestore.runTransaction(async txn => { expect(await txn.get(docRef1)).toHaveProperty('exists', false); - txn.set(docRef1, writeData({ foo: 'bar' })); + txn.set(docRef1, fs.writeData({ foo: 'bar' })); expect(() => txn.get(docRef1)).toThrow('Firestore transactions require all reads to be executed before all writes.'); }); expect(await getData(docRef1.get())).toEqual({ foo: 'bar' }); }); - test.concurrent('updating same doc multiple times', async () => { + concurrent('updating same doc multiple times', async () => { const [docRef1] = refs(); await fs.firestore.runTransaction(async txn => { expect(await txn.get(docRef1)).toHaveProperty('exists', false); - txn.set(docRef1, writeData({ foo: fs.exported.FieldValue.increment(1) })); + txn.set(docRef1, fs.writeData({ foo: fs.exported.FieldValue.increment(1) })); txn.update(docRef1, { foo: fs.exported.FieldValue.increment(1) }); }); expect(await getData(docRef1.get())).toEqual({ foo: 2 }); }); - test.concurrent('using txn.getAll', async () => { + concurrent('using txn.getAll', async () => { const [docRef1, docRef2] = refs(); - await docRef1.set(writeData({ some: 'data' })); + await docRef1.set(fs.writeData({ some: 'data' })); await fs.firestore.runTransaction(async txn => { const [snap1, snap2] = await txn.getAll(docRef1, docRef2); @@ -49,20 +46,20 @@ describe('concurrent tests', () => { expect(await getData(snap1)).toEqual({ some: 'data' }); expect(snap2.exists).toBeFalse(); - txn.set(docRef2, writeData({ foo: 'bar' })); + txn.set(docRef2, fs.writeData({ foo: 'bar' })); }); expect(await getData(docRef1.get())).toEqual({ some: 'data' }); expect(await getData(docRef2.get())).toEqual({ foo: 'bar' }); }); - test.concurrent('aborting transaction', async () => { + concurrent('aborting transaction', async () => { const [docRef1] = refs(); await expect( fs.firestore.runTransaction(async txn => { expect(await txn.get(docRef1)).toHaveProperty('exists', false); - txn.set(docRef1, writeData({ foo: 'bar' })); + txn.set(docRef1, fs.writeData({ foo: 'bar' })); throw new Error('I quit!'); }), @@ -76,10 +73,10 @@ describe('concurrent tests', () => { // - 10 ABORTED: Transaction lock timeout // - inconsistent number of `tries` fs.notImplementedInJava || - test.concurrent('retry if document is locked', async () => { + concurrent('retry if document is locked', async () => { const [docRef1] = refs(); - await docRef1.set(writeData({ some: 'data' })); + await docRef1.set(fs.writeData({ some: 'data' })); await runTxn('outer', [docRef1], async () => { const { innerTxnCompleted } = await innerTxn('inner', [docRef1]); @@ -95,7 +92,7 @@ describe('concurrent tests', () => { }); fs.notImplementedInJava || - test.concurrent('lock on non-existing document', async () => { + concurrent('lock on non-existing document', async () => { const [docRef1] = refs(); await runTxn('outer', [docRef1], async () => { @@ -110,7 +107,7 @@ describe('concurrent tests', () => { }); }); - test.concurrent('no lock if getting separate documents', async () => { + concurrent('no lock if getting separate documents', async () => { const [docRef1, docRef2] = refs(); await runTxn('outer', [docRef1], async () => { @@ -122,7 +119,7 @@ describe('concurrent tests', () => { }); // Note: Very slow on Cloud Firestore!! - test.concurrent( + concurrent( 'chaos', async () => { const [docRef1, docRef2] = refs(); @@ -147,7 +144,7 @@ describe('concurrent tests', () => { 45_000, ); - test.concurrent('only read locked document', async () => { + concurrent('only read locked document', async () => { const [docRef1, docRef2] = refs(); await docRef1.create(fs.writeData()); @@ -167,10 +164,10 @@ describe('concurrent tests', () => { }); }); - test.concurrent('regular `set` waits on transaction', async () => { + concurrent('regular `set` waits on transaction', async () => { const [docRef1] = refs(); - await docRef1.set(writeData({ some: 'data' })); + await docRef1.set(fs.writeData({ some: 'data' })); let setDone = false; await runTxn('outer', [docRef1], async () => { @@ -183,7 +180,7 @@ describe('concurrent tests', () => { }); describe('tests with synchronized processes', () => { - test.concurrent('reading the same doc from different txns', async () => { + concurrent('reading the same doc from different txns', async () => { const test = new ConcurrentTest(); // Scenario: // Process 1 - create doc @@ -248,7 +245,7 @@ describe('concurrent tests', () => { ]); }); - test.concurrent('reading the same doc from different txns, try to write in second txn', async () => { + concurrent('reading the same doc from different txns, try to write in second txn', async () => { const test = new ConcurrentTest(); // Scenario: // Process 1 - create doc @@ -326,7 +323,7 @@ describe('concurrent tests', () => { ]); }); - test.concurrent('reading the same doc from different txns, try to write in both txns', async () => { + concurrent('reading the same doc from different txns, try to write in both txns', async () => { const test = new ConcurrentTest(); // Scenario: // Process 1 - create doc @@ -411,7 +408,7 @@ describe('concurrent tests', () => { ]); }); - test.concurrent('regular writes also wait until all txns-locks are released', async () => { + concurrent('regular writes also wait until all txns-locks are released', async () => { const test = new ConcurrentTest(); // Scenario: // Process 1 - create outside txn (doc 1), completes immediately. @@ -538,7 +535,7 @@ describe('concurrent tests', () => { fs.notImplementedInRust || fs.notImplementedInJava || fs.notImplementedInCloud || - test.concurrent('deadlock', async () => { + concurrent('deadlock', async () => { const test = new ConcurrentTest(40_000); const [ref1, ref2] = refs(); @@ -585,7 +582,7 @@ describe('concurrent tests', () => { }); describe('queries', () => { - test.concurrent('update after reading a query', async () => { + concurrent('update after reading a query', async () => { const testName = 'update'; const [docRef1, docRef2] = refs(); @@ -631,7 +628,7 @@ describe('concurrent tests', () => { ]); }); - test.concurrent('delete after reading a query', async () => { + concurrent('delete after reading a query', async () => { const testName = 'delete'; const [docRef1, docRef2] = refs(); @@ -676,7 +673,7 @@ describe('concurrent tests', () => { fs.notImplementedInJava || // Timeout fs.notImplementedInRust || // Timeout - test.concurrent('create after reading a query', async () => { + concurrent('create after reading a query', async () => { const testName = 'create'; const [docRef1, docRef2] = refs(); @@ -769,7 +766,7 @@ describe('concurrent tests', () => { ({ awaitAfterTxn } = (await runAfterGet()) ?? {}); for (const ref of write) { - txn.set(ref, writeData({ [name]: { tries } }), { merge: true }); + txn.set(ref, fs.writeData({ [name]: { tries } }), { merge: true }); } }); await awaitAfterTxn; diff --git a/test-suite/tests/6-readonly-transaction.test.ts b/test-suite/tests/6-readonly-transaction.test.ts new file mode 100644 index 0000000..2452b2b --- /dev/null +++ b/test-suite/tests/6-readonly-transaction.test.ts @@ -0,0 +1,126 @@ +import { range } from 'lodash'; +import { fs } from './utils'; + +describe('readonly transactions', () => { + interface Doc { + id: number; + time: FirebaseFirestore.Timestamp; + } + + async function createDocs(n = 2) { + const refs = range(n).map(() => fs.collection.doc()); + const writeTimes: FirebaseFirestore.Timestamp[] = []; + const checkTimes = []; + for (const [ref, i] of refs.map((ref, i) => [ref, i] as const)) { + const { writeTime } = await ref.create(fs.writeData({ id: i + 1, time: fs.exported.FieldValue.serverTimestamp() })); + writeTimes.push(writeTime); + checkTimes.push( + expect.toSatisfy( + v => + // Every `time` field should not be after the reported write time... + v <= writeTime && + // ... and should be after the reported write time of the previous document (if any). + (i === 0 || v > writeTimes[i - 1]), + ), + ); + } + return { refs, writeTimes, checkTimes }; + } + + test.concurrent('simple read-only txn', async () => { + const { refs, checkTimes } = await createDocs(); + + const snaps = await fs.firestore.runTransaction(txn => txn.getAll(...refs), { readOnly: true }); + const docs = snaps.map(snap => fs.readData(snap.data())); + expect(docs).toEqual([ + { id: 1, time: checkTimes[0] }, + { id: 2, time: checkTimes[1] }, + ]); + expect(docs[0].time < docs[1].time).toBeTrue(); + }); + + test.concurrent('simple read-only with field mask', async () => { + const { refs } = await createDocs(); + + const snaps = await fs.firestore.runTransaction(txn => txn.getAll(...refs, { fieldMask: ['id'] }), { readOnly: true }); + const docs = snaps.map(snap => snap.data() as Doc); + expect(docs).toEqual([{ id: 1 }, { id: 2 }]); + }); + + test.concurrent('try to write in a read-only transaction', async () => { + const { refs } = await createDocs(); + + const txnPromise = fs.firestore.runTransaction( + async txn => { + txn.update(refs[0], { time: fs.exported.FieldValue.serverTimestamp(), broken: true }); + txn.update(refs[1], { time: fs.exported.FieldValue.serverTimestamp(), broken: true }); + }, + { readOnly: true }, + ); + + if (fs.connection === 'JAVA EMULATOR') { + // Java Firestore Emulator allows writing in read-only transactions. + await expect(txnPromise).resolves.toBeUndefined(); + const snaps = await Promise.all(refs.map(ref => ref.get())); + const docs = snaps.map(s => fs.readData(s.data())); + expect(docs).toEqual([expect.objectContaining({ broken: true }), expect.objectContaining({ broken: true })]); + } else { + await expect(txnPromise).rejects.toThrow('INVALID_ARGUMENT: Cannot modify entities in a read-only transaction'); + } + }); + + test.concurrent('with specific read-time', async () => { + const { refs, writeTimes, checkTimes } = await createDocs(); + const { writeTime: updateTime } = await refs[0].update({ updated: true }); + + // When using the time of the first document, we should not find the second document. + const onlyTheFirst = await fs.firestore.runTransaction(txn => txn.getAll(...refs), { readOnly: true, readTime: writeTimes[0] }); + expect(onlyTheFirst.map(s => s.exists)).toEqual([true, false]); + expect(fs.readData(onlyTheFirst[0].data())).toEqual({ id: 1, time: checkTimes[0] }); + + // When using the time of the second document, we should see the original documents. + const originalSnaps = await fs.firestore.runTransaction(txn => txn.getAll(...refs), { readOnly: true, readTime: writeTimes[1] }); + const originalDocs = originalSnaps.map(s => fs.readData(s.data())); + expect(originalDocs).toEqual([ + { id: 1, time: checkTimes[0] }, + { id: 2, time: checkTimes[1] }, + ]); + + // When using the time of the last update, we should see the updated documents. + const updatedSnaps = await fs.firestore.runTransaction(txn => txn.getAll(...refs), { readOnly: true, readTime: updateTime }); + const updatedDocs = updatedSnaps.map(s => fs.readData(s.data())); + expect(updatedDocs).toEqual([ + { id: 1, time: checkTimes[0], updated: true }, + { id: 2, time: checkTimes[1] }, + ]); + }); + + test.concurrent('with specific read-time with field mask', async () => { + const { refs, writeTimes } = await createDocs(); + const { writeTime: updateTime } = await refs[0].update({ updated: true }); + + // When using the time of the first document, we should not find the second document. + const onlyTheFirst = await fs.firestore.runTransaction(txn => txn.getAll(...refs, { fieldMask: ['id', 'updated'] }), { + readOnly: true, + readTime: writeTimes[0], + }); + expect(onlyTheFirst.map(s => s.exists)).toEqual([true, false]); + expect(onlyTheFirst[0].data()).toEqual({ id: 1 }); + + // When using the time of the second document, we should see the original documents. + const originalSnaps = await fs.firestore.runTransaction(txn => txn.getAll(...refs, { fieldMask: ['id', 'updated'] }), { + readOnly: true, + readTime: writeTimes[1], + }); + const originalDocs = originalSnaps.map(s => s.data()); + expect(originalDocs).toEqual([{ id: 1 }, { id: 2 }]); + + // When using the time of the last update, we should see the updated documents. + const updatedSnaps = await fs.firestore.runTransaction(txn => txn.getAll(...refs, { fieldMask: ['id', 'updated'] }), { + readOnly: true, + readTime: updateTime, + }); + const updatedDocs = updatedSnaps.map(s => s.data()); + expect(updatedDocs).toEqual([{ id: 1, updated: true }, { id: 2 }]); + }); +}); diff --git a/test-suite/tests/utils/firestore.ts b/test-suite/tests/utils/firestore.ts index 1e4e54b..815f7fa 100644 --- a/test-suite/tests/utils/firestore.ts +++ b/test-suite/tests/utils/firestore.ts @@ -23,6 +23,8 @@ export const notImplementedInCloud = connection === 'CLOUD FIRESTORE' ? [] : und const mainTestDoc = firestore.collection('tests').doc(); export const collection = mainTestDoc.collection('collection'); +export function writeData(data: FirebaseFirestore.WithFieldValue): object; +export function writeData(): object; export function writeData(data: object = {}) { const ttl = exported.Timestamp.fromMillis(Date.now() + ms('1h')); return { ...data, ttl };