diff --git a/.tangled/workflows/release.yml b/.tangled/workflows/release.yml index 5b5f18d..fa4a444 100644 --- a/.tangled/workflows/release.yml +++ b/.tangled/workflows/release.yml @@ -11,7 +11,7 @@ architecture: amd64 environment: IMAGE_REGISTRY: atcr.io - IMAGE_USER: evan.jarrett.net + IMAGE_USER: atcr.io steps: - name: Login to registry diff --git a/pkg/appview/storage/routing_repository.go b/pkg/appview/storage/routing_repository.go index c19471e..70cf324 100644 --- a/pkg/appview/storage/routing_repository.go +++ b/pkg/appview/storage/routing_repository.go @@ -64,18 +64,26 @@ func (r *RoutingRepository) Blobs(ctx context.Context) distribution.BlobStore { return blobStore } - // For pull operations, check database for hold DID from the most recent manifest - // This ensures blobs are fetched from the hold recorded in the manifest, not re-discovered + // Determine if this is a pull (GET) or push (PUT/POST/HEAD/etc) operation + // Pull operations use the historical hold DID from the database (blobs are where they were pushed) + // Push operations use the discovery-based hold DID from user's profile/default + // This allows users to change their default hold and have new pushes go there + isPull := false + if method, ok := ctx.Value("http.request.method").(string); ok { + isPull = method == "GET" + } + holdDID := r.Ctx.HoldDID // Default to discovery-based DID holdSource := "discovery" - if r.Ctx.Database != nil { + // Only query database for pull operations + if isPull && r.Ctx.Database != nil { // Query database for the latest manifest's hold DID if dbHoldDID, err := r.Ctx.Database.GetLatestHoldDIDForRepo(r.Ctx.DID, r.Ctx.Repository); err == nil && dbHoldDID != "" { // Use hold DID from database (pull case - use historical reference) holdDID = dbHoldDID holdSource = "database" - slog.Debug("Using hold from database manifest", "component", "storage/blobs", "did", r.Ctx.DID, "repo", r.Ctx.Repository, "hold", dbHoldDID) + slog.Debug("Using hold from database manifest (pull)", "component", "storage/blobs", "did", r.Ctx.DID, "repo", r.Ctx.Repository, "hold", dbHoldDID) } else if err != nil { // Log error but don't fail - fall back to discovery-based DID slog.Warn("Failed to query database for hold DID", "component", "storage/blobs", "error", err) diff --git a/pkg/appview/storage/routing_repository_test.go b/pkg/appview/storage/routing_repository_test.go index 3685806..99bc99d 100644 --- a/pkg/appview/storage/routing_repository_test.go +++ b/pkg/appview/storage/routing_repository_test.go @@ -109,24 +109,90 @@ func TestRoutingRepository_ManifestStoreCaching(t *testing.T) { assert.NotNil(t, repo.manifestStore) } -// TestRoutingRepository_Blobs_WithDatabase tests blob store with database hold DID -func TestRoutingRepository_Blobs_WithDatabase(t *testing.T) { +// TestRoutingRepository_Blobs_PullUsesDatabase tests that GET (pull) uses database hold DID +func TestRoutingRepository_Blobs_PullUsesDatabase(t *testing.T) { dbHoldDID := "did:web:database.hold.io" + discoveryHoldDID := "did:web:discovery.hold.io" ctx := &RegistryContext{ DID: "did:plc:test123", Repository: "myapp", - HoldDID: "did:web:default.hold.io", // Discovery-based hold (should be overridden) + HoldDID: discoveryHoldDID, // Discovery-based hold (should be overridden for pull) ATProtoClient: atproto.NewClient("https://pds.example.com", "did:plc:test123", ""), Database: &mockDatabase{holdDID: dbHoldDID}, } repo := NewRoutingRepository(nil, ctx) + + // Create context with GET method (pull operation) + pullCtx := context.WithValue(context.Background(), "http.request.method", "GET") + blobStore := repo.Blobs(pullCtx) + + assert.NotNil(t, blobStore) + // Verify the hold DID was updated to use the database value for pull + assert.Equal(t, dbHoldDID, repo.Ctx.HoldDID, "pull (GET) should use database hold DID") +} + +// TestRoutingRepository_Blobs_PushUsesDiscovery tests that push operations use discovery hold DID +func TestRoutingRepository_Blobs_PushUsesDiscovery(t *testing.T) { + dbHoldDID := "did:web:database.hold.io" + discoveryHoldDID := "did:web:discovery.hold.io" + + testCases := []struct { + name string + method string + }{ + {"PUT", "PUT"}, + {"POST", "POST"}, + {"HEAD", "HEAD"}, + {"PATCH", "PATCH"}, + {"DELETE", "DELETE"}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ctx := &RegistryContext{ + DID: "did:plc:test123", + Repository: "myapp-" + tc.method, // Unique repo to avoid caching + HoldDID: discoveryHoldDID, + ATProtoClient: atproto.NewClient("https://pds.example.com", "did:plc:test123", ""), + Database: &mockDatabase{holdDID: dbHoldDID}, + } + + repo := NewRoutingRepository(nil, ctx) + + // Create context with push method + pushCtx := context.WithValue(context.Background(), "http.request.method", tc.method) + blobStore := repo.Blobs(pushCtx) + + assert.NotNil(t, blobStore) + // Verify the hold DID remains the discovery-based one for push operations + assert.Equal(t, discoveryHoldDID, repo.Ctx.HoldDID, "%s should use discovery hold DID, not database", tc.method) + }) + } +} + +// TestRoutingRepository_Blobs_NoMethodUsesDiscovery tests that missing method defaults to discovery +func TestRoutingRepository_Blobs_NoMethodUsesDiscovery(t *testing.T) { + dbHoldDID := "did:web:database.hold.io" + discoveryHoldDID := "did:web:discovery.hold.io" + + ctx := &RegistryContext{ + DID: "did:plc:test123", + Repository: "myapp-nomethod", + HoldDID: discoveryHoldDID, + ATProtoClient: atproto.NewClient("https://pds.example.com", "did:plc:test123", ""), + Database: &mockDatabase{holdDID: dbHoldDID}, + } + + repo := NewRoutingRepository(nil, ctx) + + // Context without HTTP method (shouldn't happen in practice, but test defensive behavior) blobStore := repo.Blobs(context.Background()) assert.NotNil(t, blobStore) - // Verify the hold DID was updated to use the database value - assert.Equal(t, dbHoldDID, repo.Ctx.HoldDID, "should use database hold DID") + // Without method, should default to discovery (safer for push scenarios) + assert.Equal(t, discoveryHoldDID, repo.Ctx.HoldDID, "missing method should use discovery hold DID") } // TestRoutingRepository_Blobs_WithoutDatabase tests blob store with discovery-based hold @@ -292,23 +358,26 @@ func TestRoutingRepository_ConcurrentAccess(t *testing.T) { assert.NotNil(t, cachedBlobStore) } -// TestRoutingRepository_Blobs_Priority tests that database hold DID takes priority over discovery -func TestRoutingRepository_Blobs_Priority(t *testing.T) { +// TestRoutingRepository_Blobs_PullPriority tests that database hold DID takes priority for pull (GET) +func TestRoutingRepository_Blobs_PullPriority(t *testing.T) { dbHoldDID := "did:web:database.hold.io" discoveryHoldDID := "did:web:discovery.hold.io" ctx := &RegistryContext{ DID: "did:plc:test123", - Repository: "myapp", + Repository: "myapp-priority", HoldDID: discoveryHoldDID, // Discovery-based hold ATProtoClient: atproto.NewClient("https://pds.example.com", "did:plc:test123", ""), Database: &mockDatabase{holdDID: dbHoldDID}, // Database has a different hold DID } repo := NewRoutingRepository(nil, ctx) - blobStore := repo.Blobs(context.Background()) + + // For pull (GET), database should take priority + pullCtx := context.WithValue(context.Background(), "http.request.method", "GET") + blobStore := repo.Blobs(pullCtx) assert.NotNil(t, blobStore) - // Database hold DID should take priority over discovery - assert.Equal(t, dbHoldDID, repo.Ctx.HoldDID, "database hold DID should take priority over discovery") + // Database hold DID should take priority over discovery for pull operations + assert.Equal(t, dbHoldDID, repo.Ctx.HoldDID, "database hold DID should take priority over discovery for pull (GET)") }