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

Fixed faulty transaction bug #6508

Merged
merged 22 commits into from
Aug 15, 2022
Merged
Show file tree
Hide file tree
Changes from 16 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
7 changes: 7 additions & 0 deletions .changeset/warm-pillows-know.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@firebase/database-compat": patch
"@firebase/database": patch
"@firebase/rules-unit-testing": patch
maneesht marked this conversation as resolved.
Show resolved Hide resolved
---

Fixed faulty transaction bug causing filtered index queries to override default queries.
5 changes: 4 additions & 1 deletion config/database.rules.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
{
"rules": {
".read": true,
".write": true
".write": true,
"testing": {
".indexOn": "testIndex"
}
}
}
2 changes: 1 addition & 1 deletion packages/database-compat/test/helpers/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ export function getFreshRepo(path: Path) {
'ISOLATED_REPO_' + freshRepoId++
);
activeFreshApps.push(app);
return (app as any).database().ref(path.toString());
return (app as any).database().ref(path.toString()); // TODO(mtewani): Remove explicit any
}

export function getFreshRepoFromReference(ref) {
Expand Down
4 changes: 2 additions & 2 deletions packages/database/src/core/view/ViewProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,7 @@ function viewProcessorApplyServerMerge(
// and event snap. I'm not sure if this will result in edge cases when a child is in one but
// not the other.
let curViewCache = viewCache;
let viewMergeTree;
let viewMergeTree: ImmutableTree<Node>;
if (pathIsEmpty(path)) {
viewMergeTree = changedChildren;
} else {
Expand Down Expand Up @@ -641,7 +641,7 @@ function viewProcessorApplyServerMerge(
viewMergeTree.children.inorderTraversal((childKey, childMergeTree) => {
const isUnknownDeepMerge =
!viewCache.serverCache.isCompleteForChild(childKey) &&
childMergeTree.value === undefined;
childMergeTree.value === null;
if (!serverNode.hasChild(childKey) && !isUnknownDeepMerge) {
const serverChild = viewCache.serverCache
.getNode()
Expand Down
53 changes: 53 additions & 0 deletions packages/database/test/exp/integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,14 @@ import {
child,
get,
limitToFirst,
onChildAdded,
onValue,
orderByChild,
query,
refFromURL,
set,
startAt,
update,
orderByKey
} from '../../src/api/Reference_impl';
import {
Expand Down Expand Up @@ -117,6 +120,56 @@ describe('Database@exp Tests', () => {
unsubscribe();
});

it('can properly handle unknown deep merges', async () => {
// Note: This test requires `testIndex` to be added as an index.
// Without an index, the test will erroneously pass.
// But we are unable to add indexes on the fly to the actual db, so
// running on the emulator is the best way to test this.
const database = getDatabase(defaultApp);
const root = ref(database, 'testing');
await set(root, {});

const q = query(root, orderByChild('testIndex'), limitToFirst(2));

const i1 = child(root, 'i1');
await set(root, {
i1: {
testIndex: 3,
timestamp: Date.now(),
action: 'test'
},
i2: {
testIndex: 1,
timestamp: Date.now(),
action: 'test'
},
i3: {
testIndex: 2,
timestamp: Date.now(),
action: 'test'
}
});
const ec = EventAccumulatorFactory.waitsForExactCount(2);
const onChildAddedCb = onChildAdded(q, snap => {
ec.addEvent(snap);
});
const onValueCb = onValue(i1, () => {
//no-op
});
await update(i1, {
timestamp: `${Date.now()}|1`
});
const results = await ec.promise;
results.forEach(result => {
const value = result.val();
expect(value).to.haveOwnProperty('timestamp');
expect(value).to.haveOwnProperty('action');
expect(value).to.haveOwnProperty('testIndex');
});
onChildAddedCb();
onValueCb();
});

// Tests to make sure onValue's data does not get mutated after calling get
it('calls onValue only once after get request with a non-default query', async () => {
const { readerRef } = getRWRefs(getDatabase(defaultApp));
Expand Down
1 change: 1 addition & 0 deletions packages/database/test/helpers/EventAccumulator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/

export const EventAccumulatorFactory = {
// TODO: Convert to use generics to take the most advantage of types.
waitsForCount: maxCount => {
// Note: This should be used sparingly as it can result in more events being raised than expected
let count = 0;
Expand Down
2 changes: 1 addition & 1 deletion scripts/emulator-testing/emulators/database-emulator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export class DatabaseEmulator extends Emulator {
{
uri: `http://localhost:${this.port}/.settings/rules.json?ns=${this.namespace}`,
headers: { Authorization: 'Bearer owner' },
body: '{ "rules": { ".read": true, ".write": true } }'
body: '{ "rules": { ".read": true, ".write": true, "testing": { ".indexOn": "testIndex" } } }'
maneesht marked this conversation as resolved.
Show resolved Hide resolved
},
(error, response, body) => {
if (error) reject(error);
Expand Down