From 0f46d5c57ad6eba4496e93e694a0e34b010b2f30 Mon Sep 17 00:00:00 2001 From: Jeremy Schoemaker Date: Sun, 23 Aug 2026 17:23:13 -0500 Subject: [PATCH] fix(artifacts): honor the "user:" namespace in InMemoryArtifactService GcsArtifactService treats a "user:"-prefixed filename as scoped to the user across all sessions: fileHasUserNamespace() (GcsArtifactService.java:65) makes getBlobPrefix() (GcsArtifactService.java:78-84) store at appName/userId/user/filename/version, deliberately leaving sessionId out. This is a documented convention and is covered by GcsArtifactServiceTest#save_userNamespace_savesCorrectly and #load_userNamespace_loadsCorrectly. InMemoryArtifactService had no equivalent branch. getArtifactsMap always keyed by appName -> userId -> sessionId -> filename, so a "user:" artifact saved in one session was invisible from another session for the same app/user, with no error - just an empty result. Since InMemoryArtifactService is the default backend for local dev, tests, and quickstarts, this meant the documented cross-session behavior silently broke the moment someone swapped in the in-memory service instead of GCS. Mirror the GCS branch: getArtifactsMap now takes the filename and routes user-namespaced ones to a shared per-user bucket instead of the session's, using the same fileHasUserNamespace() check and naming as GcsArtifactService. listArtifactKeys merges that shared bucket with the session-scoped one, matching GcsArtifactService#listArtifactKeys, which merges its session-prefix and user-prefix listings. Added tests mirroring GcsArtifactServiceTest's user-namespace cases for save/load, listArtifactKeys, listVersions, and deleteArtifact, plus a regression guard proving a non-prefixed filename saved in one session stays invisible from another - i.e. this fix scopes only the "user:" case and does not make all artifacts global. --- .../artifacts/InMemoryArtifactService.java | 52 ++++++++++-- .../InMemoryArtifactServiceTest.java | 82 +++++++++++++++++++ 2 files changed, 125 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/com/google/adk/artifacts/InMemoryArtifactService.java b/core/src/main/java/com/google/adk/artifacts/InMemoryArtifactService.java index 510c96c2e..5cd03a755 100644 --- a/core/src/main/java/com/google/adk/artifacts/InMemoryArtifactService.java +++ b/core/src/main/java/com/google/adk/artifacts/InMemoryArtifactService.java @@ -26,19 +26,36 @@ import io.reactivex.rxjava3.core.Single; import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.stream.IntStream; import org.jspecify.annotations.Nullable; /** An in-memory implementation of the {@link BaseArtifactService}. */ public final class InMemoryArtifactService implements BaseArtifactService { + // Mirrors the "user" path segment GcsArtifactService.getBlobPrefix uses for user-namespaced + // filenames: a stand-in session key so those artifacts are stored and looked up independently + // of whatever sessionId happened to be current when they were saved. + private static final String USER_NAMESPACE_SESSION_KEY = "user"; + private final Map>>>> artifacts; public InMemoryArtifactService() { this.artifacts = new HashMap<>(); } + /** + * Checks if a filename uses the user namespace. + * + * @param filename Filename to check. + * @return true if prefixed with "user:", false otherwise. + */ + private boolean fileHasUserNamespace(String filename) { + return filename != null && filename.startsWith("user:"); + } + /** * Saves an artifact in memory and assigns a new version. * @@ -48,7 +65,7 @@ public InMemoryArtifactService() { public Single saveArtifact( String appName, String userId, String sessionId, String filename, Part artifact) { List versions = - getArtifactsMap(appName, userId, sessionId) + getArtifactsMap(appName, userId, sessionId, filename) .computeIfAbsent(filename, unused -> new ArrayList<>()); versions.add(artifact); return Single.just(versions.size() - 1); @@ -63,7 +80,7 @@ public Single saveArtifact( public Maybe loadArtifact( String appName, String userId, String sessionId, String filename, @Nullable Integer version) { List versions = - getArtifactsMap(appName, userId, sessionId) + getArtifactsMap(appName, userId, sessionId, filename) .computeIfAbsent(filename, unused -> new ArrayList<>()); if (versions.isEmpty()) { @@ -88,10 +105,15 @@ public Maybe loadArtifact( @Override public Single listArtifactKeys( String appName, String userId, String sessionId) { + Set filenames = new HashSet<>(); + // Session-scoped filenames, keyed by the actual sessionId. + filenames.addAll(getArtifactsMapForSessionKey(appName, userId, sessionId).keySet()); + // User-namespaced filenames live under the shared USER_NAMESPACE_SESSION_KEY bucket + // regardless of sessionId, mirroring GcsArtifactService's merged session+user listing. + filenames.addAll( + getArtifactsMapForSessionKey(appName, userId, USER_NAMESPACE_SESSION_KEY).keySet()); return Single.just( - ListArtifactsResponse.builder() - .filenames(ImmutableList.copyOf(getArtifactsMap(appName, userId, sessionId).keySet())) - .build()); + ListArtifactsResponse.builder().filenames(ImmutableList.copyOf(filenames)).build()); } /** @@ -102,7 +124,7 @@ public Single listArtifactKeys( @Override public Completable deleteArtifact( String appName, String userId, String sessionId, String filename) { - getArtifactsMap(appName, userId, sessionId).remove(filename); + getArtifactsMap(appName, userId, sessionId, filename).remove(filename); return Completable.complete(); } @@ -115,7 +137,7 @@ public Completable deleteArtifact( public Single> listVersions( String appName, String userId, String sessionId, String filename) { int size = - getArtifactsMap(appName, userId, sessionId) + getArtifactsMap(appName, userId, sessionId, filename) .computeIfAbsent(filename, unused -> new ArrayList<>()) .size(); if (size == 0) { @@ -131,10 +153,22 @@ public Single saveAndReloadArtifact( .flatMap(version -> loadArtifact(appName, userId, sessionId, filename, version).toSingle()); } - private Map> getArtifactsMap(String appName, String userId, String sessionId) { + /** + * Resolves the artifacts map for a filename, routing user-namespaced filenames to the shared + * {@link #USER_NAMESPACE_SESSION_KEY} bucket instead of the given session, so they are visible + * across all of a user's sessions exactly as {@link GcsArtifactService} stores them. + */ + private Map> getArtifactsMap( + String appName, String userId, String sessionId, String filename) { + String sessionKey = fileHasUserNamespace(filename) ? USER_NAMESPACE_SESSION_KEY : sessionId; + return getArtifactsMapForSessionKey(appName, userId, sessionKey); + } + + private Map> getArtifactsMapForSessionKey( + String appName, String userId, String sessionKey) { return artifacts .computeIfAbsent(appName, unused -> new HashMap<>()) .computeIfAbsent(userId, unused -> new HashMap<>()) - .computeIfAbsent(sessionId, unused -> new HashMap<>()); + .computeIfAbsent(sessionKey, unused -> new HashMap<>()); } } diff --git a/core/src/test/java/com/google/adk/artifacts/InMemoryArtifactServiceTest.java b/core/src/test/java/com/google/adk/artifacts/InMemoryArtifactServiceTest.java index 124a5e9d8..9a5e4c4f9 100644 --- a/core/src/test/java/com/google/adk/artifacts/InMemoryArtifactServiceTest.java +++ b/core/src/test/java/com/google/adk/artifacts/InMemoryArtifactServiceTest.java @@ -17,6 +17,7 @@ import static com.google.common.truth.Truth.assertThat; +import com.google.common.collect.ImmutableList; import com.google.genai.types.Part; import io.reactivex.rxjava3.core.Maybe; import io.reactivex.rxjava3.core.Single; @@ -34,6 +35,9 @@ public class InMemoryArtifactServiceTest { private static final String USER_ID = "test-user"; private static final String SESSION_ID = "test-session"; private static final String FILENAME = "test-file.txt"; + private static final String USER_FILENAME = "user:config.json"; + private static final String SESSION_A = "session-A"; + private static final String SESSION_B = "session-B"; private InMemoryArtifactService service; @@ -72,6 +76,84 @@ public void saveAndReloadArtifact_reloadsArtifact() { assertThat(result).hasValue(artifact); } + @Test + public void save_userNamespace_visibleAcrossSessions() { + // "user:"-prefixed filenames are documented/tested (GcsArtifactServiceTest) as living in a + // session-independent namespace: GcsArtifactService.getBlobPrefix special-cases + // fileHasUserNamespace(filename) to build "appName/userId/user/filename/" (no sessionId + // segment at all). InMemoryArtifactService must mirror that: a "user:" artifact saved in one + // session has to be readable from a different session for the same app/user. + Part artifact = Part.fromBytes(new byte[] {9, 9, 9}, "application/json"); + + var unused = + service.saveArtifact(APP_NAME, USER_ID, SESSION_A, USER_FILENAME, artifact).blockingGet(); + + Optional resultFromOtherSession = + asOptional(service.loadArtifact(APP_NAME, USER_ID, SESSION_B, USER_FILENAME)); + + assertThat(resultFromOtherSession).hasValue(artifact); + } + + @Test + public void load_nonNamespacedFilename_notVisibleAcrossSessions() { + // Regression guard: the user-namespace fix must not make every artifact global. A + // non-prefixed filename saved in one session must remain invisible from a different session, + // exactly as before the fix. + Part artifact = Part.fromBytes(new byte[] {1, 2, 3}, "text/plain"); + + var unused = + service.saveArtifact(APP_NAME, USER_ID, SESSION_A, FILENAME, artifact).blockingGet(); + + Optional resultFromOtherSession = + asOptional(service.loadArtifact(APP_NAME, USER_ID, SESSION_B, FILENAME)); + + assertThat(resultFromOtherSession).isEmpty(); + } + + @Test + public void listArtifactKeys_userNamespace_visibleAcrossSessions() { + Part artifact = Part.fromBytes(new byte[] {9, 9, 9}, "application/json"); + + var unused = + service.saveArtifact(APP_NAME, USER_ID, SESSION_A, USER_FILENAME, artifact).blockingGet(); + + ListArtifactsResponse response = + service.listArtifactKeys(APP_NAME, USER_ID, SESSION_B).blockingGet(); + + assertThat(response.filenames()).contains(USER_FILENAME); + } + + @Test + public void listVersions_userNamespace_visibleAcrossSessions() { + Part artifact1 = Part.fromBytes(new byte[] {1}, "application/json"); + Part artifact2 = Part.fromBytes(new byte[] {1, 2}, "application/json"); + + var unused1 = + service.saveArtifact(APP_NAME, USER_ID, SESSION_A, USER_FILENAME, artifact1).blockingGet(); + var unused2 = + service.saveArtifact(APP_NAME, USER_ID, SESSION_A, USER_FILENAME, artifact2).blockingGet(); + + ImmutableList versions = + service.listVersions(APP_NAME, USER_ID, SESSION_B, USER_FILENAME).blockingGet(); + + assertThat(versions).containsExactly(0, 1).inOrder(); + } + + @Test + public void deleteArtifact_userNamespace_removesAcrossSessions() { + Part artifact = Part.fromBytes(new byte[] {9, 9, 9}, "application/json"); + + var unused = + service.saveArtifact(APP_NAME, USER_ID, SESSION_A, USER_FILENAME, artifact).blockingGet(); + + service.deleteArtifact(APP_NAME, USER_ID, SESSION_B, USER_FILENAME).blockingAwait(); + + Optional resultFromOriginalSession = + asOptional(service.loadArtifact(APP_NAME, USER_ID, SESSION_A, USER_FILENAME)); + + assertThat(resultFromOriginalSession).isEmpty(); + } + private static Optional asOptional(Maybe maybe) { return maybe.map(Optional::of).defaultIfEmpty(Optional.empty()).blockingGet(); }