code review fixes

Author: gallayl

service/src/app-models/identity/actions/register-action.spec.ts

@@ -1,15 +1,12 @@
 import type { IncomingMessage, ServerResponse } from 'http'
 import { Injector } from '@furystack/inject'
-import { StoreManager } from '@furystack/core'
 import { RequestError } from '@furystack/rest'
 import { HttpUserContext } from '@furystack/rest-service'
-import { PasswordAuthenticator, PasswordCredential } from '@furystack/security'
+import { PasswordAuthenticator } from '@furystack/security'
 import { usingAsync } from '@furystack/utils'
 import { describe, expect, it, vi } from 'vitest'
-import { User } from 'common'
 import { RegisterAction } from './register-action.js'
 
-// Mock getLogger
 vi.mock('@furystack/logging', () => ({
   getLogger: () => ({
     withScope: () => ({
@@ -20,27 +17,32 @@ vi.mock('@furystack/logging', () => ({
   }),
 }))
 
-describe('RegisterAction', () => {
-  const createTestInjector = () => {
-    const injector = new Injector()
+const mockUserDataSet = {
+  get: vi.fn(),
+  add: vi.fn(),
+  remove: vi.fn(),
+}
 
-    const mockUserStore = {
-      get: vi.fn(),
-      add: vi.fn(),
-      remove: vi.fn(),
-    }
+const mockCredentialDataSet = {
+  add: vi.fn(),
+}
 
-    const mockCredentialStore = {
-      add: vi.fn(),
-    }
+vi.mock('@furystack/core', () => ({
+  useSystemIdentityContext: ({ injector }: { injector: unknown }) => injector,
+}))
 
-    const mockStoreManager = {
-      getStoreFor: vi.fn((model: { name: string }) => {
-        if (model === User || model.name === 'User') return mockUserStore
-        if (model === PasswordCredential || model.name === 'PasswordCredential') return mockCredentialStore
-        throw new Error(`Unknown model: ${model.name}`)
-      }),
-    }
+vi.mock('@furystack/repository', () => ({
+  getDataSetFor: (_injector: unknown, model: { name?: string } | ((...args: unknown[]) => unknown)) => {
+    const name = typeof model === 'function' ? model.name : ''
+    if (name === 'User') return mockUserDataSet
+    if (name === 'PasswordCredential') return mockCredentialDataSet
+    throw new Error(`Unknown model: ${name}`)
+  },
+}))
+
+describe('RegisterAction', () => {
+  const createTestInjector = () => {
+    const injector = new Injector()
 
     const mockHasher = {
       createCredential: vi.fn().mockResolvedValue({
@@ -59,19 +61,18 @@ describe('RegisterAction', () => {
       cookieLogin: vi.fn().mockResolvedValue(undefined),
     }
 
-    injector.setExplicitInstance(mockStoreManager as unknown as StoreManager, StoreManager)
     injector.setExplicitInstance(mockAuthenticator as unknown as PasswordAuthenticator, PasswordAuthenticator)
     injector.setExplicitInstance(mockUserContext as unknown as HttpUserContext, HttpUserContext)
 
-    return { injector, mockUserStore, mockCredentialStore, mockHasher, mockUserContext }
+    return { injector, mockHasher, mockUserContext }
   }
 
   it('should register a new user successfully', async () => {
-    const { injector, mockUserStore, mockCredentialStore, mockUserContext } = createTestInjector()
+    const { injector, mockUserContext } = createTestInjector()
 
-    mockUserStore.get.mockResolvedValue(null) // User doesn't exist
-    mockUserStore.add.mockResolvedValue(undefined)
-    mockCredentialStore.add.mockResolvedValue(undefined)
+    mockUserDataSet.get.mockResolvedValue(null)
+    mockUserDataSet.add.mockResolvedValue(undefined)
+    mockCredentialDataSet.add.mockResolvedValue(undefined)
 
     await usingAsync(injector, async (i) => {
       const result = await RegisterAction({
@@ -81,24 +82,25 @@ describe('RegisterAction', () => {
         request: {} as IncomingMessage,
       })
 
-      expect(mockUserStore.get).toHaveBeenCalledWith('newuser')
-      expect(mockUserStore.add).toHaveBeenCalledWith(
+      expect(mockUserDataSet.get).toHaveBeenCalledWith(i, 'newuser')
+      expect(mockUserDataSet.add).toHaveBeenCalledWith(
+        i,
         expect.objectContaining({
           username: 'newuser',
           roles: [],
         }),
       )
-      expect(mockCredentialStore.add).toHaveBeenCalled()
+      expect(mockCredentialDataSet.add).toHaveBeenCalled()
       expect(mockUserContext.authenticateUser).toHaveBeenCalledWith('newuser', 'password123')
       expect(mockUserContext.cookieLogin).toHaveBeenCalled()
       expect(result.chunk).toEqual({ username: 'newuser', roles: [] })
     })
   })
 
   it('should throw 409 when user already exists', async () => {
-    const { injector, mockUserStore } = createTestInjector()
+    const { injector } = createTestInjector()
 
-    mockUserStore.get.mockResolvedValue({ username: 'existinguser', roles: [] })
+    mockUserDataSet.get.mockResolvedValue({ username: 'existinguser', roles: [] })
 
     await usingAsync(injector, async (i) => {
       try {
@@ -118,9 +120,9 @@ describe('RegisterAction', () => {
   })
 
   it('should throw 400 when credential creation fails', async () => {
-    const { injector, mockUserStore, mockHasher } = createTestInjector()
+    const { injector, mockHasher } = createTestInjector()
 
-    mockUserStore.get.mockResolvedValue(null)
+    mockUserDataSet.get.mockResolvedValue(null)
     mockHasher.createCredential.mockRejectedValue(new Error('Invalid password'))
 
     await usingAsync(injector, async (i) => {
@@ -141,12 +143,12 @@ describe('RegisterAction', () => {
   })
 
   it('should cleanup user when registration fails after user creation', async () => {
-    const { injector, mockUserStore, mockCredentialStore } = createTestInjector()
+    const { injector } = createTestInjector()
 
-    mockUserStore.get.mockResolvedValue(null)
-    mockUserStore.add.mockResolvedValue(undefined)
-    mockCredentialStore.add.mockRejectedValue(new Error('Credential store error'))
-    mockUserStore.remove.mockResolvedValue(undefined)
+    mockUserDataSet.get.mockResolvedValue(null)
+    mockUserDataSet.add.mockResolvedValue(undefined)
+    mockCredentialDataSet.add.mockRejectedValue(new Error('Credential store error'))
+    mockUserDataSet.remove.mockResolvedValue(undefined)
 
     await usingAsync(injector, async (i) => {
       try {
@@ -159,19 +161,18 @@ describe('RegisterAction', () => {
         expect.fail('Expected RequestError to be thrown')
       } catch (error) {
         expect(error).toBeInstanceOf(RequestError)
-        // Verify cleanup was attempted
-        expect(mockUserStore.remove).toHaveBeenCalledWith('newuser')
+        expect(mockUserDataSet.remove).toHaveBeenCalledWith(i, 'newuser')
       }
     })
   })
 
   it('should not fail if cleanup fails', async () => {
-    const { injector, mockUserStore, mockCredentialStore } = createTestInjector()
+    const { injector } = createTestInjector()
 
-    mockUserStore.get.mockResolvedValue(null)
-    mockUserStore.add.mockResolvedValue(undefined)
-    mockCredentialStore.add.mockRejectedValue(new Error('Credential store error'))
-    mockUserStore.remove.mockRejectedValue(new Error('Cleanup failed'))
+    mockUserDataSet.get.mockResolvedValue(null)
+    mockUserDataSet.add.mockResolvedValue(undefined)
+    mockCredentialDataSet.add.mockRejectedValue(new Error('Credential store error'))
+    mockUserDataSet.remove.mockRejectedValue(new Error('Cleanup failed'))
 
     await usingAsync(injector, async (i) => {
       try {
@@ -185,8 +186,7 @@ describe('RegisterAction', () => {
       } catch (error) {
         expect(error).toBeInstanceOf(RequestError)
         expect((error as RequestError).responseCode).toBe(400)
-        // Cleanup was attempted even though it failed
-        expect(mockUserStore.remove).toHaveBeenCalledWith('newuser')
+        expect(mockUserDataSet.remove).toHaveBeenCalledWith(i, 'newuser')
       }
     })
   })

service/src/app-models/media/actions/scan-for-movies-action.ts

@@ -1,3 +1,4 @@
+import { useSystemIdentityContext } from '@furystack/core'
 import { getLogger } from '@furystack/logging'
 import { getDataSetFor } from '@furystack/repository'
 import { RequestError } from '@furystack/rest'
@@ -11,17 +12,18 @@ export const ScanForMoviesAction: RequestAction<ScanForMoviesEndpoint> = async (
   const { root, autoExtractSubtitles } = await getBody()
 
   const logger = getLogger(injector).withScope('ScanForMoviesAction')
+  const systemInjector = useSystemIdentityContext({ injector, username: 'scan-movies' })
 
   const maintainer = injector.getInstance(MovieMaintainerService)
   const driveDataSet = getDataSetFor(injector, Drive, 'letter')
-  const drive = await driveDataSet.get(injector, root.driveLetter)
+  const drive = await driveDataSet.get(systemInjector, root.driveLetter)
 
   if (!drive) {
     throw new RequestError(`Drive ${root.driveLetter} not found`, 400)
   }
 
   const movieFileDataSet = getDataSetFor(injector, MovieFile, 'id')
-  const alreadyAddedMovieFiles = await movieFileDataSet.find(injector, {})
+  const alreadyAddedMovieFiles = await movieFileDataSet.find(systemInjector, {})
 
   await logger.verbose({
     message: `Scanning for movie files in ${root.path} on drive ${drive.letter}`,
@@ -39,13 +41,13 @@ export const ScanForMoviesAction: RequestAction<ScanForMoviesEndpoint> = async (
   for (const file of toBeAdded) {
     try {
       const addedMovieFile = await linkMovie({
-        injector,
+        injector: systemInjector,
         file,
       })
 
       if (autoExtractSubtitles) {
         await extractSubtitles({
-          injector,
+          injector: systemInjector,
           file,
         })
       }

service/src/app-models/media/services/stream-file-action-caches.ts

@@ -1,6 +1,6 @@
 import { Cache } from '@furystack/cache'
 import { useSystemIdentityContext } from '@furystack/core'
-import { Injectable, type Injector } from '@furystack/inject'
+import { Injected, Injectable, type Injector } from '@furystack/inject'
 import { getDataSetFor } from '@furystack/repository'
 import { Config, Drive, type MoviesConfig, type PiRatFile, type StreamQueryParams } from 'common'
 import { join } from 'path'
@@ -12,14 +12,8 @@ import { FfprobeService } from '../../../ffprobe-service.js'
 export class StreamFileActionCaches {
   declare public injector: Injector
 
-  private get systemInjector() {
-    if (!this._systemInjector) {
-      this._systemInjector = useSystemIdentityContext({ injector: this.injector, username: 'stream-cache' })
-    }
-    return this._systemInjector
-  }
-
-  private _systemInjector?: Injector
+  @Injected((injector) => useSystemIdentityContext({ injector, username: 'stream-cache' }))
+  declare private systemInjector: Injector
 
   public driveCache = new Cache({
     load: async (key: string) => {

service/src/app-models/media/utils/ensure-movie-exists.spec.ts

@@ -4,16 +4,13 @@ import { describe, expect, it, vi } from 'vitest'
 import type { OmdbMovieMetadata } from 'common'
 import { ensureMovieExists } from './ensure-movie-exists.js'
 
-// Mock getStoreManager
 const mockMovieStoreGet = vi.fn()
 const mockMovieStoreAdd = vi.fn()
 
-vi.mock('@furystack/core', () => ({
-  getStoreManager: () => ({
-    getStoreFor: () => ({
-      get: (...args: unknown[]) => mockMovieStoreGet(...args) as unknown,
-      add: (...args: unknown[]) => mockMovieStoreAdd(...args) as unknown,
-    }),
+vi.mock('@furystack/repository', () => ({
+  getDataSetFor: () => ({
+    get: (...args: unknown[]) => mockMovieStoreGet(...args) as unknown,
+    add: (...args: unknown[]) => mockMovieStoreAdd(...args) as unknown,
   }),
 }))
 
@@ -56,7 +53,7 @@ describe('ensureMovieExists', () => {
     await usingAsync(new Injector(), async (injector) => {
       const result = await ensureMovieExists(createOmdbMeta(), injector)
 
-      expect(mockMovieStoreGet).toHaveBeenCalledWith('tt1234567')
+      expect(mockMovieStoreGet).toHaveBeenCalledWith(injector, 'tt1234567')
       expect(mockMovieStoreAdd).not.toHaveBeenCalled()
       expect(result).toBe(existingMovie)
     })
@@ -74,8 +71,9 @@ describe('ensureMovieExists', () => {
     await usingAsync(new Injector(), async (injector) => {
       const result = await ensureMovieExists(createOmdbMeta(), injector)
 
-      expect(mockMovieStoreGet).toHaveBeenCalledWith('tt1234567')
+      expect(mockMovieStoreGet).toHaveBeenCalledWith(injector, 'tt1234567')
       expect(mockMovieStoreAdd).toHaveBeenCalledWith(
+        injector,
         expect.objectContaining({
           imdbId: 'tt1234567',
           title: 'Test Movie',
@@ -111,6 +109,7 @@ describe('ensureMovieExists', () => {
       await ensureMovieExists(omdbMeta, injector)
 
       expect(mockMovieStoreAdd).toHaveBeenCalledWith(
+        injector,
         expect.objectContaining({
           imdbId: 'tt9999999',
           season: 1,
@@ -132,6 +131,7 @@ describe('ensureMovieExists', () => {
       await ensureMovieExists(omdbMeta, injector)
 
       expect(mockMovieStoreAdd).toHaveBeenCalledWith(
+        injector,
         expect.objectContaining({
           duration: undefined,
         }),

service/src/app-models/media/utils/link-movie.spec.ts

@@ -7,28 +7,26 @@ import { FfprobeService } from '../../../ffprobe-service.js'
 import { OmdbClientService } from '../metadata-services/omdb-client-service.js'
 import { linkMovie } from './link-movie.js'
 
-// Mock getStoreManager
 const mockMovieFileStoreFind = vi.fn()
 const mockMovieFileStoreAdd = vi.fn()
 const mockOmdbStoreFind = vi.fn()
 
-vi.mock('@furystack/core', () => ({
-  getStoreManager: () => ({
-    getStoreFor: (model: { name: string }) => {
-      if (model.name === 'MovieFile') {
-        return {
-          find: (...args: unknown[]) => mockMovieFileStoreFind(...args) as unknown,
-          add: (...args: unknown[]) => mockMovieFileStoreAdd(...args) as unknown,
-        }
+vi.mock('@furystack/repository', () => ({
+  getDataSetFor: (_injector: unknown, model: { name?: string } | ((...args: unknown[]) => unknown)) => {
+    const name = typeof model === 'function' ? model.name : ''
+    if (name === 'MovieFile') {
+      return {
+        find: (...args: unknown[]) => mockMovieFileStoreFind(...args) as unknown,
+        add: (...args: unknown[]) => mockMovieFileStoreAdd(...args) as unknown,
       }
-      if (model.name === 'OmdbMovieMetadata') {
-        return {
-          find: (...args: unknown[]) => mockOmdbStoreFind(...args) as unknown,
-        }
+    }
+    if (name === 'OmdbMovieMetadata') {
+      return {
+        find: (...args: unknown[]) => mockOmdbStoreFind(...args) as unknown,
       }
-      return {}
-    },
-  }),
+    }
+    return {}
+  },
 }))
 
 // Mock getLogger