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

WIP: order latest first COMPASS-6706 #6361

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
87 changes: 75 additions & 12 deletions packages/compass-crud/src/stores/crud-store.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2250,7 +2250,16 @@ describe('store', function () {
});

it('should call find with $bsonSize projection when mongodb version is >= 4.4, not connected to ADF and csfle is disabled', async function () {
await fetchDocuments(dataService, '5.0.0', false, 'test.test', {});
await fetchDocuments(
dataService,
{
serverVersion: '5.0.0',
isDataLake: false,
isTimeSeries: false,
},
'test.test',
{}
);
expect(find).to.have.been.calledOnce;
expect(find.getCall(0))
.to.have.nested.property('args.2.projection')
Expand All @@ -2261,8 +2270,11 @@ describe('store', function () {
findResult = [{ __size: new Int32(42), __doc: { _id: 1 } }];
const docs = await fetchDocuments(
dataService,
'4.0.0',
false,
{
serverVersion: '4.0.0',
isDataLake: false,
isTimeSeries: false,
},
'test.test',
{}
);
Expand All @@ -2272,7 +2284,16 @@ describe('store', function () {
});

it('should NOT call find with $bsonSize projection when mongodb version is < 4.4', async function () {
await fetchDocuments(dataService, '4.0.0', false, 'test.test', {});
await fetchDocuments(
dataService,
{
serverVersion: '4.0.0',
isDataLake: false,
isTimeSeries: false,
},
'test.test',
{}
);
expect(find).to.have.been.calledOnce;
expect(find.getCall(0)).to.have.nested.property(
'args.2.projection',
Expand All @@ -2281,7 +2302,16 @@ describe('store', function () {
});

it('should NOT call find with $bsonSize projection when connected to ADF', async function () {
await fetchDocuments(dataService, '5.0.0', true, 'test.test', {});
await fetchDocuments(
dataService,
{
serverVersion: '5.0.0',
isDataLake: true,
isTimeSeries: false,
},
'test.test',
{}
);
expect(find).to.have.been.calledOnce;
expect(find.getCall(0)).to.have.nested.property(
'args.2.projection',
Expand All @@ -2291,7 +2321,16 @@ describe('store', function () {

it('should NOT call find with $bsonSize projection when csfle is enabled', async function () {
csfleMode = 'enabled';
await fetchDocuments(dataService, '5.0.0', false, 'test.test', {});
await fetchDocuments(
dataService,
{
serverVersion: '5.0.0',
isDataLake: false,
isTimeSeries: false,
},
'test.test',
{}
);
expect(find).to.have.been.calledOnce;
expect(find.getCall(0)).to.have.nested.property(
'args.2.projection',
Expand All @@ -2302,8 +2341,11 @@ describe('store', function () {
it('should keep user projection when provided', async function () {
await fetchDocuments(
dataService,
'5.0.0',
false,
{
serverVersion: '5.0.0',
isDataLake: false,
isTimeSeries: false,
},
'test.test',
{},
{
Expand All @@ -2326,8 +2368,11 @@ describe('store', function () {

const docs = await fetchDocuments(
dataService,
'5.0.0',
false,
{
serverVersion: '5.0.0',
isDataLake: false,
isTimeSeries: false,
},
'test.test',
{}
);
Expand All @@ -2346,7 +2391,16 @@ describe('store', function () {
find = sinon.stub().rejects(new TypeError('🤷‍♂️'));

try {
await fetchDocuments(dataService, '5.0.0', false, 'test.test', {});
await fetchDocuments(
dataService,
{
serverVersion: '5.0.0',
isDataLake: false,
isTimeSeries: false,
},
'test.test',
{}
);
expect.fail('Expected fetchDocuments to fail with error');
} catch (err) {
expect(find).to.have.been.calledOnce;
Expand All @@ -2358,7 +2412,16 @@ describe('store', function () {
find = sinon.stub().rejects(new MongoServerError('Nope'));

try {
await fetchDocuments(dataService, '3.0.0', true, 'test.test', {});
await fetchDocuments(
dataService,
{
serverVersion: '3.0.0',
isDataLake: true,
isTimeSeries: false,
},
'test.test',
{}
);
expect.fail('Expected fetchDocuments to fail with error');
} catch (err) {
expect(find).to.have.been.calledOnce;
Expand Down
38 changes: 30 additions & 8 deletions packages/compass-crud/src/stores/crud-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import Reflux from 'reflux';
import toNS from 'mongodb-ns';
import { findIndex, isEmpty, isEqual } from 'lodash';
import semver from 'semver';
import type { Sort } from 'mongodb';
import StateMixin from '@mongodb-js/reflux-state-mixin';
import type { Element } from 'hadron-document';
import { Document } from 'hadron-document';
Expand Down Expand Up @@ -109,20 +110,34 @@ const INITIAL_BULK_UPDATE_TEXT = `{
},
}`;

type FetchDocumentsOptions = {
serverVersion: string;
isDataLake: boolean;
isTimeSeries: boolean;
};

function getDefaultSortOrder(isTimeSeries: boolean): Sort {
if (isTimeSeries) {
return { $natural: 1 };
}

return { _id: -1 };
}

export const fetchDocuments: (
dataService: DataService,
serverVersion: string,
isDataLake: boolean,
fetchDocumentsOptions: FetchDocumentsOptions,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is getting ugly 😆

...args: Parameters<DataService['find']>
) => Promise<HadronDocument[]> = async (
dataService: DataService,
serverVersion,
isDataLake,
fetchDocumentsOptions,
ns,
filter,
options,
executionOptions
) => {
const { isTimeSeries, isDataLake, serverVersion } = fetchDocumentsOptions;

const canCalculateDocSize =
// $bsonSize is only supported for mongodb >= 4.4.0
semver.gte(serverVersion, '4.4.0') &&
Expand All @@ -138,6 +153,7 @@ export const fetchDocuments: (

const modifiedOptions = {
...options,
sort: options?.sort ? options.sort : getDefaultSortOrder(isTimeSeries),
projection: canCalculateDocSize
? { _id: 0, __doc: '$$ROOT', __size: { $bsonSize: '$$ROOT' } }
: options?.projection,
Expand Down Expand Up @@ -884,8 +900,11 @@ class CrudStoreImpl
try {
documents = await fetchDocuments(
this.dataService,
this.state.version,
this.state.isDataLake,
{
serverVersion: this.state.version,
isDataLake: this.state.isDataLake,
isTimeSeries: this.state.isTimeSeries,
},
ns,
filter ?? {},
opts as any,
Expand Down Expand Up @@ -1712,8 +1731,11 @@ class CrudStoreImpl
),
fetchDocuments(
this.dataService,
this.state.version,
this.state.isDataLake,
{
serverVersion: this.state.version,
isDataLake: this.state.isDataLake,
isTimeSeries: this.state.isTimeSeries,
},
ns,
query.filter ?? {},
findOptions as any,
Expand Down
3 changes: 2 additions & 1 deletion packages/compass-crud/src/utils/cancellable-queries.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ describe('cancellable-queries', function () {
dataService,
preferences,
'cancel.numbers',
null,
undefined,
{
signal,
}
Expand Down Expand Up @@ -138,6 +138,7 @@ describe('cancellable-queries', function () {
dataService,
preferences,
'cancel.numbers',
// @ts-expect-error this is deliberately wrong
'this is not a filter',
{
signal,
Expand Down
2 changes: 1 addition & 1 deletion packages/compass-crud/src/utils/cancellable-queries.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ export async function countDocuments(
dataService: DataService,
preferences: PreferencesAccess,
ns: string,
filter: BSONObject,
filter: BSONObject | undefined,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

drive-by

{
signal,
skip,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export const OPTION_DEFINITION: {
sort: {
name: 'sort',
type: 'document',
placeholder: "{ field: -1 } or [['field', -1]]",
placeholder: '{ $_id: -1}',
lerouxb marked this conversation as resolved.
Show resolved Hide resolved
link: 'https://docs.mongodb.com/manual/reference/method/cursor.sort/',
},
hint: {
Expand Down
Loading