Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
24 changes: 13 additions & 11 deletions core/src/artifacts/in_memory_artifact_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,17 +101,15 @@ export class InMemoryArtifactService implements BaseArtifactService {
userId,
sessionId,
}: ListArtifactKeysRequest): Promise<string[]> {
const sessionPrefix = `${appName}/${userId}/${sessionId}/`;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we just encode/decode every part of the path (appName, userId, sessionId, filename) to not to have the delimiter (/)?

No need to create an array as a key. I think this is antipatern and better to keep keys as numbers or strings but not as objects.

const usernamespacePrefix = `${appName}/${userId}/user/`;
const sessionPrefix = artifactPrefix('session', appName, userId, sessionId);
const userPrefix = artifactPrefix('user', appName, userId);
const filenames: string[] = [];

for (const path in this.artifacts) {
if (path.startsWith(sessionPrefix)) {
const filename = path.replace(sessionPrefix, '');
filenames.push(filename);
} else if (path.startsWith(usernamespacePrefix)) {
const filename = path.replace(usernamespacePrefix, '');
filenames.push(filename);
filenames.push(decodeURIComponent(path.slice(sessionPrefix.length)));
} else if (path.startsWith(userPrefix)) {
filenames.push(decodeURIComponent(path.slice(userPrefix.length)));
}
}

Expand Down Expand Up @@ -197,13 +195,13 @@ export class InMemoryArtifactService implements BaseArtifactService {
}

/**
* Constructs the path to the artifact.
* Constructs the storage key for the artifact.
*
* @param appName The app name.
* @param userId The user ID.
* @param sessionId The session ID.
* @param filename The filename.
* @return The path to the artifact.
* @return The encoded storage key for the artifact.
*/
function artifactPath(
appName: string,
Expand All @@ -212,10 +210,14 @@ function artifactPath(
filename: string,
): string {
if (fileHasUserNamespace(filename)) {
return `${appName}/${userId}/user/${filename}`;
return `${artifactPrefix('user', appName, userId)}${encodeURIComponent(filename)}`;
}

return `${appName}/${userId}/${sessionId}/${filename}`;
return `${artifactPrefix('session', appName, userId, sessionId)}${encodeURIComponent(filename)}`;
}

function artifactPrefix(scope: string, ...parts: string[]): string {
return `${[scope, ...parts].map(encodeURIComponent).join('/')}/`;
}

/**
Expand Down
92 changes: 91 additions & 1 deletion core/test/artifacts/in_memory_artifact_service_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,102 @@
*/

import {InMemoryArtifactService} from '@google/adk';
import {describe} from 'vitest';
import {describe, expect, it} from 'vitest';
import {runArtifactServiceTests} from './artifact_service_test_utils.js';

describe('InMemoryArtifactService', () => {
runArtifactServiceTests(
async () => new InMemoryArtifactService(),
async () => {},
);

it('keeps artifacts with ambiguous path components isolated', async () => {
const service = new InMemoryArtifactService();

await service.saveArtifact({
appName: 'app',
userId: 'user',
sessionId: 'session',
filename: 'nested/report.txt',
artifact: {text: 'artifact-a'},
});
await service.saveArtifact({
appName: 'app',
userId: 'user',
sessionId: 'session/nested',
filename: 'report.txt',
artifact: {text: 'artifact-b'},
});

const artifactA = await service.loadArtifact({
appName: 'app',
userId: 'user',
sessionId: 'session',
filename: 'nested/report.txt',
});
const artifactB = await service.loadArtifact({
appName: 'app',
userId: 'user',
sessionId: 'session/nested',
filename: 'report.txt',
});

expect(artifactA?.text).toBe('artifact-a');
expect(artifactB?.text).toBe('artifact-b');
});

it('keeps artifacts with ambiguous app and user components isolated', async () => {
const service = new InMemoryArtifactService();

await service.saveArtifact({
appName: 'app',
userId: 'nested/user',
sessionId: 'session',
filename: 'report.txt',
artifact: {text: 'artifact-a'},
});
await service.saveArtifact({
appName: 'app/nested',
userId: 'user',
sessionId: 'session',
filename: 'report.txt',
artifact: {text: 'artifact-b'},
});

const artifactA = await service.loadArtifact({
appName: 'app',
userId: 'nested/user',
sessionId: 'session',
filename: 'report.txt',
});
const artifactB = await service.loadArtifact({
appName: 'app/nested',
userId: 'user',
sessionId: 'session',
filename: 'report.txt',
});

expect(artifactA?.text).toBe('artifact-a');
expect(artifactB?.text).toBe('artifact-b');
});

it('does not leak a session named user into other sessions', async () => {
const service = new InMemoryArtifactService();

await service.saveArtifact({
appName: 'app',
userId: 'user',
sessionId: 'user',
filename: 'foo.txt',
artifact: {text: 'session-scoped'},
});

const keys = await service.listArtifactKeys({
appName: 'app',
userId: 'user',
sessionId: 'other',
});

expect(keys).toEqual([]);
});
});
Loading