Skip to content

chore(user-data): remove redundant withStats methods #7134

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jul 28, 2025
Merged

chore(user-data): remove redundant withStats methods #7134

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ef94ff8
chore(user-data): remove redundant withStats methods
gribnoysup Jul 23, 2025
3f8405a
fix(user-data): remove nonexistent exports
gribnoysup Jul 23, 2025
7f07ddd
fix(my-queries-storage): adjust types to match the method behavior
gribnoysup Jul 23, 2025
e09d426
fix(saved-aggregations-queries): adjust test fixture
gribnoysup Jul 23, 2025
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
4 changes: 0 additions & 4 deletions packages/compass-preferences-model/src/user-storage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,4 @@ export class UserStorageImpl implements UserStorage {
await this.userData.write(user.id, user);
return this.getUser(user.id);
}

private getFileName(id: string) {
return `${id}.json`;
}
}
54 changes: 0 additions & 54 deletions packages/compass-user-data/src/user-data.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,39 +125,6 @@ describe('user-data', function () {
expect(result.data).to.have.lengthOf(0);
expect(result.errors).to.have.lengthOf(2);
});

it('returns file stats', async function () {
await Promise.all(
[
['data1.json', JSON.stringify({ name: 'VSCode' })],
['data2.json', JSON.stringify({ name: 'Mongosh' })],
].map(([filepath, data]) => writeFileToStorage(filepath, data))
);

const { data } = await getUserData().readAllWithStats({
ignoreErrors: true,
});

{
const vscodeData = data.find((x) => x[0].name === 'VSCode');
expect(vscodeData?.[0]).to.deep.equal({
name: 'VSCode',
hasDarkMode: true,
hasWebSupport: false,
});
expect(vscodeData?.[1]).to.be.instanceOf(Stats);
}

{
const mongoshData = data.find((x) => x[0].name === 'Mongosh');
expect(mongoshData?.[0]).to.deep.equal({
name: 'Mongosh',
hasDarkMode: true,
hasWebSupport: false,
});
expect(mongoshData?.[1]).to.be.instanceOf(Stats);
}
});
});

context('UserData.readOne', function () {
Expand Down Expand Up @@ -302,27 +269,6 @@ describe('user-data', function () {
company: 'MongoDB',
});
});

it('return file stats', async function () {
await writeFileToStorage(
'data.json',
JSON.stringify({
name: 'Mongosh',
company: 'MongoDB',
})
);

const [data, stats] = await getUserData().readOneWithStats('data', {
ignoreErrors: false,
});

expect(data).to.deep.equal({
name: 'Mongosh',
hasDarkMode: true,
hasWebSupport: false,
});
expect(stats).to.be.instanceOf(Stats);
});
});

context('UserData.write', function () {
Expand Down
139 changes: 33 additions & 106 deletions packages/compass-user-data/src/user-data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,12 @@ const { log, mongoLogId } = createLogger('COMPASS-USER-STORAGE');

type SerializeContent<I> = (content: I) => string;
type DeserializeContent = (content: string) => unknown;
type GetFileName = (id: string) => string;

export type FileUserDataOptions<Input> = {
subdir: string;
basePath?: string;
serialize?: SerializeContent<Input>;
deserialize?: DeserializeContent;
getFileName?: GetFileName;
};

export type AtlasUserDataOptions<Input> = {
Expand All @@ -29,45 +27,11 @@ type ReadOptions = {
ignoreErrors: boolean;
};

// Copied from the Node.js fs module.
export interface Stats {
isFile(): boolean;
isDirectory(): boolean;
isBlockDevice(): boolean;
isCharacterDevice(): boolean;
isSymbolicLink(): boolean;
isFIFO(): boolean;
isSocket(): boolean;
dev: number;
ino: number;
mode: number;
nlink: number;
uid: number;
gid: number;
rdev: number;
size: number;
blksize: number;
blocks: number;
atimeMs: number;
mtimeMs: number;
ctimeMs: number;
birthtimeMs: number;
atime: Date;
mtime: Date;
ctime: Date;
birthtime: Date;
}

export interface ReadAllResult<T extends z.Schema> {
data: z.output<T>[];
errors: Error[];
Copy link
Collaborator Author

@gribnoysup gribnoysup Jul 23, 2025

Choose a reason for hiding this comment

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

This shape with returned errors is also not used anywhere really, we always pick up data, but it's a bigger change and this part of the interface doesn't depend on fs, so not touching it

}

export interface ReadAllWithStatsResult<T extends z.Schema> {
data: [z.output<T>, Stats][];
errors: Error[];
}

export abstract class IUserData<T extends z.Schema> {
protected readonly validator: T;
protected readonly serialize: SerializeContent<z.input<T>>;
Expand Down Expand Up @@ -100,7 +64,6 @@ export abstract class IUserData<T extends z.Schema> {
export class FileUserData<T extends z.Schema> extends IUserData<T> {
private readonly subdir: string;
private readonly basePath?: string;
private readonly getFileName: GetFileName;
protected readonly semaphore = new Semaphore(100);

constructor(
Expand All @@ -110,13 +73,15 @@ export class FileUserData<T extends z.Schema> extends IUserData<T> {
basePath,
serialize,
deserialize,
getFileName = (id) => `${id}.json`,
}: FileUserDataOptions<z.input<T>>
) {
super(validator, { serialize, deserialize });
this.subdir = subdir;
this.basePath = basePath;
this.getFileName = getFileName;
}

private getFileName(id: string) {
return `${id}.json`;
}

private async getEnsuredBasePath(): Promise<string> {
Expand Down Expand Up @@ -148,21 +113,15 @@ export class FileUserData<T extends z.Schema> extends IUserData<T> {
return path.resolve(root, pathRelativeToRoot);
}

private async readAndParseFileWithStats(
private async readAndParseFile(
absolutePath: string,
options: ReadOptions
): Promise<[z.output<T>, Stats] | undefined> {
): Promise<z.output<T> | undefined> {
let data: string;
let stats: Stats;
let handle: fs.FileHandle | undefined = undefined;
let release: (() => void) | undefined = undefined;
try {
release = await this.semaphore.waitForRelease();
handle = await fs.open(absolutePath, 'r');
[stats, data] = await Promise.all([
handle.stat(),
handle.readFile('utf-8'),
]);
data = await fs.readFile(absolutePath, 'utf-8');
} catch (error) {
log.error(mongoLogId(1_001_000_234), 'Filesystem', 'Error reading file', {
path: absolutePath,
Expand All @@ -173,13 +132,12 @@ export class FileUserData<T extends z.Schema> extends IUserData<T> {
}
throw error;
} finally {
await handle?.close();
release?.();
}

try {
const content = this.deserialize(data);
return [this.validator.parse(content), stats];
return this.validator.parse(content);
} catch (error) {
log.error(mongoLogId(1_001_000_235), 'Filesystem', 'Error parsing data', {
path: absolutePath,
Expand Down Expand Up @@ -235,70 +193,37 @@ export class FileUserData<T extends z.Schema> extends IUserData<T> {
}
}

async readAllWithStats(
async readAll(
options: ReadOptions = {
ignoreErrors: true,
}
): Promise<ReadAllWithStatsResult<T>> {
const absolutePath = await this.getFileAbsolutePath();
const filePathList = await fs.readdir(absolutePath);

const data = await Promise.allSettled(
filePathList.map((x) =>
this.readAndParseFileWithStats(path.join(absolutePath, x), options)
)
);

const result: ReadAllWithStatsResult<T> = {
): Promise<ReadAllResult<T>> {
const result: ReadAllResult<T> = {
data: [],
errors: [],
};

for (const item of data) {
if (item.status === 'fulfilled' && item.value) {
result.data.push(item.value);
try {
const absolutePath = await this.getFileAbsolutePath();
const filePathList = await fs.readdir(absolutePath);
for (const settled of await Promise.allSettled(
filePathList.map((x) => {
return this.readAndParseFile(path.join(absolutePath, x), options);
})
)) {
if (settled.status === 'fulfilled' && settled.value) {
result.data.push(settled.value);
}
if (settled.status === 'rejected') {
result.errors.push(settled.reason);
}
}
if (item.status === 'rejected') {
result.errors.push(item.reason);
return result;
} catch (err) {
if (options.ignoreErrors) {
return result;
}
throw err;
}

return result;
}

async readOneWithStats(
id: string,
options?: { ignoreErrors: false }
): Promise<[z.output<T>, Stats]>;
async readOneWithStats(
id: string,
options?: { ignoreErrors: true }
): Promise<[z.output<T>, Stats] | undefined>;
async readOneWithStats(
id: string,
options?: ReadOptions
): Promise<[z.output<T>, Stats] | undefined>;
async readOneWithStats(
id: string,
options: ReadOptions = {
ignoreErrors: true,
}
) {
const filepath = this.getFileName(id);
const absolutePath = await this.getFileAbsolutePath(filepath);
return await this.readAndParseFileWithStats(absolutePath, options);
}

async readAll(
options: ReadOptions = {
ignoreErrors: true,
}
): Promise<ReadAllResult<T>> {
const result = await this.readAllWithStats(options);
return {
data: result.data.map(([data]) => data),
errors: result.errors,
};
}

async readOne(
Expand All @@ -319,7 +244,9 @@ export class FileUserData<T extends z.Schema> extends IUserData<T> {
ignoreErrors: true,
}
) {
return (await this.readOneWithStats(id, options))?.[0];
const filepath = this.getFileName(id);
const absolutePath = await this.getFileAbsolutePath(filepath);
return await this.readAndParseFile(absolutePath, options);
}

async updateAttributes(
Expand Down
25 changes: 4 additions & 21 deletions packages/my-queries-storage/src/compass-pipeline-storage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import type { Stats } from '@mongodb-js/compass-user-data';
import { FileUserData } from '@mongodb-js/compass-user-data';
import { PipelineSchema } from './pipeline-storage-schema';
import type { SavedPipeline } from './pipeline-storage-schema';
Expand All @@ -13,21 +12,10 @@ export class CompassPipelineStorage implements PipelineStorage {
});
}

private mergeStats(pipeline: SavedPipeline, stats: Stats): SavedPipeline {
Copy link
Collaborator Author

@gribnoysup gribnoysup Jul 23, 2025

Choose a reason for hiding this comment

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

That was the only case where we use it, and even then we always write our own number on create / update

return {
...pipeline,
lastModified: new Date(stats.ctimeMs),
};
}

async loadAll(): Promise<SavedPipeline[]> {
try {
const { data } = await this.userData.readAllWithStats({
ignoreErrors: false,
});
return data.map(([item, stats]) => {
return this.mergeStats(item, stats);
});
const { data } = await this.userData.readAll();
return data;
} catch {
return [];
}
Expand All @@ -41,16 +29,11 @@ export class CompassPipelineStorage implements PipelineStorage {
}

private async loadOne(id: string): Promise<SavedPipeline> {
const [item, stats] = await this.userData.readOneWithStats(id);
return this.mergeStats(item, stats);
return await this.userData.readOne(id);
}

async createOrUpdate(id: string, attributes: SavedPipeline) {
const pipelineExists = Boolean(
await this.userData.readOne(id, {
ignoreErrors: true,
})
);
const pipelineExists = Boolean(await this.userData.readOne(id));
return await (pipelineExists
? this.updateAttributes(id, attributes)
: this.create(attributes));
Expand Down
4 changes: 2 additions & 2 deletions packages/my-queries-storage/src/pipeline-storage-schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ export const PipelineSchema = z.preprocess(
pipelineText: z.string(),
lastModified: z
.number()
.transform((x) => new Date(x))
.optional(),
.default(0)
Copy link
Collaborator Author

@gribnoysup gribnoysup Jul 23, 2025

Choose a reason for hiding this comment

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

In the rarest case ever if someone still somehow doesn't have this value stored, we fallback to zero, the logic for writing this value to UserData was here for such a long time, I don't think this will ever be used

.transform((x) => new Date(x)),
})
);

Expand Down
Loading