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 for repoGet request #6273

Merged
merged 56 commits into from
Jun 16, 2022
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
db50631
WIP
maneesht May 13, 2022
46cd378
Simulate add/remove event registration in repoGetValue
jmwski May 13, 2022
c334171
Fixed duplicate onValue calls
maneesht May 17, 2022
f7298df
Updated test file
maneesht May 18, 2022
00a5907
Updated integration test to include get
maneesht May 19, 2022
3ea9604
Reduced timeout for tests
maneesht May 20, 2022
9520427
Removed unnecessary imports
maneesht May 20, 2022
e7dd651
Added back imports
maneesht May 20, 2022
f7ed576
Removed snapshot param
maneesht May 20, 2022
98c6e34
Addressed comments
maneesht May 23, 2022
43f1f8a
Fixed test
maneesht May 23, 2022
9394170
Removed only
maneesht May 23, 2022
2f82052
Removed unnecessary import
maneesht May 23, 2022
c976987
Updated test
maneesht May 23, 2022
fc4cf5c
Added extra test
maneesht May 24, 2022
7c31ece
Merge branch 'master' into fix-get-cache
maneesht May 25, 2022
3959cf3
Fixed formatting
maneesht May 25, 2022
536e103
Create rotten-tables-brush.md
maneesht May 25, 2022
3812a33
Added documentation
maneesht May 25, 2022
a8d8ea6
Passed formatting
maneesht May 25, 2022
7de3c12
Merge remote-tracking branch 'origin/master' into fix-get-cache
maneesht May 26, 2022
766f46d
Addressed comments
maneesht May 26, 2022
04356e4
WIP
maneesht May 31, 2022
a467055
Used unique id instead of foo
maneesht May 31, 2022
6145b2b
Used exactCount instead of Count for accumulator
maneesht Jun 1, 2022
788aaa0
Removed only
maneesht Jun 1, 2022
51d57a2
Reformatted
maneesht Jun 1, 2022
41a7303
Removed get wip
maneesht Jun 2, 2022
a58bb33
Got minimal repro
maneesht Jun 2, 2022
5d41ce0
Got minimal repro
maneesht Jun 2, 2022
bdfd40b
Fixed race condition issue
maneesht Jun 2, 2022
4ca2289
Updated tests
maneesht Jun 2, 2022
b52f468
Updated yarn lock file
maneesht Jun 3, 2022
27f9ea1
Updated module type
maneesht Jun 3, 2022
065dab0
Added comment
maneesht Jun 3, 2022
d5fef45
Merge remote-tracking branch 'origin/master' into fix-get-cache
maneesht Jun 3, 2022
74fe5e8
Removed unnecessary code
maneesht Jun 3, 2022
5858b86
Merge remote-tracking branch 'origin/master' into fix-get-cache
maneesht Jun 6, 2022
be41979
Removed unnecessary comment
maneesht Jun 6, 2022
f320a93
Removed unused imports
maneesht Jun 6, 2022
c4f8ff7
Updated test to remove redundant assertion
maneesht Jun 6, 2022
1dc798c
Moved query check
maneesht Jun 8, 2022
42e38f6
Updated tests to include example queries
maneesht Jun 8, 2022
b4c7f28
Fixed formatting
maneesht Jun 8, 2022
5559921
Added suggestions
maneesht Jun 8, 2022
52fcf72
Fixed formatting
maneesht Jun 8, 2022
c553405
Replaced let with const
maneesht Jun 8, 2022
a7e5865
Fixed tests
maneesht Jun 9, 2022
98845d1
Fixed test
maneesht Jun 9, 2022
69feeb7
Fixed formatting
maneesht Jun 9, 2022
60f9da0
Fixed tests
maneesht Jun 15, 2022
6f39fef
Removed comments
maneesht Jun 15, 2022
57a8db5
Removed version of uuid
maneesht Jun 15, 2022
31a747e
Added a new line
maneesht Jun 15, 2022
bd23960
Addressed comments
maneesht Jun 16, 2022
7e09d4b
Fixed formatting
maneesht Jun 16, 2022
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
6 changes: 0 additions & 6 deletions packages/database/src/core/PersistentConnection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,12 +206,6 @@ export class PersistentConnection extends ServerActions {
onComplete: (message: { [k: string]: unknown }) => {
const payload = message['d'] as string;
if (message['s'] === 'ok') {
this.onDataUpdate_(
request['p'],
payload,
/*isMerge*/ false,
/*tag*/ null
);
deferred.resolve(payload);
} else {
deferred.reject(payload);
Expand Down
54 changes: 46 additions & 8 deletions packages/database/src/core/Repo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,12 @@ import {
statsManagerGetOrCreateReporter
} from './stats/StatsManager';
import { StatsReporter, statsReporterIncludeStat } from './stats/StatsReporter';
import { syncPointGetView } from './SyncPoint';
import {
SyncTree,
syncTreeAckUserWrite,
syncTreeAddEventRegistration,
syncTreeAddToPath,
syncTreeApplyServerMerge,
syncTreeApplyServerOverwrite,
syncTreeApplyTaggedQueryMerge,
Expand All @@ -60,7 +62,8 @@ import {
syncTreeApplyUserOverwrite,
syncTreeCalcCompleteEventCache,
syncTreeGetServerValue,
syncTreeRemoveEventRegistration
syncTreeRemoveEventRegistration,
syncTreeTagForQuery_
} from './SyncTree';
import { Indexable } from './util/misc';
import {
Expand Down Expand Up @@ -466,15 +469,50 @@ export function repoGetValue(repo: Repo, query: QueryContext): Promise<Node> {
}
return repo.server_.get(query).then(
payload => {
const node = nodeFromJSON(payload as string).withIndex(
const node = nodeFromJSON(payload).withIndex(
query._queryParams.getIndex()
);
const events = syncTreeApplyServerOverwrite(
repo.serverSyncTree_,
query._path,
node
);
eventQueueRaiseEventsAtPath(repo.eventQueue_, query._path, events);
// if this is not a filtered query, then overwrite at path
if (query._queryParams.loadsAllData()) {
const events = syncTreeApplyServerOverwrite(
repo.serverSyncTree_,
query._path,
node
);
eventQueueRaiseEventsAtPath(repo.eventQueue_, query._path, events);
jmwski marked this conversation as resolved.
Show resolved Hide resolved
} else {
// Simulate `syncTreeAddEventRegistration` without events/listener setup.
jmwski marked this conversation as resolved.
Show resolved Hide resolved
const { syncPoint, serverCache, writesCache, serverCacheComplete } =
jmwski marked this conversation as resolved.
Show resolved Hide resolved
syncTreeAddToPath(query, repo.serverSyncTree_);
const view = syncPointGetView(
syncPoint,
query,
writesCache,
serverCache,
serverCacheComplete
);
if (!syncPoint.views.has(query._queryIdentifier)) {
syncPoint.views.set(query._queryIdentifier, view);
}
const tag = syncTreeTagForQuery_(repo.serverSyncTree_, query);
jmwski marked this conversation as resolved.
Show resolved Hide resolved
const events = syncTreeApplyTaggedQueryOverwrite(
repo.serverSyncTree_,
query._path,
node,
tag
);
eventQueueRaiseEventsAtPath(repo.eventQueue_, query._path, events);
// Call `syncTreeRemoveEventRegistration` with a null event registration, since there is none.
const cancels = syncTreeRemoveEventRegistration(
repo.serverSyncTree_,
query,
null
);
assert(
jmwski marked this conversation as resolved.
Show resolved Hide resolved
cancels.length === 0,
'unexpected cancel events in repoGetValue'
);
}
return Promise.resolve(node);
},
err => {
Expand Down
43 changes: 32 additions & 11 deletions packages/database/src/core/SyncTree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -482,16 +482,7 @@ export function syncTreeApplyTaggedQueryMerge(
}
}

/**
* Add an event callback for the specified query.
*
* @returns Events to raise.
*/
export function syncTreeAddEventRegistration(
syncTree: SyncTree,
query: QueryContext,
eventRegistration: EventRegistration
): Event[] {
export function syncTreeAddToPath(query: QueryContext, syncTree: SyncTree) {
const path = query._path;

let serverCache: Node | null = null;
Expand Down Expand Up @@ -550,6 +541,36 @@ export function syncTreeAddEventRegistration(
syncTree.tagToQueryMap.set(tag, queryKey);
}
const writesCache = writeTreeChildWrites(syncTree.pendingWriteTree_, path);
// TODO: break this down so you do the minimal amount
jmwski marked this conversation as resolved.
Show resolved Hide resolved
return {
syncPoint,
writesCache,
serverCache,
serverCacheComplete,
foundAncestorDefaultView,
viewAlreadyExists
};
}

/**
* Add an event callback for the specified query.
*
* @returns Events to raise.
*/
export function syncTreeAddEventRegistration(
syncTree: SyncTree,
query: QueryContext,
eventRegistration: EventRegistration
): Event[] {
const {
syncPoint,
serverCache,
writesCache,
serverCacheComplete,
viewAlreadyExists,
foundAncestorDefaultView
} = syncTreeAddToPath(query, syncTree);

let events = syncPointAddEventRegistration(
syncPoint,
query,
Expand Down Expand Up @@ -803,7 +824,7 @@ function syncTreeCreateListenerForView_(
/**
* Return the tag associated with the given query.
*/
function syncTreeTagForQuery_(
export function syncTreeTagForQuery_(
syncTree: SyncTree,
query: QueryContext
): number | null {
Expand Down
24 changes: 22 additions & 2 deletions packages/database/test/exp/integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,12 @@ import { initializeApp, deleteApp } from '@firebase/app';
import { Deferred } from '@firebase/util';
import { expect } from 'chai';

import { onValue, set } from '../../src/api/Reference_impl';
import {
limitToFirst,
onValue,
query,
set
} from '../../src/api/Reference_impl';
import {
get,
getDatabase,
Expand All @@ -32,7 +37,7 @@ import {
runTransaction
} from '../../src/index';
import { EventAccumulatorFactory } from '../helpers/EventAccumulator';
import { DATABASE_ADDRESS, DATABASE_URL } from '../helpers/util';
import { DATABASE_ADDRESS, DATABASE_URL, waitFor } from '../helpers/util';

export function createTestApp() {
return initializeApp({ databaseURL: DATABASE_URL });
Expand Down Expand Up @@ -97,6 +102,21 @@ describe('Database@exp Tests', () => {
expect(snap2).to.equal('b');
});

// Tests to make sure onValue's data does not get mutated after calling get
jmwski marked this conversation as resolved.
Show resolved Hide resolved
it('calls onValue only once after get request', async () => {
const db = getDatabase(defaultApp);
const testRef = ref(db, 'foo');
const initial = [{ name: 'child1' }, { name: 'child2' }];
await set(testRef, initial);
let count = 0;
onValue(testRef, () => {
count++;
jmwski marked this conversation as resolved.
Show resolved Hide resolved
});
await get(query(testRef, limitToFirst(1)));
jmwski marked this conversation as resolved.
Show resolved Hide resolved
await waitFor(2000);
expect(count).to.equal(1);
});

it('Can use onlyOnce', async () => {
const db = getDatabase(defaultApp);
const fooRef = ref(db, 'foo');
Expand Down
6 changes: 6 additions & 0 deletions packages/database/test/helpers/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,3 +70,9 @@ export function shuffle(arr, randFn = Math.random) {
arr[j] = tmp;
}
}

// Waits for specific number of milliseconds before resolving
// Example: await waitFor(4000) will wait until 4 seconds to execute the next line of code.
export function waitFor(waitTimeInMS: number) {
return new Promise(resolve => setTimeout(resolve, waitTimeInMS));
}