From 828d25ffd0a48a6fbf877e84912d666d96922f70 Mon Sep 17 00:00:00 2001 From: Zack Stout Date: Fri, 17 Jul 2026 08:27:50 -0500 Subject: [PATCH 01/10] refactor: split monolithic Common.lua into focused, testable modules - Extract OllamaClient.lua for API communication and server validation - Extract ThumbnailService.lua for thumbnail export and Base64 encoding - Extract PromptBuilder.lua for user/system prompt assembly - Extract ResponseValidator.lua for API response parsing and validation - Extract MetadataService.lua for keyword CSV parsing - Refactor Common.lua to delegation layer that re-exports original names - Add Busted test suite with 62 tests across 5 spec files - Makefile delegates to run_tests.sh as authoritative runner - Test paths resolved dynamically via debug.getinfo for portability --- Makefile | 10 + lightroom-llama.lrplugin/Common.lua | 622 ++---------------- lightroom-llama.lrplugin/LrLlama.lua | 54 +- lightroom-llama.lrplugin/MetadataService.lua | 117 ++++ lightroom-llama.lrplugin/OllamaClient.lua | 260 ++++++++ lightroom-llama.lrplugin/PromptBuilder.lua | 180 +++++ .../ResponseValidator.lua | 105 +++ lightroom-llama.lrplugin/ThumbnailService.lua | 122 ++++ tests/busted.json | 4 + tests/helpers/mock_sdk.lua | 56 ++ tests/run_tests.sh | 13 + tests/spec/common_delegation_spec.lua | 86 +++ tests/spec/metadata_service_parse_spec.lua | 51 ++ tests/spec/ollama_client_validate_spec.lua | 119 ++++ tests/spec/prompt_builder_spec.lua | 139 ++++ tests/spec/response_validator_spec.lua | 146 ++++ 16 files changed, 1452 insertions(+), 632 deletions(-) create mode 100644 Makefile create mode 100644 lightroom-llama.lrplugin/MetadataService.lua create mode 100644 lightroom-llama.lrplugin/OllamaClient.lua create mode 100644 lightroom-llama.lrplugin/PromptBuilder.lua create mode 100644 lightroom-llama.lrplugin/ResponseValidator.lua create mode 100644 lightroom-llama.lrplugin/ThumbnailService.lua create mode 100644 tests/busted.json create mode 100644 tests/helpers/mock_sdk.lua create mode 100755 tests/run_tests.sh create mode 100644 tests/spec/common_delegation_spec.lua create mode 100644 tests/spec/metadata_service_parse_spec.lua create mode 100644 tests/spec/ollama_client_validate_spec.lua create mode 100644 tests/spec/prompt_builder_spec.lua create mode 100644 tests/spec/response_validator_spec.lua diff --git a/Makefile b/Makefile new file mode 100644 index 0000000..2b83a11 --- /dev/null +++ b/Makefile @@ -0,0 +1,10 @@ +.PHONY: test clean + +# Run the Busted unit test suite for plugin modules. +# Delegates to run_tests.sh (the authoritative runner) so flags are only defined once. +test: + ./tests/run_tests.sh + +# Remove generated/test artifacts +clean: + rm -f tests/spec/*.gc* 2>/dev/null || true diff --git a/lightroom-llama.lrplugin/Common.lua b/lightroom-llama.lrplugin/Common.lua index 068a419..fe1eeae 100644 --- a/lightroom-llama.lrplugin/Common.lua +++ b/lightroom-llama.lrplugin/Common.lua @@ -1,587 +1,47 @@ --- Common.lua — Shared utilities for Lightroom Llama plugin ---- Loaded by LrLlama.lua, BatchLrLlama.lua, and ResetMetadata.lua. Provides thumbnail ---- export, Ollama API communication, model discovery, keyword management, and server ---- address validation. All public functions are exported via the return table at EOF. ---- ---- SDK imports: LrHttp, LrLogger, LrFileUtils, LrStringUtils, LrTasks, LrPrefs +-- Common.lua — Shared utilities for Lightroom Llama plugin (delegation layer). +--- Loads the focused modules and re-exports them under the original names used by +--- LrLlama.lua, BatchLrLlama.lua, and ResetMetadata.lua. This allows a zero-downtime +--- migration: entry points continue calling Common.X() while logic lives in dedicated +--- modules (OllamaClient, ThumbnailService, PromptBuilder, ResponseValidator, +--- MetadataService). Once verified, the delegation wrappers can be removed and +--- entry points updated to load modules directly. -local LrHttp = import 'LrHttp' -local LrPathUtils = import 'LrPathUtils' -local LrLogger = import 'LrLogger' -local LrFileUtils = import 'LrFileUtils' -local LrStringUtils = import 'LrStringUtils' -local LrTasks = import "LrTasks" local LrPrefs = import "LrPrefs" +local LrPathUtils = import 'LrPathUtils' -local logger = LrLogger('LrLlama') -logger:enable("logfile") -- Logs to ~/Documents/LrClassicLogs | tail -f LrLlama.log - ---- Default Ollama model name; change here to switch models. ----@type string -local model = "gemma4:latest" - ---- Fallback server address when prefs are missing, empty, or invalid. ----@type string -local defaultServerHost = "localhost:11434" - --------------------------------------------------------------------------------- --- Validates that a string is in "host:port" form (IP, hostname, or localhost) --- Returns (true, normalized_host) or (false, error_message) --------------------------------------------------------------------------------- - ---- Validate and normalize an Ollama server address. ---- Strips optional scheme (`http://`) and trailing slashes, then verifies the ---- string is `hostname:port`. Accepts localhost, IP addresses, and domain names. ----@param input string|nil User-supplied server address (may be empty) ----@return bool ok True when the address is valid ----@return string result The normalized host:port on success, error message on failure -local function validateServerHost(input) - if not input or input == "" then - return true, defaultServerHost -- treat empty as "use default" - end - -- Strip scheme and trailing slashes for normalization - local host = string.gsub(input, "^https?://", "") - host = string.gsub(host, "/+$", "") - - -- Must match something:digit (hostname or IP followed by colon and port) - local hostnamePart, portPart = string.match(host, "^([^:]+):(%d+)$") - if not hostnamePart or not portPart then - return false, "Server address must be in host:port format (e.g., localhost:11434)" - end - - local port = tonumber(portPart) - if port < 1 or port > 65535 then - return false, "Port number must be between 1 and 65535" - end - - -- hostnamePart should contain at least one valid character (alphanumeric, dot, hyphen) - if not string.match(hostnamePart, "^[%a%d%.%-]+$") then - return false, "Hostname contains invalid characters" - end - - return true, hostnamePart .. ":" .. portPart -end - ---- Build the base URL for Ollama API calls. ---- Reads `prefs.ollamaServerHost`, validates it, and falls back to ---- `defaultServerHost` if the stored value is malformed. Always returns a valid URL. ----@return string Base URL (e.g., "http://localhost:11434") -local function getOllamaBaseUrl() - local prefs = LrPrefs.prefsForPlugin() - local ok, host = validateServerHost(prefs.ollamaServerHost) - - if not ok then - logger:warn("Invalid server host in prefs: " .. tostring(prefs.ollamaServerHost)) - host = defaultServerHost - end - - return "http://" .. host -end - -JSON = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "JSON.lua"))))() - --------------------------------------------------------------------------------- --- Model discovery --------------------------------------------------------------------------------- - ---- Convert a plain list of model names into the `{title, value}` format that ---- LrView `popup_menu` requires for its `items` binding. ----@param modelNames table List of model strings (from fetchAvailableModels) ----@return table Table of `{title = m, value = m}` entries for popup_menu binding -local function makeModelItems(modelNames) - local items = {} - for _, m in ipairs(modelNames) do - table.insert(items, { title = m, value = m }) - end - return items -end - ---- Query Ollama's `/api/tags` endpoint for installed models. ---- Falls back to `{model}` (the default) on network errors, malformed JSON, ---- or an empty model list — so callers never receive nil. ----@return table List of model name strings; always non-empty. -local function fetchAvailableModels() - logger:info("Fetching available models from Ollama") - - local url = getOllamaBaseUrl() .. "/api/tags" - local response = LrHttp.get(url) - - if response then - local decodeSuccess, data = pcall(function() - return JSON:decode(response) - end) - - if decodeSuccess and data and type(data.models) == "table" then - local modelList = {} - - for _, availableModel in ipairs(data.models) do - local modelName = availableModel.name or availableModel.model - - if modelName and modelName ~= "" then - table.insert(modelList, modelName) - end - end - - if #modelList > 0 then - logger:info("Found " .. #modelList .. " models") - return modelList - end - - logger:warn("Ollama returned an empty model list") - elseif not decodeSuccess then - logger:warn( - "Failed to decode Ollama model response: " .. tostring(data) - ) - else - logger:warn("Ollama response did not contain a valid model list") - end - else - logger:warn("No response received from Ollama") - end - - logger:warn("Using default model: " .. model) - return { model } -end - ---- Persist the server host and rebuild the model dropdown. ---- MUST be called from within `LrTasks.startAsyncTask()` — LrBinding only ---- propagates property reassignments (modelItems, selectedModel) when done ---- off the UI thread. ----@param props table LrBinding property table containing serverHost, modelItems, selectedModel ----@param prefs table LrPrefs prefsForPlugin() instance ----@return bool ok True on success ----@return string message Human-readable status or error description -local function saveServerAndRefresh(props, prefs) - local ok, validatedHost = validateServerHost(props.serverHost) - if not ok then - return false, validatedHost -- caller displays error - end - - prefs.ollamaServerHost = validatedHost - - local availableModels = fetchAvailableModels() - if not availableModels or #availableModels == 0 then - return false, "No models found on server" - end - - -- Rebuild model list. This function is intended to be called from inside - -- LrTasks.startAsyncTask() so that LrBinding picks up the property - -- reassignment and propagates it to the dialog's popup_menu. - local oldSelection = props.selectedModel or "" - props.modelItems = makeModelItems(availableModels) - - -- Keep old selection if it still exists in the new list - local found = false - for _, m in ipairs(availableModels) do - if m == oldSelection then found = true; break; end - end - props.selectedModel = found and oldSelection or availableModels[1] - - return true, string.format("Loaded %d model(s)", #availableModels) -end - ---- Export a 512×512 JPEG thumbnail to a unique temp file. ---- **Side effect:** writes a file to the system temp directory. ---- Caller is responsible for deleting the returned file after use. ----@param photo LrPhoto Photo object from the catalog ----@return string|nil Absolute path on success, nil on failure -local function exportThumbnail(photo) - local tempPath = LrFileUtils.chooseUniqueFileName(LrPathUtils.getStandardFilePath('temp') .. "/thumbnail.jpg") - logger:info("Attempting to export thumbnail to: " .. tempPath) - - -- Validate that the temp directory exists and is accessible before proceeding - -- This prevents silent failures when the temp directory is missing or restricted - local tempDir = LrPathUtils.getStandardFilePath('temp') - if not LrFileUtils.exists(tempDir) then - logger:error("Temp directory does not exist: " .. tempDir) - return nil - end - - -- Track whether the thumbnail callback successfully wrote data - -- This flag is set inside the callback to communicate success back to the caller - local thumbnailSaved = false - -- Request a 512x512 JPEG thumbnail asynchronously - -- photo:requestJpegThumbnail() returns (success, result) and executes the callback - -- with the JPEG binary data if available - local success, result = photo:requestJpegThumbnail(512, 512, function(jpegData) - if jpegData then - -- Open temp file in binary write mode ("wb") to preserve JPEG data integrity - local tempFile = io.open(tempPath, "wb") - if tempFile then - tempFile:write(jpegData) - tempFile:close() - thumbnailSaved = true - logger:info("Thumbnail saved to " .. tempPath) - return true - else - logger:error("Could not open temp file for writing: " .. tempPath) - return false - end - else - logger:error("No JPEG data received from photo") - return false - end - end) - - -- Verify both the API call succeeded AND the callback wrote the file - if success and thumbnailSaved then - -- Final verification: ensure the file actually exists on disk - -- This catches edge cases where the write appeared successful but the file is missing - if LrFileUtils.exists(tempPath) then - logger:info("Thumbnail export successful: " .. tempPath) - return tempPath - else - logger:error("Thumbnail file was not created: " .. tempPath) - return nil - end - else - logger:warn("Failed to export thumbnail. Success: " .. tostring(success) .. ", Result: " .. tostring(result)) - return nil - end -end - --------------------------------------------------------------------------------- --- Base64 encoding --------------------------------------------------------------------------------- - ---- Read a JPEG file and return its Base64-encoded representation. ----@param imagePath string Absolute path to the JPEG file ----@return string|nil Base64 string on success, nil if file unreadable or empty -local function base64EncodeImage(imagePath) - logger:info("Attempting to encode image: " .. imagePath) - - -- Check if file exists - if not LrFileUtils.exists(imagePath) then - logger:error("Image file does not exist: " .. imagePath) - return nil - end - - local file = io.open(imagePath, "rb") - if not file then - logger:error("Could not open file for reading: " .. imagePath) - return nil - end - - local binaryData = file:read("*all") - file:close() - - if not binaryData or #binaryData == 0 then - logger:error("No data read from file: " .. imagePath) - return nil - end - - local base64Data = LrStringUtils.encodeBase64(binaryData) - if not base64Data then - logger:error("Failed to encode image to base64: " .. imagePath) - return nil - end - - logger:info("Successfully encoded image to base64. Size: " .. #binaryData .. " bytes") - return base64Data -end - --------------------------------------------------------------------------------- --- Default system prompt (the more detailed version from BatchLrLlama.lua) --------------------------------------------------------------------------------- - -local defaultSystemPrompt = [[# Image Metadata Generation Prompt - -You are an expert content curator specializing in creating compelling, accurate metadata for visual content. Your task is to analyze the provided image/video and generate a JSON object with three components: title, caption, and keywords. - -## Output Format -Return your response as a valid JSON object with this exact structure: -```json -{ - "title": "string", - "caption": "string", - "keywords": ["string", "string", "string"] -} -``` - -## Guidelines - -### Title Requirements -- **Length**: 5-12 words maximum -- **Style**: Write as a descriptive headline, not a sentence -- **Content**: Capture the main subject, action, and context -- **Focus**: Answer "what is happening" in the most compelling way -- **Avoid**: Generic terms, keyword stuffing, colons, redundant phrases -- **Include**: Specific details like location, time of day, or unique elements when relevant - -**Good examples:** -- "Mountain climber reaching summit during golden hour" -- "Children playing soccer in urban park" -- "Vintage red bicycle against brick wall" - -### Caption Requirements -- **Length**: 15-40 words -- **Style**: Complete sentences that expand on the title -- **Content**: Provide context, mood, or story behind the image -- **Focus**: Add emotional resonance or background information -- **Avoid**: Repeating the exact title wording -- **Include**: Atmosphere, setting details, or cultural context when relevant - -**Good example:** -*Title: "Street musician performing violin solo in subway station"* -*Caption: "A talented violinist captivates commuters with classical music during evening rush hour, creating a moment of beauty in the bustling underground transit hub."* - -### Keywords Requirements -- **Quantity**: 10-30 keywords (aim for 15-20 for optimal results) -- **Hierarchy**: Order from most specific to more general -- **Categories**: Include subjects, actions, emotions, locations, styles, colors, concepts -- **Format**: Single words or short phrases (2-3 words max) -- **Avoid**: Repeating title/caption words exactly, overly generic terms, technical camera specs - -**Keyword categories to consider:** -- Primary subjects (people, objects, animals) -- Actions and verbs -- Emotions and moods -- Locations and settings -- Colors and lighting -- Art styles or techniques -- Concepts and themes -- Seasonal or temporal elements - -## Quality Checklist -Before finalizing, ensure: -- [ ] Title is unique and descriptive without being generic -- [ ] Caption adds meaningful context beyond the title -- [ ] Keywords cover multiple relevant categories -- [ ] No unnecessary repetition across all three elements -- [ ] JSON format is valid and properly structured -- [ ] Content accurately reflects what's actually in the image - -## Example Output -```json -{ - "title": "Barista creating latte art in cozy downtown cafe", - "caption": "Skilled coffee artist carefully pours steamed milk to create an intricate leaf pattern, showcasing the craftsmanship behind specialty coffee culture in a warm, inviting neighborhood coffee shop.", - "keywords": ["barista", "latte art", "coffee shop", "cafe culture", "milk foam", "artisan", "beverage preparation", "downtown", "craftsmanship", "morning routine", "specialty coffee", "hospitality", "small business", "urban lifestyle", "food service"] -} -``` -]] - --------------------------------------------------------------------------------- --- API communication ---- Export a thumbnail, encode it as Base64, and POST to Ollama's `/api/generate`. ---- Retries thumbnail export up to 3 times. Cleans up the temp file after the ---- HTTP request regardless of success or failure. Validates the JSON response ---- has title (string), caption (string), and keywords (array of non-empty strings). ----@param photo LrPhoto Photo object from the catalog ----@param prompt string User-facing prompt text ----@param currentData table|nil Existing title/caption to prepend when useCurrentData is true ----@param useCurrentData boolean Whether to include current metadata in the prompt ----@param useSystemPrompt boolean Whether to send a system prompt alongside the user prompt ----@param selectedModel string|nil Model name; falls back to the default if nil ----@param systemPrompt string|nil Override for the built-in system prompt ----@return table|nil response Parsed JSON on success ({title, caption, keywords}) ----@return string|nil err Error message, or nil on success -local function sendDataToApi(photo, prompt, currentData, useCurrentData, useSystemPrompt, selectedModel, systemPrompt) - logger:info("Sending data to API") - - -- Try to export thumbnail with retry - local thumbnailPath = nil - for attempt = 1, 3 do - thumbnailPath = exportThumbnail(photo) - if thumbnailPath then - break - end - logger:warn("Thumbnail export attempt " .. attempt .. " failed, retrying...") - LrTasks.sleep(0.5) -- Wait 500ms before retry - end - - if not thumbnailPath then - return nil, "Failed to export thumbnail after 3 attempts" - end - - local encodedImage = base64EncodeImage(thumbnailPath) - if not encodedImage then - return nil, "Failed to encode image" - end - - local url = getOllamaBaseUrl() .. "/api/generate" - - -- Choose system prompt: caller-provided > default - local activeSystemPrompt = systemPrompt or defaultSystemPrompt - - local postData = { - model = selectedModel or model, - prompt = (useCurrentData and "Title: "..(currentData.title or ""):gsub('"', '\\"') .. " Caption: "..(currentData.caption or ""):gsub('"', '\\"') .. " " .. prompt) or prompt, - format = "json", - system = useSystemPrompt and activeSystemPrompt or nil, - images = {encodedImage}, - stream = false - } - - local jsonPayload = JSON:encode(postData) - - local response, headers = LrHttp.post(url, jsonPayload, {{ - field = "Content-Type", - value = "application/json" - }}) - - -- Clean up thumbnail file - LrFileUtils.delete(thumbnailPath) - - if response then - -- Decode outer Ollama API response - local decodeOk1, response_data = pcall(function() - return JSON:decode(response) - end) - - if not decodeOk1 - or type(response_data) ~= "table" - or type(response_data.response) ~= "string" then - logger:warn("Invalid API response structure: " .. tostring(response_data)) - return nil, "Invalid API response structure" - end - - local rawResponse = response_data.response - - -- Many Ollama models emit markdown-wrapped JSON ("```json\n{...}\n```"). - -- Strip the fences so JSON:decode receives bare JSON. Two patterns handle - -- both ```json and bare ``` variants. - rawResponse = string.gsub(rawResponse, "^%s*```%w+\n?", "") - rawResponse = string.gsub(rawResponse, "\n?```%s*$", "") - rawResponse = string.gsub(rawResponse, "^%s*```%s*\n?", "") - rawResponse = string.gsub(rawResponse, "\n?```%s*$", "") - - -- Decode the model-generated JSON - local decodeOk2, response_json = pcall(function() - return JSON:decode(rawResponse) - end) - - if not decodeOk2 or type(response_json) ~= "table" then - logger:warn("Invalid JSON in API response: " .. tostring(response_json)) - return nil, "Invalid JSON in API response" - end - - -- Validate required fields exist with correct types - if type(response_json.title) ~= "string" or - type(response_json.caption) ~= "string" or - type(response_json.keywords) ~= "table" then - logger:warn("API response missing required fields (title, caption, keywords)") - return nil, "API response missing required fields (title, caption, keywords)" - end - - -- Validate each keyword entry is a non-empty string - for _, kw in ipairs(response_json.keywords) do - if type(kw) ~= "string" or kw:match("^%s*$") then - logger:warn("API response contains an invalid keyword entry") - return nil, "API response contains an invalid keyword" - end - end - - return response_json, nil - else - return nil, "Failed to send data to the API" - end -end - --------------------------------------------------------------------------------- --- Keyword management --------------------------------------------------------------------------------- - ---- Add keywords under the `llm` parent keyword. ---- Creates (or retrieves) a top-level `llm` keyword, then adds each entry as a child ---- and associates it with the photo. Idempotent — safe to call on photos that ---- already have LLM keywords. ----@param catalog LrCatalog Active catalog (from LrApplication.activeCatalog()) ----@param photo LrPhoto Target photo ----@param keywords table Array of keyword strings -local function addKeywordsWithParent(catalog, photo, keywords) - if not keywords or type(keywords) ~= "table" then - return - end - - -- First create or get the parent 'llm' keyword - local llmKeyword = catalog:createKeyword("llm", nil, true, nil, true) - if not llmKeyword then - error("Failed to create or get 'llm' parent keyword") - end - - for _, keyword in ipairs(keywords) do - if keyword and keyword ~= "" then - -- Create child keyword under 'llm' parent - local childKeyword = catalog:createKeyword(keyword, nil, true, llmKeyword, true) - if childKeyword then - photo:addKeyword(childKeyword) - else - logger:warn("Failed to create keyword: " .. tostring(keyword)) - end - end - end -end - ---- Read existing LLM-generated keywords from a photo. ---- Filters all keywords to only those whose parent is `llm`. Wrapped in ---- `pcall` so that malformed keyword data doesn't crash the plugin. ----@param photo LrPhoto Target photo ----@return table Array of keyword name strings (may be empty) -local function getLlmKeywordsFromPhoto(photo) - local llmKeywords = {} - - -- Wrap in pcall to catch any errors - local success, result = pcall(function() - local allKeywords = photo:getRawMetadata("keywords") - - if allKeywords then - for _, keyword in ipairs(allKeywords) do - local parent = keyword:getParent() - if parent and parent:getName() == "llm" then - table.insert(llmKeywords, keyword:getName()) - end - end - end - end) - - if not success then - logger:warn("Error getting LLM keywords: " .. tostring(result)) - return {} -- Return empty array on error - end - - return llmKeywords -end - ---- Remove all `llm`-parented keywords from a photo. ---- Only removes the keyword-to-photo association — does not delete the keyword ---- definitions from the catalog (other photos may reference them). ----@param catalog LrCatalog Active catalog ----@param photo LrPhoto Target photo -local function removeLlmKeywords(catalog, photo) - local allKeywords = photo:getRawMetadata("keywords") - if not allKeywords then - return - end - - local keywordsToRemove = {} - for _, keyword in ipairs(allKeywords) do - local parent = keyword:getParent() - if parent and parent:getName() == "llm" then - table.insert(keywordsToRemove, keyword) - end - end - - for _, keyword in ipairs(keywordsToRemove) do - photo:removeKeyword(keyword) - end -end - --------------------------------------------------------------------------------- ---- Public API — imported by LrLlama.lua, BatchLrLlama.lua, ResetMetadata.lua --------------------------------------------------------------------------------- +-- Load focused modules +local OllamaClient = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "OllamaClient.lua"))))() +local ThumbnailService = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "ThumbnailService.lua"))))() +local MetadataService = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "MetadataService.lua"))))() return { - model = model, - defaultServerHost = defaultServerHost, - validateServerHost = validateServerHost, - makeModelItems = makeModelItems, - fetchAvailableModels = fetchAvailableModels, - saveServerAndRefresh = saveServerAndRefresh, - exportThumbnail = exportThumbnail, - base64EncodeImage = base64EncodeImage, - sendDataToApi = sendDataToApi, - addKeywordsWithParent = addKeywordsWithParent, - getLlmKeywordsFromPhoto = getLlmKeywordsFromPhoto, - removeLlmKeywords = removeLlmKeywords, + -- OllamaClient constants + model = OllamaClient.defaultModel, + defaultServerHost = OllamaClient.defaultServerHost, + + -- OllamaClient functions (direct delegation) + validateServerHost = OllamaClient.validateServerHost, + makeModelItems = OllamaClient.makeModelItems, + saveServerAndRefresh = OllamaClient.saveServerAndRefresh, + + -- fetchAvailableModels wraps client.fetchModels to provide no-arg compatibility + fetchAvailableModels = function() + return OllamaClient.fetchModels(LrPrefs.prefsForPlugin()) + end, + + -- ThumbnailService functions (direct delegation) + exportThumbnail = ThumbnailService.export, + base64EncodeImage = ThumbnailService.encodeBase64, + + -- OllamaClient.generate is the decomposition of sendDataToApi; same 7-param signature. + sendDataToApi = function(photo, prompt, currentData, useCurrentData, + useSystemPrompt, selectedModel, systemPrompt) + return OllamaClient.generate(photo, prompt, currentData, useCurrentData, + useSystemPrompt, selectedModel, systemPrompt) + end, + + -- MetadataService functions (direct delegation) + addKeywordsWithParent = MetadataService.addKeywordsWithParent, + getLlmKeywordsFromPhoto = MetadataService.getLlmKeywordsFromPhoto, + removeLlmKeywords = MetadataService.removeLlmKeywords, } diff --git a/lightroom-llama.lrplugin/LrLlama.lua b/lightroom-llama.lrplugin/LrLlama.lua index 31846c6..ff46622 100644 --- a/lightroom-llama.lrplugin/LrLlama.lua +++ b/lightroom-llama.lrplugin/LrLlama.lua @@ -12,57 +12,9 @@ local LrColor = import "LrColor" local LrPrefs = import "LrPrefs" local LrPathUtils = import 'LrPathUtils' --- Load shared utilities (includes logger, model constant, JSON loader, shared functions) +-- Load shared utilities (delegation layer to focused modules) local Common = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "Common.lua"))))() - ---- Legacy system prompt used only by the single-photo dialog. ---- Simpler and more permissive than Common.defaultSystemPrompt — allows broader ---- keyword ranges and retains existing metadata verbatim. Passed to sendDataToApi ---- as the systemPrompt override so single-photo mode differs from batch mode. -local originalSystemPrompt = [[You are an AI tasked with creating a JSON object containing a `title`, a `caption`, and a list of `keywords` based on a given piece of content (such as an image or video). ]] .. -[[The content currently has the following metadata which you need to implement and improve upon. It is important to keep the title and caption as close to this as possible. - -Please follow these detailed guidelines for creating excellent metadata: - -1. **Title (Description):** - - Provide a unique, descriptive title for the content. - - The title should answer the Who, What, When, Where, and Why of the content. - - It should be written as a sentence or phrase, similar to a news headline, capturing the key details, mood, and emotions of the scene. - - Do not list keywords in the title. Avoid repetition of words and phrases. - - Include helpful details such as the angle, focus, and perspective if relevant. - - Do not include :. - - If given, use the current title as a starting point. - -2. **Caption:** - - Provide a more detailed description or context for the content. This can be a fuller explanation of the title, including any relevant background or emotional tone that helps convey the essence of the scene. - - If given, use the current caption as a starting point. - -3. **Keywords:** - - Provide a list of 7 to 50 keywords. - - Keywords should be specific and directly related to the content. - - Include broader topics, feelings, concepts, or associations represented by the content. - - Avoid using unrelated terms or repeating words or compound words. - - Do not include links, camera information, or trademarks unless required for editorial content. - -### JSON Format: -```json -{ - "title": "string", - "caption": "string", - "keywords": ["string"] -} -``` - -### Example: -```json -{ - "title": "A serene sunset over a peaceful beach with golden skies", - "caption": "A calm evening beach scene with a golden sunset reflecting on the ocean waves, creating a peaceful and tranquil mood. The horizon is clear with soft, pastel colors blending into the blue sky.", - "keywords": ["sunset", "beach", "calm", "ocean", "serene", "golden skies", "peaceful", "tranquil", "pastel colors", "horizon", "evening"] -} -``` - -Use this structure and guidelines to generate titles, captions, and keywords that are descriptive, unique, and accurate.]] +local PromptBuilder = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "PromptBuilder.lua"))))() -- Note: saveServerAndRefresh moved to Common.lua — used by both dialogs @@ -262,7 +214,7 @@ local function main() props.useCurrentData, props.useSystemPrompt, props.selectedModel, - originalSystemPrompt + PromptBuilder.singlePhotoSystemPrompt ) if apiError then props.status = "Error: " .. apiError diff --git a/lightroom-llama.lrplugin/MetadataService.lua b/lightroom-llama.lrplugin/MetadataService.lua new file mode 100644 index 0000000..750ad2b --- /dev/null +++ b/lightroom-llama.lrplugin/MetadataService.lua @@ -0,0 +1,117 @@ +--- MetadataService.lua — Read and write photo metadata (title, caption, keywords). +--- Manages the `llm` parent keyword hierarchy: creating it, adding children, reading +--- existing keywords, and removing llm-parented keywords from photos. +--- SDK imports: LrLogger only. Catalog and photo objects are passed as parameters. + +local LrLogger = import 'LrLogger' + +local logger = LrLogger('LrLlama') + +local metadata = {} + +--- Add keywords under the `llm` parent keyword (idempotent). +--- Creates (or retrieves) a top-level `llm` keyword, then adds each entry as a child +--- and associates it with the photo. Safe to call on photos that already have LLM +--- keywords — duplicate keyword names are handled by Lightroom's createKeyword API. +---@param catalog LrCatalog Active catalog (from LrApplication.activeCatalog()) +---@param photo LrPhoto Target photo +---@param keywords table Array of keyword strings to add +function metadata.addKeywordsWithParent(catalog, photo, keywords) + if not keywords or type(keywords) ~= "table" then + return + end + + -- First create or get the parent 'llm' keyword + local llmKeyword = catalog:createKeyword("llm", nil, true, nil, true) + if not llmKeyword then + error("Failed to create or get 'llm' parent keyword") + end + + for _, keyword in ipairs(keywords) do + if keyword and keyword ~= "" then + -- Create child keyword under 'llm' parent + local childKeyword = catalog:createKeyword(keyword, nil, true, llmKeyword, true) + if childKeyword then + photo:addKeyword(childKeyword) + else + logger:warn("Failed to create keyword: " .. tostring(keyword)) + end + end + end +end + +--- Read existing llm-parented keywords from a photo. +--- Filters all keywords to only those whose parent is `llm`. Wrapped in +--- `pcall` so that malformed keyword data doesn't crash the plugin. +---@param photo LrPhoto Target photo +---@return table Keyword name strings (may be empty) +function metadata.getLlmKeywordsFromPhoto(photo) + local llmKeywords = {} + + -- Wrap in pcall to catch any errors + local success, result = pcall(function() + local allKeywords = photo:getRawMetadata("keywords") + + if allKeywords then + for _, keyword in ipairs(allKeywords) do + local parent = keyword:getParent() + if parent and parent:getName() == "llm" then + table.insert(llmKeywords, keyword:getName()) + end + end + end + end) + + if not success then + logger:warn("Error getting LLM keywords: " .. tostring(result)) + return {} -- Return empty array on error + end + + return llmKeywords +end + +--- Remove all llm-parented keywords from a photo. +--- Only removes the keyword-to-photo association — does not delete the keyword +--- definitions from the catalog (other photos may reference them). +---@param catalog LrCatalog Active catalog +---@param photo LrPhoto Target photo +function metadata.removeLlmKeywords(catalog, photo) + local allKeywords = photo:getRawMetadata("keywords") + if not allKeywords then + return + end + + local keywordsToRemove = {} + for _, keyword in ipairs(allKeywords) do + local parent = keyword:getParent() + if parent and parent:getName() == "llm" then + table.insert(keywordsToRemove, keyword) + end + end + + for _, keyword in ipairs(keywordsToRemove) do + photo:removeKeyword(keyword) + end +end + +--- Parse a comma-separated keyword string into a trimmed list. +--- Filters out empty entries. Used when reading keywords from the dialog's +--- edit field, where the user enters "sunset, beach, calm". +---@param csv string Comma-separated keywords (e.g., "sunset, beach, calm") +---@return table Trimmed keyword strings, empty entries filtered out +function metadata.parseKeywordCsv(csv) + if not csv or csv == "" then + return {} + end + + local result = {} + for keyword in string.gmatch(csv, "([^,]+)") do + local trimmed = keyword:match("^%s*(.-)%s*$") + if trimmed ~= "" then + table.insert(result, trimmed) + end + end + return result +end + +return metadata diff --git a/lightroom-llama.lrplugin/OllamaClient.lua b/lightroom-llama.lrplugin/OllamaClient.lua new file mode 100644 index 0000000..1487f7e --- /dev/null +++ b/lightroom-llama.lrplugin/OllamaClient.lua @@ -0,0 +1,260 @@ +--- OllamaClient.lua — Communicate with a local Ollama server. +--- Handles base URL construction, model discovery, HTTP requests to the generate API, +--- and orchestrates the full pipeline: export thumbnail -> encode -> build prompt -> POST +--- -> validate response. +--- SDK imports: LrHttp, LrPrefs, LrLogger, LrPathUtils, LrTasks + +local LrHttp = import 'LrHttp' +local LrPathUtils = import 'LrPathUtils' +local LrLogger = import 'LrLogger' +local LrTasks = import "LrTasks" +local LrPrefs = import "LrPrefs" + +local logger = LrLogger('LrLlama') +logger:enable("logfile") -- Logs to ~/Documents/LrClassicLogs | tail -f LrLlama.log + +-- Load shared utilities +local JSON = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "JSON.lua"))))() +local ThumbnailService = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "ThumbnailService.lua"))))() +local PromptBuilder = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "PromptBuilder.lua"))))() +local ResponseValidator = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "ResponseValidator.lua"))))() + +local client = {} + +-------------------------------------------------------------------------------- +-- Constants +-------------------------------------------------------------------------------- + +--- Default Ollama model name; change here to switch models. +client.defaultModel = "gemma4:latest" + +--- Fallback server address when prefs are missing, empty, or invalid. +client.defaultServerHost = "localhost:11434" + +-------------------------------------------------------------------------------- +-- Server configuration +-------------------------------------------------------------------------------- + +--- Validate and normalize an Ollama server address. +--- Strips optional scheme (`http://`) and trailing slashes, then verifies the +--- string is `hostname:port`. Accepts localhost, IP addresses, and domain names. +---@param input string|nil User-supplied server address (may be empty) +---@return bool ok True when the address is valid +---@return string result The normalized host:port on success, error message on failure +function client.validateServerHost(input) + if not input or input == "" then + return true, client.defaultServerHost -- treat empty as "use default" + end + -- Strip scheme and trailing slashes for normalization + local host = string.gsub(input, "^https?://", "") + host = string.gsub(host, "/+$", "") + + -- Must match something:digit (hostname or IP followed by colon and port) + local hostnamePart, portPart = string.match(host, "^([^:]+):(%d+)$") + if not hostnamePart or not portPart then + return false, "Server address must be in host:port format (e.g., localhost:11434)" + end + + local port = tonumber(portPart) + if port < 1 or port > 65535 then + return false, "Port number must be between 1 and 65535" + end + + -- hostnamePart should contain at least one valid character (alphanumeric, dot, hyphen) + if not string.match(hostnamePart, "^[%a%d%.%-]+$") then + return false, "Hostname contains invalid characters" + end + + return true, hostnamePart .. ":" .. portPart +end + +--- Build the base URL from saved preferences. +--- Reads prefs.ollamaServerHost, validates, falls back to default if malformed. +---@param prefs table LrPrefs.prefsForPlugin() instance (injected for testability) +---@return string baseUrl Full URL like "http://localhost:11434" +function client.getBaseUrl(prefs) + local ok, host = client.validateServerHost(prefs.ollamaServerHost) + + if not ok then + logger:warn("Invalid server host in prefs: " .. tostring(prefs.ollamaServerHost)) + host = client.defaultServerHost + end + + return "http://" .. host +end + +-------------------------------------------------------------------------------- +-- Model discovery +-------------------------------------------------------------------------------- + +--- Convert a plain list of model names into the `{title, value}` format that +--- LrView `popup_menu` requires for its `items` binding. +---@param modelNames table List of model strings (from fetchModels) +---@return table
Table of `{title = m, value = m}` entries for popup_menu binding +function client.makeModelItems(modelNames) + local items = {} + for _, m in ipairs(modelNames) do + table.insert(items, { title = m, value = m }) + end + return items +end + +--- Query Ollama's `/api/tags` endpoint for installed models. +--- Falls back to `{defaultModel}` on network errors, malformed JSON, or an empty +--- model list — so callers never receive nil. +---@param prefs table LrPrefs instance +---@return table List of model name strings; always non-empty. +function client.fetchModels(prefs) + logger:info("Fetching available models from Ollama") + + local url = client.getBaseUrl(prefs) .. "/api/tags" + local response = LrHttp.get(url) + + if response then + local decodeSuccess, data = pcall(function() + return JSON:decode(response) + end) + + if decodeSuccess and data and type(data.models) == "table" then + local modelList = {} + + for _, availableModel in ipairs(data.models) do + local modelName = availableModel.name or availableModel.model + + if modelName and modelName ~= "" then + table.insert(modelList, modelName) + end + end + + if #modelList > 0 then + logger:info("Found " .. #modelList .. " models") + return modelList + end + + logger:warn("Ollama returned an empty model list") + elseif not decodeSuccess then + logger:warn( + "Failed to decode Ollama model response: " .. tostring(data) + ) + else + logger:warn("Ollama response did not contain a valid model list") + end + else + logger:warn("No response received from Ollama") + end + + logger:warn("Using default model: " .. client.defaultModel) + return { client.defaultModel } +end + +--- Persist the server host and rebuild the model dropdown. +--- MUST be called from within `LrTasks.startAsyncTask()` — LrBinding only +--- propagates property reassignments (modelItems, selectedModel) when done +--- off the UI thread. +---@param props table LrBinding property table containing serverHost, modelItems, selectedModel +---@param prefs table LrPrefs.prefsForPlugin() instance +---@return bool ok True on success +---@return string message Human-readable status or error description +function client.saveServerAndRefresh(props, prefs) + local ok, validatedHost = client.validateServerHost(props.serverHost) + if not ok then + return false, validatedHost -- caller displays error + end + + prefs.ollamaServerHost = validatedHost + + local availableModels = client.fetchModels(prefs) + if not availableModels or #availableModels == 0 then + return false, "No models found on server" + end + + -- Rebuild model list. This function is intended to be called from inside + -- LrTasks.startAsyncTask() so that LrBinding picks up the property + -- reassignment and propagates it to the dialog's popup_menu. + local oldSelection = props.selectedModel or "" + props.modelItems = client.makeModelItems(availableModels) + + -- Keep old selection if it still exists in the new list + local found = false + for _, m in ipairs(availableModels) do + if m == oldSelection then found = true; break; end + end + props.selectedModel = found and oldSelection or availableModels[1] + + return true, string.format("Loaded %d model(s)", #availableModels) +end + +-------------------------------------------------------------------------------- +-- Generate pipeline +-------------------------------------------------------------------------------- + +--- High-level generate: export thumbnail, encode, build prompt, POST, validate response. +--- Orchestrates ThumbnailService + PromptBuilder + ResponseValidator internally. +--- This is the decomposition of the former Common.sendDataToApi mega-function. +--- Retries thumbnail export up to 3 times. Cleans up the temp file after the +--- HTTP request regardless of success or failure. Validates the JSON response +--- has title (string), caption (string), and keywords (array of non-empty strings). +---@param photo LrPhoto Photo object from the catalog +---@param userInstruction string User-facing prompt text +---@param currentData table|nil Existing title/caption to prepend when useCurrentData is true +---@param useCurrentData boolean Whether to include current metadata in the prompt +---@param useSystemPrompt boolean Whether to send a system prompt alongside the user prompt +---@param selectedModel string|nil Model name; falls back to the default if nil +---@param systemPromptOverride string|nil Override for the built-in system prompt +---@return table|nil response Parsed JSON on success ({title, caption, keywords}) +---@return string|nil err Error message, or nil on success +function client.generate(photo, userInstruction, currentData, useCurrentData, + useSystemPrompt, selectedModel, systemPromptOverride) + logger:info("Sending data to API") + + -- 1. Export thumbnail with retry (up to 3 attempts) + local thumbnailPath = nil + for attempt = 1, 3 do + thumbnailPath = ThumbnailService.export(photo) + if thumbnailPath then + break + end + logger:warn("Thumbnail export attempt " .. attempt .. " failed, retrying...") + LrTasks.sleep(0.5) -- Wait 500ms before retry + end + + if not thumbnailPath then + return nil, "Failed to export thumbnail after 3 attempts" + end + + -- 2. Encode image as Base64 + local encodedImage = ThumbnailService.encodeBase64(thumbnailPath) + if not encodedImage then + ThumbnailService.cleanup(thumbnailPath) + return nil, "Failed to encode image" + end + + -- 3. Build user prompt and request body + local builtPrompt = PromptBuilder.buildUserPrompt(userInstruction, currentData, useCurrentData) + local requestBody = PromptBuilder.assembleRequestBody( + builtPrompt, selectedModel or client.defaultModel, + useSystemPrompt, systemPromptOverride + ) + requestBody.images = {encodedImage} + + -- 4. Send HTTP POST + local url = client.getBaseUrl(LrPrefs.prefsForPlugin()) .. "/api/generate" + local jsonPayload = JSON:encode(requestBody) + + local response = LrHttp.post(url, jsonPayload, {{ + field = "Content-Type", + value = "application/json" + }}) + + -- 5. Clean up thumbnail file (always, regardless of success) + ThumbnailService.cleanup(thumbnailPath) + + -- 6. Parse and validate response + if not response then + return nil, "Failed to send data to the API" + end + + return ResponseValidator.validateAndParse(response) +end + +return client diff --git a/lightroom-llama.lrplugin/PromptBuilder.lua b/lightroom-llama.lrplugin/PromptBuilder.lua new file mode 100644 index 0000000..ce66c4a --- /dev/null +++ b/lightroom-llama.lrplugin/PromptBuilder.lua @@ -0,0 +1,180 @@ +--- PromptBuilder.lua — Build prompts and request payloads for Ollama API calls. +--- Pure Lua module with no Lightroom SDK dependencies (no LrView, LrDialogs, etc.). +--- Owns all system prompt definitions and user-prompt assembly logic. +--- Loaded by OllamaClient.lua for the generate pipeline and by tests directly. + +local prompt = {} + +-------------------------------------------------------------------------------- +-- System prompts +-------------------------------------------------------------------------------- + +--- Default system prompt for batch/general mode (the detailed version). +--- Stricter guidelines: 5-12 word titles, 10-30 keywords, quality checklist. +prompt.defaultSystemPrompt = [[# Image Metadata Generation Prompt + +You are an expert content curator specializing in creating compelling, accurate metadata for visual content. Your task is to analyze the provided image/video and generate a JSON object with three components: title, caption, and keywords. + +## Output Format +Return your response as a valid JSON object with this exact structure: +```json +{ + "title": "string", + "caption": "string", + "keywords": ["string", "string", "string"] +} +``` + +## Guidelines + +### Title Requirements +- **Length**: 5-12 words maximum +- **Style**: Write as a descriptive headline, not a sentence +- **Content**: Capture the main subject, action, and context +- **Focus**: Answer "what is happening" in the most compelling way +- **Avoid**: Generic terms, keyword stuffing, colons, redundant phrases +- **Include**: Specific details like location, time of day, or unique elements when relevant + +**Good examples:** +- "Mountain climber reaching summit during golden hour" +- "Children playing soccer in urban park" +- "Vintage red bicycle against brick wall" + +### Caption Requirements +- **Length**: 15-40 words +- **Style**: Complete sentences that expand on the title +- **Content**: Provide context, mood, or story behind the image +- **Focus**: Add emotional resonance or background information +- **Avoid**: Repeating the exact title wording +- **Include**: Atmosphere, setting details, or cultural context when relevant + +**Good example:** +*Title: "Street musician performing violin solo in subway station"* +*Caption: "A talented violinist captivates commuters with classical music during evening rush hour, creating a moment of beauty in the bustling underground transit hub."* + +### Keywords Requirements +- **Quantity**: 10-30 keywords (aim for 15-20 for optimal results) +- **Hierarchy**: Order from most specific to more general +- **Categories**: Include subjects, actions, emotions, locations, styles, colors, concepts +- **Format**: Single words or short phrases (2-3 words max) +- **Avoid**: Repeating title/caption words exactly, overly generic terms, technical camera specs + +**Keyword categories to consider:** +- Primary subjects (people, objects, animals) +- Actions and verbs +- Emotions and moods +- Locations and settings +- Colors and lighting +- Art styles or techniques +- Concepts and themes +- Seasonal or temporal elements + +## Quality Checklist +Before finalizing, ensure: +- [ ] Title is unique and descriptive without being generic +- [ ] Caption adds meaningful context beyond the title +- [ ] Keywords cover multiple relevant categories +- [ ] No unnecessary repetition across all three elements +- [ ] JSON format is valid and properly structured +- [ ] Content accurately reflects what's actually in the image + +## Example Output +```json +{ + "title": "Barista creating latte art in cozy downtown cafe", + "caption": "Skilled coffee artist carefully pours steamed milk to create an intricate leaf pattern, showcasing the craftsmanship behind specialty coffee culture in a warm, inviting neighborhood coffee shop.", + "keywords": ["barista", "latte art", "coffee shop", "cafe culture", "milk foam", "artisan", "beverage preparation", "downtown", "craftsmanship", "morning routine", "specialty coffee", "hospitality", "small business", "urban lifestyle", "food service"] +} +``` +]] + +--- Legacy system prompt for single-photo dialog (more permissive). +--- Allows broader keyword ranges (7-50) and retains existing metadata verbatim. +prompt.singlePhotoSystemPrompt = [[You are an AI tasked with creating a JSON object containing a `title`, a `caption`, and a list of `keywords` based on a given piece of content (such as an image or video). ]] .. +[[The content currently has the following metadata which you need to implement and improve upon. It is important to keep the title and caption as close to this as possible. + +Please follow these detailed guidelines for creating excellent metadata: + +1. **Title (Description):** + - Provide a unique, descriptive title for the content. + - The title should answer the Who, What, When, Where, and Why of the content. + - It should be written as a sentence or phrase, similar to a news headline, capturing the key details, mood, and emotions of the scene. + - Do not list keywords in the title. Avoid repetition of words and phrases. + - Include helpful details such as the angle, focus, and perspective if relevant. + - Do not include :. + - If given, use the current title as a starting point. + +2. **Caption:** + - Provide a more detailed description or context for the content. This can be a fuller explanation of the title, including any relevant background or emotional tone that helps convey the essence of the scene. + - If given, use the current caption as a starting point. + +3. **Keywords:** + - Provide a list of 7 to 50 keywords. + - Keywords should be specific and directly related to the content. + - Include broader topics, feelings, concepts, or associations represented by the content. + - Avoid using unrelated terms or repeating words or compound words. + - Do not include links, camera information, or trademarks unless required for editorial content. + +### JSON Format: +```json +{ + "title": "string", + "caption": "string", + "keywords": ["string"] +} +``` + +### Example: +```json +{ + "title": "A serene sunset over a peaceful beach with golden skies", + "caption": "A calm evening beach scene with a golden sunset reflecting on the ocean waves, creating a peaceful and tranquil mood. The horizon is clear with soft, pastel colors blending into the blue sky.", + "keywords": ["sunset", "beach", "calm", "ocean", "serene", "golden skies", "peaceful", "tranquil", "pastel colors", "horizon", "evening"] +} +``` + +Use this structure and guidelines to generate titles, captions, and keywords that are descriptive, unique, and accurate.]] + +-------------------------------------------------------------------------------- +-- Prompt assembly +-------------------------------------------------------------------------------- + +--- Build the user prompt string, optionally prepending current metadata. +--- When `useCurrentData` is true, formats as `"Title: Caption: "` +--- to give the model context about existing metadata. Escapes double quotes in +--- title and caption to avoid breaking JSON when this string is embedded in a payload. +---@param userInstruction string The user's custom prompt text (e.g., "Caption this photo") +---@param currentData {title: string, caption: string}|nil Existing metadata +---@param useCurrentData boolean Whether to prepend existing title/caption +---@return string assembled Prompt text ready for the API +function prompt.buildUserPrompt(userInstruction, currentData, useCurrentData) + if not useCurrentData or not currentData then + return userInstruction + end + + local title = (currentData.title or ""):gsub('"', '\\"') + local caption = (currentData.caption or ""):gsub('"', '\\"') + return "Title: " .. title .. " Caption: " .. caption .. " " .. userInstruction +end + +--- Assemble the full Ollama /api/generate request body fields. +--- Returns a table suitable for JSON:encode before HTTP POST. +--- Does NOT include the `images` field — callers add that after receiving this table. +---@param userPrompt string The assembled user prompt (from buildUserPrompt) +---@param model string Model name to use +---@param useSystemPrompt boolean Whether to include system prompt in request +---@param systemPromptOverride string|nil Custom system prompt, or nil to use default +---@return table requestBody Ready-to-encode POST fields (model, prompt, format, system?, stream) +function prompt.assembleRequestBody(userPrompt, model, useSystemPrompt, systemPromptOverride) + local activeSystemPrompt = systemPromptOverride or prompt.defaultSystemPrompt + + return { + model = model, + prompt = userPrompt, + format = "json", + system = useSystemPrompt and activeSystemPrompt or nil, + stream = false, + } +end + +return prompt diff --git a/lightroom-llama.lrplugin/ResponseValidator.lua b/lightroom-llama.lrplugin/ResponseValidator.lua new file mode 100644 index 0000000..229073e --- /dev/null +++ b/lightroom-llama.lrplugin/ResponseValidator.lua @@ -0,0 +1,105 @@ +--- ResponseValidator.lua — Parse and validate Ollama API responses. +--- Pure Lua module with no Lightroom SDK dependencies. Strips markdown fences, +--- decodes the outer API envelope, and validates the inner JSON schema has +--- title (string), caption (string), and keywords (array of non-empty strings). +--- Loaded by OllamaClient.lua for the generate pipeline and by tests directly. + +local LrPathUtils = import 'LrPathUtils' + +local JSON = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "JSON.lua"))))() + +local validate = {} + +--- Strip markdown code fences from a string. +--- Handles ```json, ```, and bare backtick variants. Two passes ensure both +--- opening and closing fences are removed, including edge cases where the model +--- emits extra whitespace around the blocks. +---@param raw string Raw model response text +---@return string cleaned Text with fences removed +function validate.stripMarkdownFences(raw) + local result = string.gsub(raw, "^%s*```%w+\n?", "") + result = string.gsub(result, "\n?```%s*$", "") + result = string.gsub(result, "^%s*```%s*\n?", "") + result = string.gsub(result, "\n?```%s*$", "") + return result +end + +--- Decode and validate an Ollama API envelope response. +--- Verifies the outer `{response: "..."}` structure exists. The envelope is the +--- direct JSON output of the `/api/generate` endpoint, which wraps the model's +--- text in a `response` field. +---@param apiResponseString string The raw HTTP response body from /api/generate +---@return table|nil parsed Envelope data (has .response field), or nil +---@return string|nil err Error message or nil +function validate.parseApiEnvelope(apiResponseString) + local decodeOk, data = pcall(function() + return JSON:decode(apiResponseString) + end) + + if not decodeOk + or type(data) ~= "table" + or type(data.response) ~= "string" then + return nil, "Invalid API response structure" + end + + return data, nil +end + +--- Validate that a decoded JSON object has the required metadata schema. +--- Checks for title (string), caption (string), keywords (array of non-empty strings). +---@param data table Decoded JSON object from the model +---@return bool ok True if schema is valid +---@return string|nil err Describes which field failed, or nil +function validate.checkMetadataSchema(data) + if type(data.title) ~= "string" + or type(data.caption) ~= "string" + or type(data.keywords) ~= "table" then + return false, "API response missing required fields (title, caption, keywords)" + end + + for _, kw in ipairs(data.keywords) do + if type(kw) ~= "string" or kw:match("^%s*$") then + return false, "API response contains an invalid keyword" + end + end + + return true, nil +end + +--- Full pipeline: parse envelope, strip fences, decode inner JSON, validate schema. +--- Accepts the raw HTTP response string and returns parsed metadata or an error. +---@param rawHttpResponse string Raw POST /api/generate response body +---@return table|nil metadata {title, caption, keywords} on success +---@return string|nil err Error message, or nil +function validate.validateAndParse(rawHttpResponse) + -- Parse outer Ollama API envelope + local envelope, err = validate.parseApiEnvelope(rawHttpResponse) + if not envelope then + return nil, err + end + + local rawResponse = envelope.response + + -- Many Ollama models emit markdown-wrapped JSON ("```json\n{...}\n```"). + -- Strip the fences so JSON:decode receives bare JSON. + rawResponse = validate.stripMarkdownFences(rawResponse) + + -- Decode the model-generated JSON + local decodeOk, responseJson = pcall(function() + return JSON:decode(rawResponse) + end) + + if not decodeOk or type(responseJson) ~= "table" then + return nil, "Invalid JSON in API response" + end + + -- Validate required fields exist with correct types + local schemaOk, schemaErr = validate.checkMetadataSchema(responseJson) + if not schemaOk then + return nil, schemaErr + end + + return responseJson, nil +end + +return validate diff --git a/lightroom-llama.lrplugin/ThumbnailService.lua b/lightroom-llama.lrplugin/ThumbnailService.lua new file mode 100644 index 0000000..71b6bad --- /dev/null +++ b/lightroom-llama.lrplugin/ThumbnailService.lua @@ -0,0 +1,122 @@ +--- ThumbnailService.lua — Export, encode, and clean up photo thumbnails. +--- Handles the full thumbnail lifecycle: request JPEG from Lightroom, write to temp, +--- Base64-encode for API payload, and delete after use. +--- SDK imports: LrFileUtils, LrPathUtils, LrStringUtils, LrLogger + +local LrFileUtils = import 'LrFileUtils' +local LrPathUtils = import 'LrPathUtils' +local LrStringUtils = import 'LrStringUtils' +local LrLogger = import 'LrLogger' + +local logger = LrLogger('LrLlama') + +local thumbnail = {} + +--- Export a 512×512 JPEG thumbnail to a unique temp file. +--- **Side effect:** writes a file to the system temp directory. +--- Caller is responsible for deleting the returned file after use via `cleanup()`. +---@param photo LrPhoto Photo object from the catalog +---@return string|nil Absolute path on success, nil on failure +function thumbnail.export(photo) + local tempPath = LrFileUtils.chooseUniqueFileName(LrPathUtils.getStandardFilePath('temp') .. "/thumbnail.jpg") + logger:info("Attempting to export thumbnail to: " .. tempPath) + + -- Validate that the temp directory exists and is accessible before proceeding + -- This prevents silent failures when the temp directory is missing or restricted + local tempDir = LrPathUtils.getStandardFilePath('temp') + if not LrFileUtils.exists(tempDir) then + logger:error("Temp directory does not exist: " .. tempDir) + return nil + end + + -- Track whether the thumbnail callback successfully wrote data + -- This flag is set inside the callback to communicate success back to the caller + local thumbnailSaved = false + + -- Request a 512x512 JPEG thumbnail asynchronously + -- photo:requestJpegThumbnail() returns (success, result) and executes the callback + -- with the JPEG binary data if available + local success, result = photo:requestJpegThumbnail(512, 512, function(jpegData) + if jpegData then + -- Open temp file in binary write mode ("wb") to preserve JPEG data integrity + local tempFile = io.open(tempPath, "wb") + if tempFile then + tempFile:write(jpegData) + tempFile:close() + thumbnailSaved = true + logger:info("Thumbnail saved to " .. tempPath) + return true + else + logger:error("Could not open temp file for writing: " .. tempPath) + return false + end + else + logger:error("No JPEG data received from photo") + return false + end + end) + + -- Verify both the API call succeeded AND the callback wrote the file + if success and thumbnailSaved then + -- Final verification: ensure the file actually exists on disk + -- This catches edge cases where the write appeared successful but the file is missing + if LrFileUtils.exists(tempPath) then + logger:info("Thumbnail export successful: " .. tempPath) + return tempPath + else + logger:error("Thumbnail file was not created: " .. tempPath) + return nil + end + else + logger:warn("Failed to export thumbnail. Success: " .. tostring(success) .. ", Result: " .. tostring(result)) + return nil + end +end + +--- Read a JPEG file and return its Base64-encoded representation. +---@param imagePath string Absolute path to the JPEG file +---@return string|nil Base64 string on success, nil if file unreadable or empty +function thumbnail.encodeBase64(imagePath) + logger:info("Attempting to encode image: " .. imagePath) + + -- Check if file exists + if not LrFileUtils.exists(imagePath) then + logger:error("Image file does not exist: " .. imagePath) + return nil + end + + local file = io.open(imagePath, "rb") + if not file then + logger:error("Could not open file for reading: " .. imagePath) + return nil + end + + local binaryData = file:read("*all") + file:close() + + if not binaryData or #binaryData == 0 then + logger:error("No data read from file: " .. imagePath) + return nil + end + + local base64Data = LrStringUtils.encodeBase64(binaryData) + if not base64Data then + logger:error("Failed to encode image to base64: " .. imagePath) + return nil + end + + logger:info("Successfully encoded image to base64. Size: " .. #binaryData .. " bytes") + return base64Data +end + +--- Clean up a temp thumbnail file (idempotent — no error if already missing). +--- Call after the API request completes, regardless of success or failure. +---@param imagePath string Path to delete +function thumbnail.cleanup(imagePath) + if imagePath and LrFileUtils.exists(imagePath) then + LrFileUtils.delete(imagePath) + logger:info("Cleaned up temp thumbnail: " .. imagePath) + end +end + +return thumbnail diff --git a/tests/busted.json b/tests/busted.json new file mode 100644 index 0000000..8150aee --- /dev/null +++ b/tests/busted.json @@ -0,0 +1,4 @@ +{ + "helper": [ "helpers/mock_sdk.lua" ], + "lua_sources": [ "." ] +} diff --git a/tests/helpers/mock_sdk.lua b/tests/helpers/mock_sdk.lua new file mode 100644 index 0000000..6872dfe --- /dev/null +++ b/tests/helpers/mock_sdk.lua @@ -0,0 +1,56 @@ +--- mock_sdk.lua — Minimal Lightroom SDK stubs for unit testing modules +--- that import LrPathUtils, LrLogger, LrPrefs, etc. at load time. +--- +--- Runs automatically before each spec via busted.json "helpers" config. + +-- Resolve the .lrplugin root relative to this helper file's location so tests +-- work on any developer's machine regardless of where the repo is cloned. +-- The structure is: /tests/helpers/ (this file) +-- /lightroom-llama.lrplugin/ (target) +local helperDir = string.match(debug.getinfo(1, "S").source, "^@(.*/)") or "" +PLUGIN_PATH = helperDir .. "../..//lightroom-llama.lrplugin/" + +-- _PLUGIN is a global Lightroom injects. Point it at the real plugin dir so +-- loadfile() can find JSON.lua and sibling modules. +_G._PLUGIN = { path = PLUGIN_PATH } + +-- import() is the Lightroom SDK class loader. Return a stub for each class +--- that a module might request. +import = function(className) + if className == "LrPathUtils" then + return { + child = function(parent, name) + return parent .. "/" .. name + end, + } + end + + if className == "LrLogger" then + -- LrLogger is called as a constructor: LrLogger('name') → logger instance + return setmetatable({}, { + __call = function(_, _) + return { + info = function() end, + warn = function() end, + error = function() end, + enable = function() end, + } + end, + }) + end + + if className == "LrPrefs" then + return { prefsForPlugin = function() return {} end } + end + + if className == "LrHttp" then + return {} + end + + if className == "LrTasks" then + return { sleep = function(_) end } + end + + -- Unknown classes get a blank table so loadfile() keeps working. + return {} +end diff --git a/tests/run_tests.sh b/tests/run_tests.sh new file mode 100755 index 0000000..4e47a77 --- /dev/null +++ b/tests/run_tests.sh @@ -0,0 +1,13 @@ +#!/usr/bin/env bash +# Run the Busted test suite for Lightroom Llama plugin modules. +# Usage: ./tests/run_tests.sh [spec file or directory] +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +ROOT_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" + +cd "$SCRIPT_DIR" + +busted \ + -e "dofile('helpers/mock_sdk.lua')" \ + "${1:-spec/}" diff --git a/tests/spec/common_delegation_spec.lua b/tests/spec/common_delegation_spec.lua new file mode 100644 index 0000000..c7ba38f --- /dev/null +++ b/tests/spec/common_delegation_spec.lua @@ -0,0 +1,86 @@ +--- common_delegation_spec.lua — Verify Common.lua exports all expected keys and +--- that each delegated function behaves correctly. Because loadfile() creates new +--- function objects on each call, we test behavior (output equivalence) rather +--- than pointer identity. + +local path = PLUGIN_PATH + +describe("Common.lua delegation", function() + local Common + + before_each(function() + _PLUGIN = { path = path } + Common = assert(loadfile(path .. "Common.lua"))() + end) + + -------------------------------------------------------------------- + -- All expected keys are present and non-nil + -------------------------------------------------------------------- + describe("exports", function() + it("has all expected constants", function() + assert.is_not_nil(Common.model) + assert.is_not_nil(Common.defaultServerHost) + end) + + it("has all expected functions", function() + assert.is_function(Common.validateServerHost) + assert.is_function(Common.makeModelItems) + assert.is_function(Common.saveServerAndRefresh) + assert.is_function(Common.fetchAvailableModels) + assert.is_function(Common.exportThumbnail) + assert.is_function(Common.base64EncodeImage) + assert.is_function(Common.sendDataToApi) + assert.is_function(Common.addKeywordsWithParent) + assert.is_function(Common.getLlmKeywordsFromPhoto) + assert.is_function(Common.removeLlmKeywords) + end) + + it("exports correct default values", function() + assert.are_same("gemma4:latest", Common.model) + assert.are_same("localhost:11434", Common.defaultServerHost) + end) + end) + + -------------------------------------------------------------------- + -- Delegated functions produce correct results + -------------------------------------------------------------------- + describe("validateServerHost delegation", function() + it("accepts localhost:11434", function() + local ok, result = Common.validateServerHost("localhost:11434") + assert.is_true(ok) + assert.are_same("localhost:11434", result) + end) + + it("rejects missing port", function() + local ok = Common.validateServerHost("localhost") + assert.is_false(ok) + end) + + it("normalizes http:// prefix", function() + local ok, result = Common.validateServerHost("http://192.168.1.5:9000/") + assert.is_true(ok) + assert.are_same("192.168.1.5:9000", result) + end) + end) + + describe("makeModelItems delegation", function() + it("converts model names to title/value pairs", function() + local items = Common.makeModelItems({ "gemma4:latest", "llama3" }) + assert.are_same(2, #items) + assert.are_same("gemma4:latest", items[1].title) + assert.are_same("gemma4:latest", items[1].value) + end) + end) + + describe("sendDataToApi wrapper", function() + it("is a function, not a table or nil", function() + assert.is_function(Common.sendDataToApi) + end) + end) + + describe("fetchAvailableModels wrapper", function() + it("is a function, not a table or nil", function() + assert.is_function(Common.fetchAvailableModels) + end) + end) +end) diff --git a/tests/spec/metadata_service_parse_spec.lua b/tests/spec/metadata_service_parse_spec.lua new file mode 100644 index 0000000..83e4019 --- /dev/null +++ b/tests/spec/metadata_service_parse_spec.lua @@ -0,0 +1,51 @@ +--- metadata_service_parse_spec.lua — Unit tests for MetadataService.parseKeywordCsv +--- Tests CSV parsing, trimming, and edge cases. Does not test catalog-bound +--- methods (addKeywordsWithParent, getLlmKeywordsFromPhoto, removeLlmKeywords) +--- since they require real Lightroom SDK objects. + +local path = PLUGIN_PATH + +describe("MetadataService.parseKeywordCsv", function() + local metadata + + before_each(function() + metadata = assert(loadfile(path .. "MetadataService.lua"))() + end) + + it("returns empty table for nil input", function() + assert.are_same({}, metadata.parseKeywordCsv(nil)) + end) + + it("returns empty table for empty string", function() + assert.are_same({}, metadata.parseKeywordCsv("")) + end) + + it("parses a single keyword", function() + assert.are_same({ "sunset" }, metadata.parseKeywordCsv("sunset")) + end) + + it("parses multiple keywords", function() + local got = metadata.parseKeywordCsv("beach, sunset, calm") + assert.are_same({ "beach", "sunset", "calm" }, got) + end) + + it("trims whitespace around each keyword", function() + local got = metadata.parseKeywordCsv(" ocean , hills , sky ") + assert.are_same({ "ocean", "hills", "sky" }, got) + end) + + it("filters out empty entries from consecutive commas", function() + local got = metadata.parseKeywordCsv("a,,b, ,c") + assert.are_same({ "a", "b", "c" }, got) + end) + + it("handles trailing comma", function() + local got = metadata.parseKeywordCsv("one, two,") + assert.are_same({ "one", "two" }, got) + end) + + it("handles leading comma", function() + local got = metadata.parseKeywordCsv(",first, second") + assert.are_same({ "first", "second" }, got) + end) +end) diff --git a/tests/spec/ollama_client_validate_spec.lua b/tests/spec/ollama_client_validate_spec.lua new file mode 100644 index 0000000..c110366 --- /dev/null +++ b/tests/spec/ollama_client_validate_spec.lua @@ -0,0 +1,119 @@ +--- ollama_client_validate_spec.lua — Unit tests for OllamaClient.validateServerHost +--- Tests input normalization, host:port validation, and error cases. + +local path = PLUGIN_PATH + +describe("OllamaClient.validateServerHost", function() + local client + + before_each(function() + client = assert(loadfile(path .. "OllamaClient.lua"))() + end) + + -------------------------------------------------------------------- + -- Defaults + -------------------------------------------------------------------- + describe("defaults", function() + it("accepts nil and returns default host", function() + local ok, result = client.validateServerHost(nil) + assert.is_true(ok) + assert.are_same("localhost:11434", result) + end) + + it("accepts empty string and returns default host", function() + local ok, result = client.validateServerHost("") + assert.is_true(ok) + assert.are_same("localhost:11434", result) + end) + end) + + -------------------------------------------------------------------- + -- Valid inputs + -------------------------------------------------------------------- + describe("valid inputs", function() + it("accepts localhost with default port", function() + local ok, result = client.validateServerHost("localhost:11434") + assert.is_true(ok) + assert.are_same("localhost:11434", result) + end) + + it("accepts IP address with port", function() + local ok, result = client.validateServerHost("192.168.1.10:11434") + assert.is_true(ok) + assert.are_same("192.168.1.10:11434", result) + end) + + it("accepts domain name with port", function() + local ok, result = client.validateServerHost("ollama.example.com:9000") + assert.is_true(ok) + assert.are_same("ollama.example.com:9000", result) + end) + + it("strips http:// scheme", function() + local ok, result = client.validateServerHost("http://localhost:11434") + assert.is_true(ok) + assert.are_same("localhost:11434", result) + end) + + it("strips https:// scheme", function() + local ok, result = client.validateServerHost("https://myserver:8080") + assert.is_true(ok) + assert.are_same("myserver:8080", result) + end) + + it("strips trailing slashes", function() + local ok, result = client.validateServerHost("localhost:11434///") + assert.is_true(ok) + assert.are_same("localhost:11434", result) + end) + + it("handles scheme + trailing slash combo", function() + local ok, result = client.validateServerHost("http://host:8080/") + assert.is_true(ok) + assert.are_same("host:8080", result) + end) + + it("accepts port edge values", function() + local ok1, _ = client.validateServerHost("h:1") + assert.is_true(ok1) + local ok2, _ = client.validateServerHost("h:65535") + assert.is_true(ok2) + end) + end) + + -------------------------------------------------------------------- + -- Invalid inputs + -------------------------------------------------------------------- + describe("invalid inputs", function() + it("rejects missing port", function() + local ok, err = client.validateServerHost("localhost") + assert.is_false(ok) + assert.is_not_nil(err) + end) + + it("rejects non-numeric port", function() + local ok, err = client.validateServerHost("host:abc") + assert.is_false(ok) + end) + + it("rejects port > 65535", function() + local ok, err = client.validateServerHost("h:99999") + assert.is_false(ok) + end) + + it("rejects port 0", function() + local ok, err = client.validateServerHost("h:0") + assert.is_false(ok) + end) + + it("rejects hostname with invalid characters", function() + local ok, err = client.validateServerHost("host name:80") + assert.is_false(ok) + end) + + it("rejects bare scheme without host", function() + local ok, err = client.validateServerHost("http://") + assert.is_false(ok) + end) + end) +end) diff --git a/tests/spec/prompt_builder_spec.lua b/tests/spec/prompt_builder_spec.lua new file mode 100644 index 0000000..6ae5ed0 --- /dev/null +++ b/tests/spec/prompt_builder_spec.lua @@ -0,0 +1,139 @@ +--- prompt_builder_spec.lua — Unit tests for PromptBuilder.lua +--- Tests user-prompt assembly, request-body construction, and system prompts. + +local path = PLUGIN_PATH + +describe("PromptBuilder", function() + local prompt + + before_each(function() + prompt = assert(loadfile(path .. "PromptBuilder.lua"))() + end) + + -------------------------------------------------------------------- + -- System prompts exist and are non-empty + -------------------------------------------------------------------- + describe("system prompts", function() + it("has a default system prompt", function() + assert.is_true(#prompt.defaultSystemPrompt > 100) + assert.is_not_nil(string.find(prompt.defaultSystemPrompt, "title")) + assert.is_not_nil(string.find(prompt.defaultSystemPrompt, "caption")) + assert.is_not_nil(string.find(prompt.defaultSystemPrompt, "keywords")) + end) + + it("has a single-photo system prompt", function() + assert.is_true(#prompt.singlePhotoSystemPrompt > 50) + end) + + it("uses different prompts for batch vs single-photo modes", function() + assert.is_not.equal( + prompt.defaultSystemPrompt, + prompt.singlePhotoSystemPrompt + ) + end) + end) + + -------------------------------------------------------------------- + -- buildUserPrompt + -------------------------------------------------------------------- + describe("buildUserPrompt", function() + it("returns instruction when no current data", function() + local got = prompt.buildUserPrompt("Caption this photo", nil, false) + assert.are_same("Caption this photo", got) + end) + + it("returns instruction when useCurrentData is false", function() + local got = prompt.buildUserPrompt( + "Go", { title = "T", caption = "C" }, false + ) + assert.are_same("Go", got) + end) + + it("prepends title and caption when useCurrentData is true", function() + local got = prompt.buildUserPrompt( + "Update metadata", + { title = "Sunset", caption = "Golden hour beach" }, + true + ) + assert.are_same( + "Title: Sunset Caption: Golden hour beach Update metadata", + got + ) + end) + + it("handles empty strings for title/caption", function() + local got = prompt.buildUserPrompt( + "Do it", + { title = "", caption = "" }, + true + ) + assert.are_same("Title: Caption: Do it", got) + end) + + it("escapes double quotes in title and caption", function() + local got = prompt.buildUserPrompt( + "Go", + { title = 'He said "hi"', caption = 'It\'s "great"' }, + true + ) + assert.are_same( + [[Title: He said \"hi\" Caption: It's \"great\" Go]], + got + ) + end) + + it("handles nil title or caption gracefully", function() + local got = prompt.buildUserPrompt( + "Go", + { title = nil, caption = "C" }, + true + ) + assert.are_same("Title: Caption: C Go", got) + end) + end) + + -------------------------------------------------------------------- + -- assembleRequestBody + -------------------------------------------------------------------- + describe("assembleRequestBody", function() + it("includes model, prompt, format, and stream fields", function() + local body = prompt.assembleRequestBody( + "Caption this", "gemma4:latest", true, nil + ) + assert.are_same("gemma4:latest", body.model) + assert.are_same("Caption this", body.prompt) + assert.are_same("json", body.format) + assert.is_false(body.stream) + end) + + it("includes system prompt when useSystemPrompt is true", function() + local body = prompt.assembleRequestBody( + "Go", "llama3", true, nil + ) + assert.is_not_nil(body.system) + assert.is_same(prompt.defaultSystemPrompt, body.system) + end) + + it("omits system prompt when useSystemPrompt is false", function() + local body = prompt.assembleRequestBody( + "Go", "llama3", false, nil + ) + assert.is_nil(body.system) + end) + + it("uses override system prompt when provided", function() + local custom = "You are a pirate." + local body = prompt.assembleRequestBody( + "Arrr", "gemma4:latest", true, custom + ) + assert.are_same(custom, body.system) + end) + + it("does not include images field", function() + local body = prompt.assembleRequestBody( + "Go", "model", true, nil + ) + assert.is_nil(body.images) + end) + end) +end) diff --git a/tests/spec/response_validator_spec.lua b/tests/spec/response_validator_spec.lua new file mode 100644 index 0000000..3ee4ac7 --- /dev/null +++ b/tests/spec/response_validator_spec.lua @@ -0,0 +1,146 @@ +--- response_validator_spec.lua — Unit tests for ResponseValidator.lua +--- Tests fence stripping, envelope parsing, schema validation, and the full pipeline. + +local path = PLUGIN_PATH + +describe("ResponseValidator", function() + local validate + + before_each(function() + -- Reload fresh each time so cached JSON state doesn't leak between tests + validate = assert(loadfile(path .. "ResponseValidator.lua"))() + end) + + -------------------------------------------------------------------- + -- stripMarkdownFences + -------------------------------------------------------------------- + describe("stripMarkdownFences", function() + it("returns bare JSON unchanged", function() + local json = '{"title":"A","caption":"B","keywords":["c"]}' + assert.are.same(json, validate.stripMarkdownFences(json)) + end) + + it("strips ```json ... ``` fences", function() + local raw = "```json\n{\"title\":\"A\"}\n```" + local got = validate.stripMarkdownFences(raw) + assert.are_same('{"title":"A"}', got) + end) + + it("strips bare ``` fences without language tag", function() + local raw = "```\n{}\n```" + local got = validate.stripMarkdownFences(raw) + assert.are_same('{}', got) + end) + + it("handles leading/trailing whitespace around fences", function() + local raw = " ```json\n{\"x\":1}\n``` " + local got = validate.stripMarkdownFences(raw) + assert.are_same('{"x":1}', got) + end) + end) + + -------------------------------------------------------------------- + -- parseApiEnvelope + -------------------------------------------------------------------- + describe("parseApiEnvelope", function() + it("decodes a valid envelope with response field", function() + local envelope = '{"response":"hello world"}' + local data, err = validate.parseApiEnvelope(envelope) + assert.falsy(err) + assert.are_same("hello world", data.response) + end) + + it("rejects malformed JSON", function() + local data, err = validate.parseApiEnvelope("{bad json") + assert.falsy(data) + assert.is_not_nil(err) + end) + + it("rejects an envelope missing the response field", function() + local data, err = validate.parseApiEnvelope('{"foo":1}') + assert.falsy(data) + assert.is_not_nil(err) + end) + end) + + -------------------------------------------------------------------- + -- checkMetadataSchema + -------------------------------------------------------------------- + describe("checkMetadataSchema", function() + it("accepts valid title, caption, keywords", function() + local ok, err = validate.checkMetadataSchema({ + title = "Sunset over ocean", + caption = "A beautiful scene", + keywords = { "ocean", "sunset" }, + }) + assert.is_true(ok) + assert.falsy(err) + end) + + it("rejects missing title", function() + local ok, err = validate.checkMetadataSchema({ + caption = "ok", keywords = { "a" } + }) + assert.is_false(ok) + assert.is_not_nil(err) + end) + + it("rejects empty keyword string in array", function() + local ok, err = validate.checkMetadataSchema({ + title = "T", caption = "C", keywords = { "", "b" } + }) + assert.is_false(ok) + assert.is_not_nil(err) + end) + + it("rejects whitespace-only keyword", function() + local ok, err = validate.checkMetadataSchema({ + title = "T", caption = "C", keywords = { " ", "b" } + }) + assert.is_false(ok) + end) + end) + + -------------------------------------------------------------------- + -- validateAndParse (full pipeline) + -------------------------------------------------------------------- + describe("validateAndParse", function() + local function makeEnvelope(innerJson) + -- Build a proper outer envelope string. The JSON.lua decoder is + --- available via _PLUGIN + LrPathUtils, but simpler to inline. + return '{"response":' .. innerJson .. '}' + end + + it("returns parsed metadata for clean JSON response", function() + local inner = [[{"title":"Beach sunset","caption":"Waves roll in","keywords":["beach","sunset"]}]] + -- Need to escape quotes for embedding inside the envelope JSON string + local envelope = '{"response":' .. '"' .. string.gsub(inner, '"', '\\"') .. '"}' + local metadata, err = validate.validateAndParse(envelope) + assert.falsy(err) + assert.are_same("Beach sunset", metadata.title) + end) + + it("handles markdown-fenced inner JSON", function() + -- Build: {"response": "```json\n{...}\n```"} + local fenced = '```json\n{"title":"X","caption":"Y","keywords":["z"]}\n```' + local escaped = string.gsub(fenced, '"', '\\"') + local envelope = '{"response":"' .. escaped .. '"}' + local metadata, err = validate.validateAndParse(envelope) + assert.falsy(err) + assert.are_same("X", metadata.title) + end) + + it("returns error for non-json response body", function() + local envelope = '{"response":"just plain text"}' + local metadata, err = validate.validateAndParse(envelope) + assert.falsy(metadata) + assert.is_not_nil(err) + end) + + it("returns error when envelope is invalid", function() + local metadata, err = validate.validateAndParse("not json at all") + assert.falsy(metadata) + assert.is_not_nil(err) + end) + end) +end) From d7299ec6bde35ec51b2f91841fb1e504af2dc82a Mon Sep 17 00:00:00 2001 From: Zack Stout Date: Fri, 17 Jul 2026 09:17:25 -0500 Subject: [PATCH 02/10] fix: wire production code to tested parseKeywordCsv parser The inline string.gmatch keyword loop in LrLlama.lua duplicated the logic already tested in MetadataService.parseKeywordCsv. Replace it with the Common delegation layer so production and tests share the same code path. - Export parseKeywordCsv from Common.lua as a MetadataService delegation - Replace 4-line inline parsing loop in LrLlama.lua with Common.parseKeywordCsv - Add parseKeywordCsv to Common.lua export checklist and delegation behavior test - Add production wiring assertion verifying LrLlama.lua calls the tested parser - Add Lua tests GitHub Actions workflow file 65 tests pass. --- .github/workflows/lua-tests.yml | 37 +++++++++++++++++++++++++++ lightroom-llama.lrplugin/Common.lua | 1 + lightroom-llama.lrplugin/LrLlama.lua | 5 +--- tests/spec/common_delegation_spec.lua | 25 ++++++++++++++++++ 4 files changed, 64 insertions(+), 4 deletions(-) create mode 100644 .github/workflows/lua-tests.yml diff --git a/.github/workflows/lua-tests.yml b/.github/workflows/lua-tests.yml new file mode 100644 index 0000000..4d4fe65 --- /dev/null +++ b/.github/workflows/lua-tests.yml @@ -0,0 +1,37 @@ +name: Lua tests + +on: + push: + pull_request: + branches: + - main + workflow_dispatch: + +permissions: + contents: read + +jobs: + test: + name: Lua 5.1 / Busted + runs-on: ubuntu-latest + timeout-minutes: 10 + + steps: + - name: Check out repository + uses: actions/checkout@v6 + + - name: Install Lua 5.1.5 + uses: leafo/gh-actions-lua@v13 + with: + luaVersion: "5.1.5" + + - name: Install LuaRocks + uses: leafo/gh-actions-luarocks@v6 + + - name: Install Busted + run: luarocks install busted 2.3.0-1 + + - name: Run tests + run: | + chmod +x tests/run_tests.sh + make test \ No newline at end of file diff --git a/lightroom-llama.lrplugin/Common.lua b/lightroom-llama.lrplugin/Common.lua index fe1eeae..12b1d7a 100644 --- a/lightroom-llama.lrplugin/Common.lua +++ b/lightroom-llama.lrplugin/Common.lua @@ -44,4 +44,5 @@ return { addKeywordsWithParent = MetadataService.addKeywordsWithParent, getLlmKeywordsFromPhoto = MetadataService.getLlmKeywordsFromPhoto, removeLlmKeywords = MetadataService.removeLlmKeywords, + parseKeywordCsv = MetadataService.parseKeywordCsv, } diff --git a/lightroom-llama.lrplugin/LrLlama.lua b/lightroom-llama.lrplugin/LrLlama.lua index ff46622..508bb77 100644 --- a/lightroom-llama.lrplugin/LrLlama.lua +++ b/lightroom-llama.lrplugin/LrLlama.lua @@ -291,10 +291,7 @@ local function main() selectedPhoto:setRawMetadata("caption", props.caption) -- Parse keywords from comma-separated string and add with llm parent if props.keywords and props.keywords ~= "" then - local keywordList = {} - for keyword in string.gmatch(props.keywords, "([^,]+)") do - table.insert(keywordList, keyword:match("^%s*(.-)%s*$")) -- trim whitespace - end + local keywordList = Common.parseKeywordCsv(props.keywords) Common.addKeywordsWithParent(catalog, selectedPhoto, keywordList) end end) diff --git a/tests/spec/common_delegation_spec.lua b/tests/spec/common_delegation_spec.lua index c7ba38f..bc68648 100644 --- a/tests/spec/common_delegation_spec.lua +++ b/tests/spec/common_delegation_spec.lua @@ -33,6 +33,7 @@ describe("Common.lua delegation", function() assert.is_function(Common.addKeywordsWithParent) assert.is_function(Common.getLlmKeywordsFromPhoto) assert.is_function(Common.removeLlmKeywords) + assert.is_function(Common.parseKeywordCsv) end) it("exports correct default values", function() @@ -83,4 +84,28 @@ describe("Common.lua delegation", function() assert.is_function(Common.fetchAvailableModels) end) end) + + describe("parseKeywordCsv delegation", function() + it("parses tricky CSV correctly", function() + local got = Common.parseKeywordCsv("sunset,, beach, ,portrait") + assert.are_same({ "sunset", "beach", "portrait" }, got) + end) + end) + + -------------------------------------------------------------------- + -- Production wiring: LrLlama.lua must use the tested parser + -------------------------------------------------------------------- + describe("production wiring", function() + it("uses Common.parseKeywordCsv and has no inline keyword parsing loop", function() + local source = assert(io.open(path .. "LrLlama.lua", "r")):read("*a") + assert.truthy( + string.find(source, "Common%.parseKeywordCsv"), + "LrLlama.lua should call Common.parseKeywordCsv" + ) + assert.falsy( + string.find(source, "gmatch.*keywords.*%[%,", nil, true), + "LrLlama.lua should not contain inline gmatch keyword parsing" + ) + end) + end) end) From fcbb47ace0c85cf8723230e377c5b3bd9d70e427 Mon Sep 17 00:00:00 2001 From: Zack Stout Date: Fri, 17 Jul 2026 10:19:25 -0500 Subject: [PATCH 03/10] refactor: add DI constructor to OllamaClient with comprehensive test coverage - Wrap OllamaClient module in createClient(deps) constructor for testability while maintaining backward compatibility (existing callers see no changes) - Expose defaultClient.new so tests can inject fake dependencies - Add 27 tests for getBaseUrl, fetchModels, makeModelItems, saveServerAndRefresh - Add 13 tests for generate() pipeline: success path, retry logic, error propagation, temp-file cleanup, prompt construction, validation delegation - All 104 tests pass; Lua syntax valid across plugin and test files Co-Authored-By: Claude --- lightroom-llama.lrplugin/OllamaClient.lua | 428 ++++++++++-------- tests/spec/ollama_client_generate_spec.lua | 425 ++++++++++++++++++ tests/spec/ollama_client_models_spec.lua | 496 +++++++++++++++++++++ 3 files changed, 1151 insertions(+), 198 deletions(-) create mode 100644 tests/spec/ollama_client_generate_spec.lua create mode 100644 tests/spec/ollama_client_models_spec.lua diff --git a/lightroom-llama.lrplugin/OllamaClient.lua b/lightroom-llama.lrplugin/OllamaClient.lua index 1487f7e..26c0a44 100644 --- a/lightroom-llama.lrplugin/OllamaClient.lua +++ b/lightroom-llama.lrplugin/OllamaClient.lua @@ -19,242 +19,274 @@ local ThumbnailService = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "Thumb local PromptBuilder = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "PromptBuilder.lua"))))() local ResponseValidator = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "ResponseValidator.lua"))))() -local client = {} - --------------------------------------------------------------------------------- --- Constants --------------------------------------------------------------------------------- - ---- Default Ollama model name; change here to switch models. -client.defaultModel = "gemma4:latest" - ---- Fallback server address when prefs are missing, empty, or invalid. -client.defaultServerHost = "localhost:11434" - -------------------------------------------------------------------------------- --- Server configuration +--- Constructor — create a client with injectable dependencies. +--- Each dep falls back to the real Lightroom SDK when omitted, so production +--- callers need no changes while tests can provide lightweight fakes. +---@param deps table|nil Optional dependency overrides: +--- { http, prefs, tasks, json, thumbnailService, promptBuilder, responseValidator, logger } +---@return table client Fully initialised OllamaClient instance -------------------------------------------------------------------------------- +local function createClient(deps) + deps = deps or {} + + local http = deps.http or LrHttp + local prefsService = deps.prefs or LrPrefs + local tasks = deps.tasks or LrTasks + local json = deps.json or JSON + local thumbnailService = deps.thumbnailService or ThumbnailService + local promptBuilder = deps.promptBuilder or PromptBuilder + local responseValidator = deps.responseValidator or ResponseValidator + local activeLogger = deps.logger or logger + + local client = {} + + -------------------------------------------------------------------------------- + -- Constants + -------------------------------------------------------------------------------- + + --- Default Ollama model name; change here to switch models. + client.defaultModel = "gemma4:latest" + + --- Fallback server address when prefs are missing, empty, or invalid. + client.defaultServerHost = "localhost:11434" + + -------------------------------------------------------------------------------- + -- Server configuration + -------------------------------------------------------------------------------- + + --- Validate and normalize an Ollama server address. + --- Strips optional scheme (`http://`) and trailing slashes, then verifies the + --- string is `hostname:port`. Accepts localhost, IP addresses, and domain names. + ---@param input string|nil User-supplied server address (may be empty) + ---@return bool ok True when the address is valid + ---@return string result The normalized host:port on success, error message on failure + function client.validateServerHost(input) + if not input or input == "" then + return true, client.defaultServerHost -- treat empty as "use default" + end + -- Strip scheme and trailing slashes for normalization + local host = string.gsub(input, "^https?://", "") + host = string.gsub(host, "/+$", "") + + -- Must match something:digit (hostname or IP followed by colon and port) + local hostnamePart, portPart = string.match(host, "^([^:]+):(%d+)$") + if not hostnamePart or not portPart then + return false, "Server address must be in host:port format (e.g., localhost:11434)" + end ---- Validate and normalize an Ollama server address. ---- Strips optional scheme (`http://`) and trailing slashes, then verifies the ---- string is `hostname:port`. Accepts localhost, IP addresses, and domain names. ----@param input string|nil User-supplied server address (may be empty) ----@return bool ok True when the address is valid ----@return string result The normalized host:port on success, error message on failure -function client.validateServerHost(input) - if not input or input == "" then - return true, client.defaultServerHost -- treat empty as "use default" - end - -- Strip scheme and trailing slashes for normalization - local host = string.gsub(input, "^https?://", "") - host = string.gsub(host, "/+$", "") - - -- Must match something:digit (hostname or IP followed by colon and port) - local hostnamePart, portPart = string.match(host, "^([^:]+):(%d+)$") - if not hostnamePart or not portPart then - return false, "Server address must be in host:port format (e.g., localhost:11434)" - end + local port = tonumber(portPart) + if port < 1 or port > 65535 then + return false, "Port number must be between 1 and 65535" + end - local port = tonumber(portPart) - if port < 1 or port > 65535 then - return false, "Port number must be between 1 and 65535" - end + -- hostnamePart should contain at least one valid character (alphanumeric, dot, hyphen) + if not string.match(hostnamePart, "^[%a%d%.%-]+$") then + return false, "Hostname contains invalid characters" + end - -- hostnamePart should contain at least one valid character (alphanumeric, dot, hyphen) - if not string.match(hostnamePart, "^[%a%d%.%-]+$") then - return false, "Hostname contains invalid characters" + return true, hostnamePart .. ":" .. portPart end - return true, hostnamePart .. ":" .. portPart -end + --- Build the base URL from saved preferences. + --- Reads prefs.ollamaServerHost, validates, falls back to default if malformed. + ---@param prefs table LrPrefs.prefsForPlugin() instance (injected for testability) + ---@return string baseUrl Full URL like "http://localhost:11434" + function client.getBaseUrl(prefs) + local ok, host = client.validateServerHost(prefs.ollamaServerHost) ---- Build the base URL from saved preferences. ---- Reads prefs.ollamaServerHost, validates, falls back to default if malformed. ----@param prefs table LrPrefs.prefsForPlugin() instance (injected for testability) ----@return string baseUrl Full URL like "http://localhost:11434" -function client.getBaseUrl(prefs) - local ok, host = client.validateServerHost(prefs.ollamaServerHost) + if not ok then + activeLogger:warn("Invalid server host in prefs: " .. tostring(prefs.ollamaServerHost)) + host = client.defaultServerHost + end - if not ok then - logger:warn("Invalid server host in prefs: " .. tostring(prefs.ollamaServerHost)) - host = client.defaultServerHost + return "http://" .. host end - return "http://" .. host -end - --------------------------------------------------------------------------------- --- Model discovery --------------------------------------------------------------------------------- - ---- Convert a plain list of model names into the `{title, value}` format that ---- LrView `popup_menu` requires for its `items` binding. ----@param modelNames table List of model strings (from fetchModels) ----@return table
Table of `{title = m, value = m}` entries for popup_menu binding -function client.makeModelItems(modelNames) - local items = {} - for _, m in ipairs(modelNames) do - table.insert(items, { title = m, value = m }) + -------------------------------------------------------------------------------- + -- Model discovery + -------------------------------------------------------------------------------- + + --- Convert a plain list of model names into the `{title, value}` format that + --- LrView `popup_menu` requires for its `items` binding. + ---@param modelNames table List of model strings (from fetchModels) + ---@return table
Table of `{title = m, value = m}` entries for popup_menu binding + function client.makeModelItems(modelNames) + local items = {} + for _, m in ipairs(modelNames) do + table.insert(items, { title = m, value = m }) + end + return items end - return items -end ---- Query Ollama's `/api/tags` endpoint for installed models. ---- Falls back to `{defaultModel}` on network errors, malformed JSON, or an empty ---- model list — so callers never receive nil. ----@param prefs table LrPrefs instance ----@return table List of model name strings; always non-empty. -function client.fetchModels(prefs) - logger:info("Fetching available models from Ollama") + --- Query Ollama's `/api/tags` endpoint for installed models. + --- Falls back to `{defaultModel}` on network errors, malformed JSON, or an empty + --- model list — so callers never receive nil. + ---@param prefs table LrPrefs instance + ---@return table List of model name strings; always non-empty. + function client.fetchModels(prefs) + activeLogger:info("Fetching available models from Ollama") - local url = client.getBaseUrl(prefs) .. "/api/tags" - local response = LrHttp.get(url) + local url = client.getBaseUrl(prefs) .. "/api/tags" + local response = http.get(url) - if response then - local decodeSuccess, data = pcall(function() - return JSON:decode(response) - end) + if response then + local decodeSuccess, data = pcall(function() + return json:decode(response) + end) - if decodeSuccess and data and type(data.models) == "table" then - local modelList = {} + if decodeSuccess and data and type(data.models) == "table" then + local modelList = {} - for _, availableModel in ipairs(data.models) do - local modelName = availableModel.name or availableModel.model + for _, availableModel in ipairs(data.models) do + local modelName = availableModel.name or availableModel.model - if modelName and modelName ~= "" then - table.insert(modelList, modelName) + if modelName and modelName ~= "" then + table.insert(modelList, modelName) + end end - end - if #modelList > 0 then - logger:info("Found " .. #modelList .. " models") - return modelList - end + if #modelList > 0 then + activeLogger:info("Found " .. #modelList .. " models") + return modelList + end - logger:warn("Ollama returned an empty model list") - elseif not decodeSuccess then - logger:warn( - "Failed to decode Ollama model response: " .. tostring(data) - ) + activeLogger:warn("Ollama returned an empty model list") + elseif not decodeSuccess then + activeLogger:warn( + "Failed to decode Ollama model response: " .. tostring(data) + ) + else + activeLogger:warn("Ollama response did not contain a valid model list") + end else - logger:warn("Ollama response did not contain a valid model list") + activeLogger:warn("No response received from Ollama") end - else - logger:warn("No response received from Ollama") + + activeLogger:warn("Using default model: " .. client.defaultModel) + return { client.defaultModel } end - logger:warn("Using default model: " .. client.defaultModel) - return { client.defaultModel } -end + --- Persist the server host and rebuild the model dropdown. + --- MUST be called from within `LrTasks.startAsyncTask()` — LrBinding only + --- propagates property reassignments (modelItems, selectedModel) when done + --- off the UI thread. + ---@param props table LrBinding property table containing serverHost, modelItems, selectedModel + ---@param prefs table LrPrefs.prefsForPlugin() instance + ---@return bool ok True on success + ---@return string message Human-readable status or error description + function client.saveServerAndRefresh(props, prefs) + local ok, validatedHost = client.validateServerHost(props.serverHost) + if not ok then + return false, validatedHost -- caller displays error + end ---- Persist the server host and rebuild the model dropdown. ---- MUST be called from within `LrTasks.startAsyncTask()` — LrBinding only ---- propagates property reassignments (modelItems, selectedModel) when done ---- off the UI thread. ----@param props table LrBinding property table containing serverHost, modelItems, selectedModel ----@param prefs table LrPrefs.prefsForPlugin() instance ----@return bool ok True on success ----@return string message Human-readable status or error description -function client.saveServerAndRefresh(props, prefs) - local ok, validatedHost = client.validateServerHost(props.serverHost) - if not ok then - return false, validatedHost -- caller displays error - end + prefs.ollamaServerHost = validatedHost - prefs.ollamaServerHost = validatedHost + local availableModels = client.fetchModels(prefs) + if not availableModels or #availableModels == 0 then + return false, "No models found on server" + end - local availableModels = client.fetchModels(prefs) - if not availableModels or #availableModels == 0 then - return false, "No models found on server" - end + -- Rebuild model list. This function is intended to be called from inside + -- LrTasks.startAsyncTask() so that LrBinding picks up the property + -- reassignment and propagates it to the dialog's popup_menu. + local oldSelection = props.selectedModel or "" + props.modelItems = client.makeModelItems(availableModels) - -- Rebuild model list. This function is intended to be called from inside - -- LrTasks.startAsyncTask() so that LrBinding picks up the property - -- reassignment and propagates it to the dialog's popup_menu. - local oldSelection = props.selectedModel or "" - props.modelItems = client.makeModelItems(availableModels) + -- Keep old selection if it still exists in the new list + local found = false + for _, m in ipairs(availableModels) do + if m == oldSelection then found = true; break; end + end + props.selectedModel = found and oldSelection or availableModels[1] - -- Keep old selection if it still exists in the new list - local found = false - for _, m in ipairs(availableModels) do - if m == oldSelection then found = true; break; end + return true, string.format("Loaded %d model(s)", #availableModels) end - props.selectedModel = found and oldSelection or availableModels[1] - - return true, string.format("Loaded %d model(s)", #availableModels) -end --------------------------------------------------------------------------------- --- Generate pipeline --------------------------------------------------------------------------------- + -------------------------------------------------------------------------------- + -- Generate pipeline + -------------------------------------------------------------------------------- + + --- High-level generate: export thumbnail, encode, build prompt, POST, validate response. + --- Orchestrates ThumbnailService + PromptBuilder + ResponseValidator internally. + --- This is the decomposition of the former Common.sendDataToApi mega-function. + --- Retries thumbnail export up to 3 times. Cleans up the temp file after the + --- HTTP request regardless of success or failure. Validates the JSON response + --- has title (string), caption (string), and keywords (array of non-empty strings). + ---@param photo LrPhoto Photo object from the catalog + ---@param userInstruction string User-facing prompt text + ---@param currentData table|nil Existing title/caption to prepend when useCurrentData is true + ---@param useCurrentData boolean Whether to include current metadata in the prompt + ---@param useSystemPrompt boolean Whether to send a system prompt alongside the user prompt + ---@param selectedModel string|nil Model name; falls back to the default if nil + ---@param systemPromptOverride string|nil Override for the built-in system prompt + ---@return table|nil response Parsed JSON on success ({title, caption, keywords}) + ---@return string|nil err Error message, or nil on success + function client.generate(photo, userInstruction, currentData, useCurrentData, + useSystemPrompt, selectedModel, systemPromptOverride) + activeLogger:info("Sending data to API") + + -- 1. Export thumbnail with retry (up to 3 attempts) + local thumbnailPath = nil + for attempt = 1, 3 do + thumbnailPath = thumbnailService.export(photo) + if thumbnailPath then + break + end + activeLogger:warn("Thumbnail export attempt " .. attempt .. " failed, retrying...") + tasks.sleep(0.5) -- Wait 500ms before retry + end ---- High-level generate: export thumbnail, encode, build prompt, POST, validate response. ---- Orchestrates ThumbnailService + PromptBuilder + ResponseValidator internally. ---- This is the decomposition of the former Common.sendDataToApi mega-function. ---- Retries thumbnail export up to 3 times. Cleans up the temp file after the ---- HTTP request regardless of success or failure. Validates the JSON response ---- has title (string), caption (string), and keywords (array of non-empty strings). ----@param photo LrPhoto Photo object from the catalog ----@param userInstruction string User-facing prompt text ----@param currentData table|nil Existing title/caption to prepend when useCurrentData is true ----@param useCurrentData boolean Whether to include current metadata in the prompt ----@param useSystemPrompt boolean Whether to send a system prompt alongside the user prompt ----@param selectedModel string|nil Model name; falls back to the default if nil ----@param systemPromptOverride string|nil Override for the built-in system prompt ----@return table|nil response Parsed JSON on success ({title, caption, keywords}) ----@return string|nil err Error message, or nil on success -function client.generate(photo, userInstruction, currentData, useCurrentData, - useSystemPrompt, selectedModel, systemPromptOverride) - logger:info("Sending data to API") - - -- 1. Export thumbnail with retry (up to 3 attempts) - local thumbnailPath = nil - for attempt = 1, 3 do - thumbnailPath = ThumbnailService.export(photo) - if thumbnailPath then - break + if not thumbnailPath then + return nil, "Failed to export thumbnail after 3 attempts" end - logger:warn("Thumbnail export attempt " .. attempt .. " failed, retrying...") - LrTasks.sleep(0.5) -- Wait 500ms before retry - end - if not thumbnailPath then - return nil, "Failed to export thumbnail after 3 attempts" - end + -- 2. Encode image as Base64 + local encodedImage = thumbnailService.encodeBase64(thumbnailPath) + if not encodedImage then + thumbnailService.cleanup(thumbnailPath) + return nil, "Failed to encode image" + end - -- 2. Encode image as Base64 - local encodedImage = ThumbnailService.encodeBase64(thumbnailPath) - if not encodedImage then - ThumbnailService.cleanup(thumbnailPath) - return nil, "Failed to encode image" - end + -- 3. Build user prompt and request body + local builtPrompt = promptBuilder.buildUserPrompt(userInstruction, currentData, useCurrentData) + local requestBody = promptBuilder.assembleRequestBody( + builtPrompt, selectedModel or client.defaultModel, + useSystemPrompt, systemPromptOverride + ) + requestBody.images = {encodedImage} + + -- 4. Send HTTP POST + local url = client.getBaseUrl(prefsService.prefsForPlugin()) .. "/api/generate" + local jsonPayload = json:encode(requestBody) + + local response = http.post(url, jsonPayload, {{ + field = "Content-Type", + value = "application/json" + }}) + + -- 5. Clean up thumbnail file (always, regardless of success) + thumbnailService.cleanup(thumbnailPath) + + -- 6. Parse and validate response + if not response then + return nil, "Failed to send data to the API" + end - -- 3. Build user prompt and request body - local builtPrompt = PromptBuilder.buildUserPrompt(userInstruction, currentData, useCurrentData) - local requestBody = PromptBuilder.assembleRequestBody( - builtPrompt, selectedModel or client.defaultModel, - useSystemPrompt, systemPromptOverride - ) - requestBody.images = {encodedImage} - - -- 4. Send HTTP POST - local url = client.getBaseUrl(LrPrefs.prefsForPlugin()) .. "/api/generate" - local jsonPayload = JSON:encode(requestBody) - - local response = LrHttp.post(url, jsonPayload, {{ - field = "Content-Type", - value = "application/json" - }}) - - -- 5. Clean up thumbnail file (always, regardless of success) - ThumbnailService.cleanup(thumbnailPath) - - -- 6. Parse and validate response - if not response then - return nil, "Failed to send data to the API" + return responseValidator.validateAndParse(response) end - return ResponseValidator.validateAndParse(response) + return client end -return client +-- Create the default production client with real Lightroom SDK dependencies. +local defaultClient = createClient() + +--- Create a new OllamaClient with optional dependency overrides. +--- Tests inject fake deps; production callers may omit deps entirely. +---@param deps table|nil Optional dependency overrides +---@return table client New client instance +defaultClient.new = createClient + +return defaultClient diff --git a/tests/spec/ollama_client_generate_spec.lua b/tests/spec/ollama_client_generate_spec.lua new file mode 100644 index 0000000..b866ed1 --- /dev/null +++ b/tests/spec/ollama_client_generate_spec.lua @@ -0,0 +1,425 @@ +--- ollama_client_generate_spec.lua — Unit tests for OllamaClient.generate() pipeline. +--- Covers success path, retry behavior, error propagation, and temp-file cleanup. + +local path = PLUGIN_PATH + +-------------------------------------------------------------------------------- +--- Helper: build fakes aligned with how OllamaClient calls each dependency. +--- Dot-notation deps get no self; json uses colon so it does. +-------------------------------------------------------------------------------- +local function makeFakes() + local http = { + post_calls = {}, + post_response = nil, + } + http.post = function(url, body, headers) + table.insert(http.post_calls, { url = url, body = body, headers = headers }) + return http.post_response + end + + local prefs = {} + prefs.prefsForPlugin = function() + return { ollamaServerHost = prefs.ollamaServerHost or "localhost:11434" } + end + + local tasks = { sleep_calls = {} } + tasks.sleep = function(seconds) + table.insert(tasks.sleep_calls, seconds) + end + + -- JSON uses colon notation (json:encode / json:decode) — self is first arg. + local json = { + encode_calls = {}, + encode_return = nil, + } + json.encode = function(_, tbl) + table.insert(json.encode_calls, tbl) + return json.encode_return or "{}" + end + + local thumbnailService = { + export_return = nil, + encodeBase64_return = nil, + cleanup_calls = {}, + export_calls = {}, + } + -- Dot-notation: no implicit self (OllamaClient calls thumbnailService.export(photo)) + thumbnailService.export = function(photo) + table.insert(thumbnailService.export_calls, photo) + return thumbnailService.export_return + end + thumbnailService.encodeBase64 = function(imagePath) + return thumbnailService.encodeBase64_return + end + thumbnailService.cleanup = function(imagePath) + table.insert(thumbnailService.cleanup_calls, imagePath) + end + + local promptBuilder = { + buildUserPrompt_calls = {}, + assembleRequestBody_calls = {}, + buildUserPrompt_return = "default prompt", + assembleRequestBody_return = { model = "test", prompt = "p" }, + } + -- Dot-notation: no implicit self (OllamaClient calls promptBuilder.buildUserPrompt(...)) + promptBuilder.buildUserPrompt = function(instruction, currentData, useCurrent) + table.insert(promptBuilder.buildUserPrompt_calls, { + instruction = instruction, + currentData = currentData, + useCurrent = useCurrent + }) + return promptBuilder.buildUserPrompt_return + end + promptBuilder.assembleRequestBody = function(userPrompt, model, useSys, override) + table.insert(promptBuilder.assembleRequestBody_calls, { + userPrompt = userPrompt, + model = model, + useSystemPrompt = useSys, + systemPromptOverride = override + }) + return promptBuilder.assembleRequestBody_return + end + + local responseValidator = { + validateAndParse_calls = {}, + validateAndParse_metadata = nil, + validateAndParse_error = nil, + } + -- Dot-notation: no implicit self (OllamaClient calls responseValidator.validateAndParse(...)) + responseValidator.validateAndParse = function(rawResponse) + table.insert(responseValidator.validateAndParse_calls, rawResponse) + return responseValidator.validateAndParse_metadata, + responseValidator.validateAndParse_error + end + + local logger = { + info_calls = {}, warn_calls = {}, error_calls = {}, + enable = function() end, + } + logger.info = function(msg) table.insert(logger.info_calls, msg) end + logger.warn = function(msg) table.insert(logger.warn_calls, msg) end + logger.error = function(msg) table.insert(logger.error_calls, msg) end + + return { + http = http, + prefs = prefs, + tasks = tasks, + json = json, + thumbnailService = thumbnailService, + promptBuilder = promptBuilder, + responseValidator = responseValidator, + logger = logger, + } +end + +-------------------------------------------------------------------------------- +describe("OllamaClient — generate()", function() + + -------------------------------------------------------------------- + -- Happy path + -------------------------------------------------------------------- + describe("success path", function() + it("orchestrates the full pipeline on first attempt", function() + local f = makeFakes() + f.thumbnailService.export_return = "/tmp/thumb.jpg" + f.thumbnailService.encodeBase64_return = "base64data" + f.http.post_response = '{"response":"{\"title\":\"T\",\"caption\":\"C\",\"keywords\":[\"k\"]}"}' + f.responseValidator.validateAndParse_metadata = { title = "T", caption = "C", keywords = { "k" } } + f.responseValidator.validateAndParse_error = nil + + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + + local result, err = c.generate( + { uuid = "photo-1" }, -- photo + "Caption this", -- userInstruction + nil, -- currentData + false, -- useCurrentData + true, -- useSystemPrompt + "gemma4:latest", -- selectedModel + nil -- systemPromptOverride + ) + + -- Thumbnail exported once + assert.are_same(1, #f.thumbnailService.export_calls) + assert.are_same("photo-1", f.thumbnailService.export_calls[1].uuid) + + -- Prompt built with correct args + assert.are_same(1, #f.promptBuilder.buildUserPrompt_calls) + local bpCall = f.promptBuilder.buildUserPrompt_calls[1] + assert.are_same("Caption this", bpCall.instruction) + assert.is_nil(bpCall.currentData) + assert.is_false(bpCall.useCurrent) + + -- Request body assembled + assert.are_same(1, #f.promptBuilder.assembleRequestBody_calls) + + -- Image attached to request body, then encoded + POSTed + assert.are_same(1, #f.json.encode_calls) + local encodedBody = f.json.encode_calls[1] + assert.is_true(type(encodedBody.images) == "table") + assert.are_same("base64data", encodedBody.images[1]) + + -- HTTP POST sent to correct URL + assert.are_same(1, #f.http.post_calls) + assert.are_same("http://localhost:11434/api/generate", f.http.post_calls[1].url) + + -- Cleanup always runs + assert.are_same(1, #f.thumbnailService.cleanup_calls) + assert.are_same("/tmp/thumb.jpg", f.thumbnailService.cleanup_calls[1]) + + -- ResponseValidator called with raw HTTP response + assert.are_same(1, #f.responseValidator.validateAndParse_calls) + + -- Result from validateAndParse is returned + assert.are_same("T", result.title) + assert.is_nil(err) + end) + + it("falls back to default model when selectedModel is nil", function() + local f = makeFakes() + f.thumbnailService.export_return = "/tmp/t.jpg" + f.thumbnailService.encodeBase64_return = "img" + f.http.post_response = '{}' + + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + + c.generate({ uuid = "p" }, "hi", nil, false, true, nil, nil) + + local rbCall = f.promptBuilder.assembleRequestBody_calls[1] + assert.are_same("gemma4:latest", rbCall.model) + end) + + it("passes systemPromptOverride through to assembleRequestBody", function() + local f = makeFakes() + f.thumbnailService.export_return = "/tmp/t.jpg" + f.thumbnailService.encodeBase64_return = "img" + f.http.post_response = '{}' + + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + + c.generate({ uuid = "p" }, "hi", nil, false, true, "gemma4:latest", "custom system") + + local rbCall = f.promptBuilder.assembleRequestBody_calls[1] + assert.are_same("custom system", rbCall.systemPromptOverride) + end) + end) + + -------------------------------------------------------------------- + -- Thumbnail export retry + -------------------------------------------------------------------- + describe("thumbnail retry", function() + it("retries up to 3 times when export fails", function() + local f = makeFakes() + -- First two calls return nil, third succeeds + local attempt = 0 + f.thumbnailService.export = function(photo) + attempt = attempt + 1 + if attempt < 3 then + return nil + end + return "/tmp/t.jpg" + end + f.thumbnailService.encodeBase64_return = "img" + f.http.post_response = '{}' + + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + + c.generate({ uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil) + + assert.are_same(3, attempt) + -- Sleep called between attempts (2 sleeps for 3 attempts) + assert.are_same(2, #f.tasks.sleep_calls) + end) + + it("returns error after 3 failed exports", function() + local f = makeFakes() + -- Always return nil from export + f.thumbnailService.export = function() return nil end + + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + + local result, err = c.generate( + { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil + ) + + assert.is_nil(result) + assert.are_same("Failed to export thumbnail after 3 attempts", err) + end) + + it("sleeps between retry attempts", function() + local f = makeFakes() + local attempt = 0 + f.thumbnailService.export = function() + attempt = attempt + 1 + return nil + end + + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + + c.generate({ uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil) + + -- 3 attempts → 3 sleeps (sleep after each failed attempt including last) + assert.are_same(3, #f.tasks.sleep_calls) + assert.are_same(0.5, f.tasks.sleep_calls[1]) + end) + end) + + -------------------------------------------------------------------- + -- Base64 encoding failure + -------------------------------------------------------------------- + describe("encoding failure", function() + it("returns error and cleans up when encode fails", function() + local f = makeFakes() + f.thumbnailService.export_return = "/tmp/t.jpg" + f.thumbnailService.encodeBase64_return = nil -- encoding failed + + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + + local result, err = c.generate( + { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil + ) + + assert.is_nil(result) + assert.are_same("Failed to encode image", err) + -- Cleanup happens even on failure + assert.are_same(1, #f.thumbnailService.cleanup_calls) + assert.are_same("/tmp/t.jpg", f.thumbnailService.cleanup_calls[1]) + end) + end) + + -------------------------------------------------------------------- + -- HTTP POST failure + -------------------------------------------------------------------- + describe("HTTP failure", function() + it("returns error when http.post returns nil", function() + local f = makeFakes() + f.thumbnailService.export_return = "/tmp/t.jpg" + f.thumbnailService.encodeBase64_return = "img" + f.http.post_response = nil -- network error + + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + + local result, err = c.generate( + { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil + ) + + assert.is_nil(result) + assert.are_same("Failed to send data to the API", err) + -- Cleanup still runs + assert.are_same(1, #f.thumbnailService.cleanup_calls) + end) + end) + + -------------------------------------------------------------------- + -- Response validation delegation + -------------------------------------------------------------------- + describe("response validation", function() + it("passes raw HTTP response to validateAndParse", function() + local f = makeFakes() + f.thumbnailService.export_return = "/tmp/t.jpg" + f.thumbnailService.encodeBase64_return = "img" + f.http.post_response = '{"response":"raw inner"}' + + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + + c.generate({ uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil) + + assert.are_same(1, #f.responseValidator.validateAndParse_calls) + assert.are_same('{"response":"raw inner"}', f.responseValidator.validateAndParse_calls[1]) + end) + + it("returns validation error when response is invalid", function() + local f = makeFakes() + f.thumbnailService.export_return = "/tmp/t.jpg" + f.thumbnailService.encodeBase64_return = "img" + f.http.post_response = 'invalid json' + f.responseValidator.validateAndParse_metadata = nil + f.responseValidator.validateAndParse_error = "Bad response" + + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + + local result, err = c.generate( + { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil + ) + + assert.is_nil(result) + assert.are_same("Bad response", err) + end) + end) + + -------------------------------------------------------------------- + -- Cleanup verification + -------------------------------------------------------------------- + describe("cleanup", function() + it("cleans up thumbnail even when HTTP fails", function() + local f = makeFakes() + f.thumbnailService.export_return = "/tmp/x.jpg" + f.thumbnailService.encodeBase64_return = "img" + f.http.post_response = nil + + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + + c.generate({ uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil) + + assert.are_same(1, #f.thumbnailService.cleanup_calls) + assert.are_same("/tmp/x.jpg", f.thumbnailService.cleanup_calls[1]) + end) + + it("cleans up thumbnail even when validation fails", function() + local f = makeFakes() + f.thumbnailService.export_return = "/tmp/y.jpg" + f.thumbnailService.encodeBase64_return = "img" + f.http.post_response = 'bad' + f.responseValidator.validateAndParse_metadata = nil + f.responseValidator.validateAndParse_error = "validation failed" + + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + + c.generate({ uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil) + + assert.are_same(1, #f.thumbnailService.cleanup_calls) + assert.are_same("/tmp/y.jpg", f.thumbnailService.cleanup_calls[1]) + end) + end) + + -------------------------------------------------------------------- + -- Prompt construction + -------------------------------------------------------------------- + describe("prompt construction", function() + it("builds prompt with current data when useCurrentData is true", function() + local f = makeFakes() + f.thumbnailService.export_return = "/tmp/t.jpg" + f.thumbnailService.encodeBase64_return = "img" + f.http.post_response = '{}' + + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + + c.generate( + { uuid = "p" }, + "Caption this", + { title = "Old Title", caption = "Old Caption" }, + true, -- useCurrentData + false, + "gemma4:latest", + nil + ) + + local bpCall = f.promptBuilder.buildUserPrompt_calls[1] + assert.is_true(bpCall.useCurrent) + assert.are_same("Old Title", bpCall.currentData.title) + end) + end) +end) diff --git a/tests/spec/ollama_client_models_spec.lua b/tests/spec/ollama_client_models_spec.lua new file mode 100644 index 0000000..7848b7b --- /dev/null +++ b/tests/spec/ollama_client_models_spec.lua @@ -0,0 +1,496 @@ +--- ollama_client_models_spec.lua — Unit tests for OllamaClient model-related functions. +--- Covers getBaseUrl, fetchModels, makeModelItems, saveServerAndRefresh with injected fakes. + +local path = PLUGIN_PATH + +-------------------------------------------------------------------------------- +--- Helper: build a fresh set of fake dependencies each test. +-------------------------------------------------------------------------------- +local function makeFakes() + local calls = {} + + local http = { + get_calls = {}, + post_calls = {}, + get_response = nil, + post_response = nil, + } + http.get = function(url) + table.insert(http.get_calls, url) + return http.get_response + end + http.post = function(url, body, headers) + table.insert(http.post_calls, { url = url, body = body, headers = headers }) + return http.post_response + end + + local prefs = { ollamaServerHost = nil } + prefs.prefsForPlugin = function() + return { ollamaServerHost = prefs.ollamaServerHost } + end + + local tasks = { sleep_calls = {} } + tasks.sleep = function(seconds) + table.insert(tasks.sleep_calls, seconds) + end + + -- Jeffrey Friedl's JSON uses colon-style methods (self is first arg). + -- Our fake mirrors that so the SUT calls json:decode / json:encode. + local json = { + decode_calls = {}, + encode_calls = {}, + decode_return = nil, + decode_error = nil, + encode_return = nil, + } + json.decode = function(_, str) + table.insert(json.decode_calls, str) + if json.decode_error then + error(json.decode_error) + end + return json.decode_return + end + json.encode = function(_, tbl) + table.insert(json.encode_calls, tbl) + return json.encode_return or "{}" + end + + local thumbnailService = { + export_response = nil, + encodeBase64_response = nil, + cleanup_calls = {}, + export_calls = {}, + + export = function(_, photo) + table.insert(thumbnailService.export_calls, photo) + return thumbnailService.export_response + end, + + encodeBase64 = function(_, imagePaths) + return thumbnailService.encodeBase64_response + end, + + cleanup = function(imagePath) + table.insert(thumbnailService.cleanup_calls, imagePath) + end, + } + + local promptBuilder = { + buildUserPrompt_calls = {}, + assembleRequestBody_calls = {}, + buildUserPrompt_return = "default prompt", + assembleRequestBody_return = { model = "test", prompt = "p" }, + + buildUserPrompt = function(_, instruction, currentData, useCurrent) + table.insert(promptBuilder.buildUserPrompt_calls, { + instruction = instruction, + currentData = currentData, + useCurrent = useCurrent + }) + return promptBuilder.buildUserPrompt_return + end, + + assembleRequestBody = function(_, userPrompt, model, useSys, override) + table.insert(promptBuilder.assembleRequestBody_calls, { + userPrompt = userPrompt, + model = model, + useSystemPrompt = useSys, + systemPromptOverride = override + }) + return promptBuilder.assembleRequestBody_return + end, + } + + local responseValidator = { + validateAndParse_calls = {}, + validateAndParse_metadata = nil, + validateAndParse_error = nil, + + validateAndParse = function(rawResponse) + table.insert(responseValidator.validateAndParse_calls, rawResponse) + return responseValidator.validateAndParse_metadata, + responseValidator.validateAndParse_error + end, + } + + local logger = { + info_calls = {}, warn_calls = {}, error_calls = {}, + enable = function() end, + } + logger.info = function(msg) table.insert(logger.info_calls, msg) end + logger.warn = function(msg) table.insert(logger.warn_calls, msg) end + logger.error = function(msg) table.insert(logger.error_calls, msg) end + + calls.http = http + calls.prefs = prefs + calls.tasks = tasks + calls.json = json + calls.thumbnailService = thumbnailService + calls.promptBuilder = promptBuilder + calls.responseValidator = responseValidator + calls.logger = logger + + return calls +end + +-------------------------------------------------------------------------------- +describe("OllamaClient — models & configuration", function() + + -------------------------------------------------------------------- + -- getBaseUrl() + -------------------------------------------------------------------- + describe("getBaseUrl()", function() + it("returns default URL when prefs are empty", function() + local f = makeFakes() + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + local url = c.getBaseUrl({ ollamaServerHost = nil }) + assert.are_same("http://localhost:11434", url) + end) + + it("returns default URL when prefs is nil", function() + local f = makeFakes() + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + -- validateServerHost treats nil as default + local url = c.getBaseUrl({ ollamaServerHost = nil }) + assert.are_same("http://localhost:11434", url) + end) + + it("uses saved valid host", function() + local f = makeFakes() + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + local url = c.getBaseUrl({ ollamaServerHost = "192.168.1.5:9000" }) + assert.are_same("http://192.168.1.5:9000", url) + end) + + it("handles host with http:// prefix", function() + local f = makeFakes() + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + local url = c.getBaseUrl({ ollamaServerHost = "http://myhost:11434/" }) + assert.are_same("http://myhost:11434", url) + end) + + it("falls back to default for invalid saved host", function() + local f = makeFakes() + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + local url = c.getBaseUrl({ ollamaServerHost = "invalid-no-port" }) + assert.are_same("http://localhost:11434", url) + end) + + it("includes exactly one http:// prefix", function() + local f = makeFakes() + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + local url = c.getBaseUrl({ ollamaServerHost = "localhost:11434" }) + local count = 0 + for _ in string.gmatch(url, "http://") do count = count + 1 end + assert.are_same(1, count) + end) + end) + + -------------------------------------------------------------------- + -- makeModelItems() + -------------------------------------------------------------------- + describe("makeModelItems()", function() + it("converts model names to title/value pairs", function() + local f = makeFakes() + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + local items = c.makeModelItems({ "gemma4:latest", "llama3" }) + assert.are_same(2, #items) + assert.are_same("gemma4:latest", items[1].title) + assert.are_same("gemma4:latest", items[1].value) + assert.are_same("llama3", items[2].title) + assert.are_same("llama3", items[2].value) + end) + + it("returns empty table for empty input", function() + local f = makeFakes() + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + local items = c.makeModelItems({}) + assert.are_same(0, #items) + end) + end) + + -------------------------------------------------------------------- + -- fetchModels() — success paths + -------------------------------------------------------------------- + describe("fetchModels() — success", function() + it("sends GET to /api/tags", function() + local f = makeFakes() + f.http.get_response = '{"models":[{"name":"gemma4:latest"}]}' + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + + local models = c.fetchModels({ ollamaServerHost = "localhost:11434" }) + + assert.are_same(1, #f.http.get_calls) + assert.are_same("http://localhost:11434/api/tags", f.http.get_calls[1]) + end) + + it("parses models using the name field", function() + local f = makeFakes() + f.json.decode_return = { + models = { { name = "gemma4:latest" }, { name = "llama3" } } + } + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + f.http.get_response = "{}" + local models = c.fetchModels({ ollamaServerHost = nil }) + assert.are_same(2, #models) + assert.are_same("gemma4:latest", models[1]) + assert.are_same("llama3", models[2]) + end) + + it("parses models using the legacy model field", function() + local f = makeFakes() + f.json.decode_return = { + models = { { model = "oldmodel" } } + } + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + f.http.get_response = "{}" + local models = c.fetchModels({ ollamaServerHost = nil }) + assert.are_same(1, #models) + assert.are_same("oldmodel", models[1]) + end) + + it("ignores entries with missing or empty names", function() + local f = makeFakes() + f.json.decode_return = { + models = { + { name = "good" }, + { name = "" }, + { name = nil }, + {}, -- no name field + } + } + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + f.http.get_response = "{}" + local models = c.fetchModels({ ollamaServerHost = nil }) + assert.are_same(1, #models) + assert.are_same("good", models[1]) + end) + + it("preserves response order", function() + local f = makeFakes() + f.json.decode_return = { + models = { + { name = "first" }, + { name = "second" }, + { name = "third" }, + } + } + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + f.http.get_response = "{}" + local models = c.fetchModels({ ollamaServerHost = nil }) + assert.are_same({ "first", "second", "third" }, models) + end) + end) + + -------------------------------------------------------------------- + -- fetchModels() — fallback paths + -------------------------------------------------------------------- + describe("fetchModels() — fallback to default", function() + it("returns default model on no HTTP response", function() + local f = makeFakes() + f.http.get_response = nil + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + local models = c.fetchModels({ ollamaServerHost = nil }) + assert.are_same({ "gemma4:latest" }, models) + end) + + it("returns default model on malformed JSON", function() + local f = makeFakes() + f.json.decode_return = "not a table" -- string, not table + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + f.http.get_response = "garbage" + local models = c.fetchModels({ ollamaServerHost = nil }) + assert.are_same({ "gemma4:latest" }, models) + end) + + it("returns default model when models key is missing", function() + local f = makeFakes() + f.json.decode_return = { other = "data" } + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + f.http.get_response = '{"other":"data"}' + local models = c.fetchModels({ ollamaServerHost = nil }) + assert.are_same({ "gemma4:latest" }, models) + end) + + it("returns default model for empty model list", function() + local f = makeFakes() + f.json.decode_return = { models = {} } + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + f.http.get_response = '{"models":[]}' + local models = c.fetchModels({ ollamaServerHost = nil }) + assert.are_same({ "gemma4:latest" }, models) + end) + + it("handles JSON decoder exception without crashing", function() + local f = makeFakes() + f.json.decode_error = "attempt to index nil" + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + f.http.get_response = "bad json" + -- Should not throw; should return default + local models = c.fetchModels({ ollamaServerHost = nil }) + assert.are_same({ "gemma4:latest" }, models) + end) + end) + + -------------------------------------------------------------------- + -- saveServerAndRefresh() — invalid input + -------------------------------------------------------------------- + describe("saveServerAndRefresh() — validation", function() + it("returns failure for invalid server input", function() + local f = makeFakes() + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + local ok, msg = c.saveServerAndRefresh({ serverHost = "noport" }, {}) + assert.is_false(ok) + assert.is_not_nil(msg) + end) + + it("does not modify prefs for invalid input", function() + local f = makeFakes() + f.prefs.ollamaServerHost = "original:1234" + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + local prefs = { ollamaServerHost = "original:1234" } + c.saveServerAndRefresh({ serverHost = "bad" }, prefs) + -- prefs should be unchanged + assert.are_same("original:1234", prefs.ollamaServerHost) + end) + end) + + -------------------------------------------------------------------- + -- saveServerAndRefresh() — valid input + -------------------------------------------------------------------- + describe("saveServerAndRefresh() — success path", function() + it("normalizes and saves valid host", function() + local f = makeFakes() + f.json.decode_return = { models = { { name = "m1" } } } + f.http.get_response = '{"models":[{"name":"m1"}]}' + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + local prefs = {} + local props = { serverHost = "http://myhost:9000/" } + + local ok, msg = c.saveServerAndRefresh(props, prefs) + + assert.is_true(ok) + assert.are_same("myhost:9000", prefs.ollamaServerHost) + end) + + it("builds model items from fetched models", function() + local f = makeFakes() + f.json.decode_return = { + models = { { name = "a" }, { name = "b" } } + } + f.http.get_response = '{"models":[{"name":"a"},{"name":"b"}]}' + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + local prefs = {} + local props = { serverHost = "localhost:11434" } + + c.saveServerAndRefresh(props, prefs) + + assert.are_same(2, #props.modelItems) + assert.are_same("a", props.modelItems[1].title) + assert.are_same("a", props.modelItems[1].value) + assert.are_same("b", props.modelItems[2].title) + end) + + it("preserves selected model when still available", function() + local f = makeFakes() + f.json.decode_return = { + models = { { name = "a" }, { name = "b" } } + } + f.http.get_response = '{}' + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + local prefs = {} + local props = { serverHost = "localhost:11434", selectedModel = "b" } + + c.saveServerAndRefresh(props, prefs) + + assert.are_same("b", props.selectedModel) + end) + + it("selects first model when old selection unavailable", function() + local f = makeFakes() + f.json.decode_return = { + models = { { name = "new1" }, { name = "new2" } } + } + f.http.get_response = '{}' + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + local prefs = {} + local props = { serverHost = "localhost:11434", selectedModel = "old" } + + c.saveServerAndRefresh(props, prefs) + + assert.are_same("new1", props.selectedModel) + end) + + it("reports correct count in success message", function() + local f = makeFakes() + f.json.decode_return = { + models = { { name = "x" }, { name = "y" }, { name = "z" } } + } + f.http.get_response = '{}' + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + local prefs = {} + local props = { serverHost = "localhost:11434" } + + local ok, msg = c.saveServerAndRefresh(props, prefs) + + assert.is_true(ok) + assert.are_same("Loaded 3 model(s)", msg) + end) + + it("model items contain title and value pairs", function() + local f = makeFakes() + f.json.decode_return = { models = { { name = "only" } } } + f.http.get_response = '{}' + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + local prefs = {} + local props = { serverHost = "localhost:11434" } + + c.saveServerAndRefresh(props, prefs) + + assert.are_same("only", props.modelItems[1].title) + assert.are_same("only", props.modelItems[1].value) + end) + + it("passes updated prefs into model discovery", function() + local f = makeFakes() + f.http.get_response = '{}' + -- The get_calls URL reveals which host was used + local Client = assert(loadfile(path .. "OllamaClient.lua"))() + local c = Client.new(f) + local prefs = {} + local props = { serverHost = "custom:7777" } + + c.saveServerAndRefresh(props, prefs) + + -- fetchModels should be called with updated prefs; the URL shows it + assert.are_same(1, #f.http.get_calls) + assert.are_same("http://custom:7777/api/tags", f.http.get_calls[1]) + end) + end) +end) From d8ac98500602c854ebf6c3c9258e0cc3d43df00d Mon Sep 17 00:00:00 2001 From: Zack Stout Date: Fri, 17 Jul 2026 11:16:48 -0500 Subject: [PATCH 04/10] feat: split LrLlama.lua into focused, testable modules Extract SinglePhotoController.lua with pure workflow logic (state transitions for generate, save-server, save-metadata) that accepts all dependencies via injection. Create LlamaDialog.lua owning all LrView/LrBinding UI construction. Rewrite LrLlama.lua as a thin entry point that builds the client adapter and delegates to LlamaDialog. - Add 173 Busted unit tests across 9 spec files (0 failures, 0 errors) - Client adapter pattern maps Common delegation layer to stable OllamaClient interface expected by SinglePhotoController - Fix loadfile invocation bugs in LrLlama.lua and LlamaDialog.lua Co-Authored-By: Claude --- lightroom-llama.lrplugin/LlamaDialog.lua | 186 ++++++ lightroom-llama.lrplugin/LrLlama.lua | 362 +++--------- .../SinglePhotoController.lua | 223 ++++++++ tests/spec/common_delegation_spec.lua | 18 +- tests/spec/lrllama_entrypoint_spec.lua | 382 +++++++++++++ tests/spec/single_photo_controller_spec.lua | 532 ++++++++++++++++++ 6 files changed, 1415 insertions(+), 288 deletions(-) create mode 100644 lightroom-llama.lrplugin/LlamaDialog.lua create mode 100644 lightroom-llama.lrplugin/SinglePhotoController.lua create mode 100644 tests/spec/lrllama_entrypoint_spec.lua create mode 100644 tests/spec/single_photo_controller_spec.lua diff --git a/lightroom-llama.lrplugin/LlamaDialog.lua b/lightroom-llama.lrplugin/LlamaDialog.lua new file mode 100644 index 0000000..72857b8 --- /dev/null +++ b/lightroom-llama.lrplugin/LlamaDialog.lua @@ -0,0 +1,186 @@ +--- LlamaDialog.lua — Lightroom view adapter for single-photo metadata generation. +--- Owns all LrView / LrBinding / LrDialogs construction: property table, UI layout, +--- button actions, and modal presentation. Delegates workflow logic to the injected +--- controller so that business rules are testable without Lightroom. + +local LrDialogs = import "LrDialogs" +local LrView = import "LrView" +local LrTasks = import "LrTasks" +local LrFunctionContext = import "LrFunctionContext" +local LrBinding = import "LrBinding" +local LrColor = import "LrColor" +local LrPathUtils = import 'LrPathUtils' + +--- Map semantic status kinds to Lightroom display colors. +local STATUS_COLORS = { + success = LrColor(0.149, 0.616, 0.412), -- green + working = LrColor(0.439, 0.345, 0.745), -- purple + error = LrColor(0.8, 0.2, 0.2), -- red +} + +-------------------------------------------------------------------------------- +--- Build and show the modal dialog. +---@param deps table Dependency map: +--- { client, common, promptBuilder, prefs, catalog, photo, thumbnailPath, +--- statusMessages, writeMetadata } +--- `writeMetadata` wraps catalog:withWriteAccessDo for the save flow. +-------------------------------------------------------------------------------- +local function show(deps) + local ControllerModule = + assert(loadfile(LrPathUtils.child(_PLUGIN.path, "SinglePhotoController.lua")))() + local controller = ControllerModule.new(deps) + + LrFunctionContext.callWithContext("showLlamaDialog", function(context) + local props = LrBinding.makePropertyTable(context) + + -- Populate binding table from controller initial state. The same table + -- is passed back to controller actions so they mutate the live bindings. + local initialState = controller.initState() + for k, v in pairs(initialState) do + props[k] = v + end + + -- Create a view factory + local f = LrView.osFactory() + + -- Define the dialog contents (preserving exact current layout and sizing) + local c = f:view{ + bind_to_object = props, + f:row{f:column{ + f:picture{ + value = deps.thumbnailPath, + frame_width = 2, + width = 400, + height = 400 + }, + width = 400 + }, f:spacer{ width = 10 }, f:column{ + + -- Title field + f:column{f:static_text{ title = "Title:" }, + f:spacer{ f:label_spacing{} }, + f:edit_field{ value = LrView.bind("title"), width = 400 }}, + f:spacer{ height = 10 }, + + -- Caption field + f:static_text{ title = "Caption:", alignment = 'left' }, + f:spacer{ f:label_spacing{} }, + f:edit_field{ value = LrView.bind("caption"), width = 400, height = 100 }, + f:spacer{ height = 10 }, + + -- Keywords field + f:static_text{ title = "Keywords:", alignment = 'left' }, + f:spacer{ f:label_spacing{} }, + f:edit_field{ value = LrView.bind("keywords"), width = 400, height = 60 }, + f:spacer{ height = 10 }, + + f:separator{ width = 400 }, + f:spacer{ height = 10 }, + + -- Prompt field + f:static_text{ title = "Prompt:", alignment = 'left' }, + f:spacer{ f:label_spacing{} }, + f:edit_field{ value = LrView.bind("prompt"), width = 400, height = 60 }, + f:spacer{ height = 10 }, + + -- Checkboxes + f:checkbox{ title = "Use current title and caption", + value = LrView.bind("useCurrentData") }, + f:spacer{ height = 10 }, + f:checkbox{ title = "Use system prompt", + value = LrView.bind("useSystemPrompt") }, + f:spacer{ height = 10 }, + + f:separator{ width = 400 }, + f:spacer{ height = 10 }, + f:separator{ width = 400 }, + f:spacer{ height = 10 }, + + -- Server host + f:static_text{ title = "Ollama Server:", alignment = 'left' }, + f:spacer{ f:label_spacing{} }, + f:edit_field{ value = LrView.bind("serverHost"), width = 250 }, + f:static_text{ title = "(default: localhost:11434)", + alignment = 'left', text_color = LrColor(0.6, 0.6, 0.6) }, + f:spacer{ height = 10 }, + + -- Model dropdown + f:static_text{ title = "Model:", alignment = 'left' }, + f:spacer{ f:label_spacing{} }, + f:popup_menu{ + value = LrView.bind("selectedModel"), + items = LrView.bind("modelItems"), + width = 250, + }, + f:spacer{ height = 10 }, + + -- Status text with dynamic color + f:row{f:static_text{ fill_horizontal = 1 }, + f:static_text{ + alignment = 'right', + title = LrView.bind("status"), + width = 200, + font = "", + text_color = LrView.bind("statusColor") + }}, + f:spacer{ height = 10 }, + + -- Action buttons + f:row{ + f:push_button{ + title = "Generate", + action = function() + LrTasks.startAsyncTask(function() + controller.generate(props) + props.statusColor = STATUS_COLORS[props.statusKind] + or STATUS_COLORS.success + end) + end + }, + f:spacer{ width = 10 }, + f:push_button{ + title = "Save Server", + action = function() + LrTasks.startAsyncTask(function() + controller.saveServer(props) + props.statusColor = STATUS_COLORS[props.statusKind] + or STATUS_COLORS.success + end) + end + }}, + f:spacer{ height = 20 }, + + width = 400 + }} + } + + -- Show the dialog + local result = LrDialogs.presentModalDialog({ + title = "Lightroom Llama", + contents = c, + actionVerb = "Save" + }) + + if result == "ok" then + local saveResult = controller.saveMetadata(props) + + if not saveResult.ok then + -- Validation failure — show error; controller did NOT write prefs/data + LrDialogs.message( + "Invalid Server Address", + saveResult.message + .. "\n\nPlease enter it as host:port (e.g., localhost:11434 or 192.168.1.10:11434).", + "warning" + ) + else + LrDialogs.message( + "Metadata Saved", + "Title, caption, and keywords have been saved to the photo.", + "info" + ) + end + end + end) +end + +return { show = show } diff --git a/lightroom-llama.lrplugin/LrLlama.lua b/lightroom-llama.lrplugin/LrLlama.lua index 508bb77..70b21c2 100644 --- a/lightroom-llama.lrplugin/LrLlama.lua +++ b/lightroom-llama.lrplugin/LrLlama.lua @@ -1,304 +1,100 @@ ---- LrLlama.lua — Single-photo metadata generation. ---- Invoked from Library or Export menus. Exports a thumbnail, sends it to Ollama, ---- and presents the generated title, caption, and keywords in an editable dialog. +--- LrLlama.lua — Single-photo metadata generation entry point. +--- Invoked from Library or Export menus. Gets the active catalog and selected photo, +--- builds dependencies for the controller and dialog, and starts the plugin task. local LrApplication = import "LrApplication" -local LrDialogs = import "LrDialogs" -local LrView = import "LrView" -local LrTasks = import "LrTasks" -local LrFunctionContext = import "LrFunctionContext" -local LrBinding = import "LrBinding" -local LrColor = import "LrColor" -local LrPrefs = import "LrPrefs" -local LrPathUtils = import 'LrPathUtils' +local LrDialogs = import "LrDialogs" +local LrTasks = import "LrTasks" +local LrPrefs = import "LrPrefs" +local LrPathUtils = import 'LrPathUtils' --- Load shared utilities (delegation layer to focused modules) +-- Common provides utility functions (parseKeywordCsv, addKeywordsWithParent, etc.) +-- and a delegation layer to OllamaClient for generate/fetchModels operations. local Common = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "Common.lua"))))() +-- PromptBuilder for the single-photo system prompt override. local PromptBuilder = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "PromptBuilder.lua"))))() --- Note: saveServerAndRefresh moved to Common.lua — used by both dialogs +--- Build a client adapter that exposes the interface SinglePhotoController expects. +--- Common delegates to OllamaClient internally; this adapter maps method names and +--- adapts signatures so the controller has a stable, testable contract. +local function makeClientAdapter(common) + return { + --- Delegate to Common.sendDataToApi with matching 7-param signature. + generate = function(photo, userInstruction, currentData, useCurrentData, + useSystemPrompt, selectedModel, systemPromptOverride) + return common.sendDataToApi(photo, userInstruction, currentData, + useCurrentData, useSystemPrompt, + selectedModel, systemPromptOverride) + end, + + --- Delegate to Common.fetchAvailableModels (no-arg variant). + fetchModels = function() + return common.fetchAvailableModels() + end, + + -- Direct delegation — signatures match what the controller calls. + validateServerHost = common.validateServerHost, + saveServerAndRefresh = common.saveServerAndRefresh, + makeModelItems = common.makeModelItems, + + -- Constants from OllamaClient via Common. + defaultModel = common.model, + defaultServerHost = common.defaultServerHost, + } +end ---- Entry point. Invoked via `LrTasks.startAsyncTask(main)` — required by the ---- Lightroom SDK for menu-activated plugins. Shows a modal dialog that lets the ---- user generate and edit title, caption, and keywords for one photo. +--- Entry point. Obtains catalog and selected photo, exports a thumbnail, +--- constructs dependencies, and delegates to LlamaDialog for the UI flow. local function main() local catalog = LrApplication.activeCatalog() local selectedPhotos = catalog:getTargetPhotos() + if #selectedPhotos == 0 then - LrDialogs.message("No photo selected", "Please select a photo to view.", "critical") + LrDialogs.message( + "No photo selected", + "Please select a photo to view.", + "critical" + ) return end - -- Single-photo mode only processes the first selection; batch mode handles multiples. - local selectedPhoto = selectedPhotos[1] - local thumbnailPath = Common.exportThumbnail(selectedPhoto) - - -- LrBinding requires a live function context for property tables. Without it, - -- bound UI elements (edit fields, checkboxes, status text) won't update live. - LrFunctionContext.callWithContext("showLlamaDialog", function(context) - local props = LrBinding.makePropertyTable(context) - local prefs = LrPrefs.prefsForPlugin() - props.status = "Ready" - props.statusColor = LrColor(0.149, 0.616, 0.412) - props.prompt = "Caption this photo" - props.title = selectedPhoto:getFormattedMetadata('title') - props.caption = selectedPhoto:getFormattedMetadata('caption') - -- Initialize keywords with existing llm keywords - local existingLlmKeywords = Common.getLlmKeywordsFromPhoto(selectedPhoto) - props.keywords = table.concat(existingLlmKeywords, ", ") - props.response = "" - -- Pre-check so the "Use current title and caption" checkbox is only enabled - -- when there's actually existing metadata to include. - props.useCurrentData = props.title ~= "" or props.caption ~= "" - props.serverHost = prefs.ollamaServerHost or Common.defaultServerHost - props.useSystemPrompt = true - - -- Fetch available models from Ollama and populate the dropdown - local availableModels = Common.fetchAvailableModels() - props.modelItems = Common.makeModelItems(availableModels) - props.selectedModel = availableModels[1] -- default to first available model - - -- Create a view factory - local f = LrView.osFactory() - - -- Define the dialog contents - local c = f:view{ - bind_to_object = props, - f:row{f:column{ - f:picture{ - value = thumbnailPath, - frame_width = 2, - width = 400, - height = 400 - }, - width = 400 - }, f:spacer{ - width = 10 - }, f:column{ - f:column{f:static_text{ - title = "Title:" - }, f:spacer{f:label_spacing{}}, f:edit_field{ - value = LrView.bind("title"), -- Bind to the new response property - width = 400 - }}, - f:spacer{ - height = 10 - }, - f:static_text{ - title = "Caption:", - alignment = 'left' - }, - f:spacer{f:label_spacing{}}, - f:edit_field{ - value = LrView.bind("caption"), -- Bind to the new response property - width = 400, - height = 100 - }, - f:spacer{ - height = 10 - }, - f:static_text{ - title = "Keywords:", - alignment = 'left' - }, - f:spacer{f:label_spacing{}}, - f:edit_field{ - value = LrView.bind("keywords"), - width = 400, - height = 60 - }, - f:spacer{ - height = 10 - }, - f:separator{ - width = 400 - }, - f:spacer{ - height = 10 - }, - f:static_text{ - title = "Prompt:", - alignment = 'left' - }, - f:spacer{f:label_spacing{}}, - f:edit_field{ - value = LrView.bind("prompt"), - width = 400, - height = 60 - }, - f:spacer{ - height = 10 - }, - f:checkbox{ - title = "Use current title and caption", - value = LrView.bind("useCurrentData") - }, - f:spacer{ - height = 10 - }, - f:checkbox{ - title = "Use system prompt", - value = LrView.bind("useSystemPrompt") - }, - f:spacer{ - height = 10 - }, - f:separator{ - width = 400 - }, - f:spacer{ - height = 10 - }, - f:separator{ - width = 400 - }, - f:spacer{ - height = 10 - }, - f:static_text{ - title = "Ollama Server:", - alignment = 'left' - }, - f:spacer{f:label_spacing{}}, - f:edit_field{ - value = LrView.bind("serverHost"), - width = 250, - }, - f:static_text{ - title = "(default: localhost:11434)", - alignment = 'left', - text_color = LrColor(0.6, 0.6, 0.6) - }, - f:spacer{ - height = 10 - }, - f:static_text{ - title = "Model:", - alignment = 'left' - }, - f:spacer{f:label_spacing{}}, - f:popup_menu{ - value = LrView.bind("selectedModel"), - items = LrView.bind("modelItems"), - width = 250, - }, - f:spacer{ - height = 10 - }, - f:row{f:static_text{ - fill_horizontal = 1 - }, f:static_text{ - alignment = 'right', - title = LrView.bind("status"), - width = 200, - font = "", - text_color = LrView.bind("statusColor") - }}, - f:spacer{ - height = 10 - }, - f:row{f:push_button{ - title = "Generate", - action = function() - props.status = "The llama is thinking..." - props.statusColor = LrColor(0.439, 0.345, 0.745) + -- Single-photo mode only processes the first selection. + local photo = selectedPhotos[1] - -- API call must run off the UI thread. LrBinding properties can - -- be mutated from this async callback — they'll propagate live - -- to the dialog while we're still inside callWithContext above. - LrTasks.startAsyncTask(function() - local apiResponse, apiError = Common.sendDataToApi( - selectedPhoto, - props.prompt, - { title = props.title, caption = props.caption }, - props.useCurrentData, - props.useSystemPrompt, - props.selectedModel, - PromptBuilder.singlePhotoSystemPrompt - ) - if apiError then - props.status = "Error: " .. apiError - props.statusColor = LrColor(0.8, 0.2, 0.2) - return - end - props.response = apiResponse - props.title = apiResponse.title - props.caption = apiResponse.caption - -- Convert keywords array to comma-separated string for display - if apiResponse.keywords and type(apiResponse.keywords) == "table" then - props.keywords = table.concat(apiResponse.keywords, ", ") - else - props.keywords = "" - end - props.status = "Ready" - props.statusColor = LrColor(0.149, 0.616, 0.412) - end) - end - }, f:spacer{ - width = 10 - }, f:push_button{ - title = "Save Server", - -- saveServerAndRefresh must run async so LrBinding picks up modelItems changes - action = function() - props.status = "Refreshing models..." - props.statusColor = LrColor(0.439, 0.345, 0.745) + -- Export thumbnail (may fail; dialog will show nothing but won't crash) + local thumbnailPath = Common.exportThumbnail(photo) - LrTasks.startAsyncTask(function() - local ok, msg = Common.saveServerAndRefresh(props, prefs) - - if not ok then - props.status = "Error: " .. msg - props.statusColor = LrColor(0.8, 0.2, 0.2) - else - props.status = msg - props.statusColor = LrColor(0.149, 0.616, 0.412) - end - end) - end - }}, - f:spacer{ - height = 20 - }, - width = 400 - }} - } - - -- Show the dialog - local result = LrDialogs.presentModalDialog({ - title = "Lightroom Llama", - contents = c, - actionVerb = "Save" - }) - - - if result == "ok" then - -- Validate and save server host preference - local ok, validatedHost = Common.validateServerHost(props.serverHost) - if not ok then - LrDialogs.message( - "Invalid Server Address", - validatedHost .. "\n\nPlease enter it as host:port (e.g., localhost:11434 or 192.168.1.10:11434).", - "warning" - ) - else - prefs.ollamaServerHost = validatedHost + -- Build the catalog-write adapter. Ensures title, caption, and keyword + -- mutations happen inside a single catalog transaction. + local function writeMetadata(catalog, photo, metadata) + catalog:withWriteAccessDo("Save Llama metadata", function() + photo:setRawMetadata("title", metadata.title) + photo:setRawMetadata("caption", metadata.caption) + if metadata.keywords and #metadata.keywords > 0 then + Common.addKeywordsWithParent(catalog, photo, metadata.keywords) end + end) + end - -- All catalog mutations must run inside withWriteAccessDo — Lightroom - -- wraps the callback in a transaction and silently discards changes outside it. - catalog:withWriteAccessDo("Save Llama metadata", function() - selectedPhoto:setRawMetadata("title", props.title) - selectedPhoto:setRawMetadata("caption", props.caption) - -- Parse keywords from comma-separated string and add with llm parent - if props.keywords and props.keywords ~= "" then - local keywordList = Common.parseKeywordCsv(props.keywords) - Common.addKeywordsWithParent(catalog, selectedPhoto, keywordList) - end - end) - - LrDialogs.message("Metadata Saved", "Title, caption, and keywords have been saved to the photo.", "info") - end - end) + -- Load the dialog adapter (which in turn creates the controller). + local LlamaDialog = + assert(loadfile(LrPathUtils.child(_PLUGIN.path, "LlamaDialog.lua")))() + + LlamaDialog.show({ + client = makeClientAdapter(Common), + common = Common, + promptBuilder = PromptBuilder, + prefs = LrPrefs.prefsForPlugin(), + catalog = catalog, + photo = photo, + thumbnailPath = thumbnailPath, + statusMessages = { + ready = "Ready", + generating = "The llama is thinking...", + refreshing = "Refreshing models...", + }, + writeMetadata = writeMetadata, + }) end LrTasks.startAsyncTask(main) diff --git a/lightroom-llama.lrplugin/SinglePhotoController.lua b/lightroom-llama.lrplugin/SinglePhotoController.lua new file mode 100644 index 0000000..99269f0 --- /dev/null +++ b/lightroom-llama.lrplugin/SinglePhotoController.lua @@ -0,0 +1,223 @@ +--- SinglePhotoController.lua — Testable workflow logic for single-photo metadata +--- generation. Owns state transitions for generate, save-server, and save-metadata +--- actions without any Lightroom SDK dependencies (no LrView, LrBinding, LrColor, +--- LrDialogs, or LrFunctionContext). +--- +--- Inject dependencies via the constructor; each operation returns structured +--- results ({ok, message}) and mutates the state table in place so that callers +--- using the same table as an LrBinding property table see live updates. + +local controller = {} + +-------------------------------------------------------------------------------- +--- Constructor — validate dependencies and return a controller instance. +---@param deps table Dependency map: +--- { client, common, promptBuilder, prefs, catalog, photo, statusMessages, writeMetadata } +--- - client: OllamaClient instance (generate, fetchModels, validateServerHost, +--- saveServerAndRefresh, makeModelItems, defaultModel, defaultServerHost) +--- - common: Utility functions { parseKeywordCsv, getLlmKeywordsFromPhoto } +--- - promptBuilder: PromptBuilder module with singlePhotoSystemPrompt +--- - prefs: LrPrefs.prefsForPlugin() table for persistence +--- - catalog: LrCatalog active catalog +--- - photo: LrPhoto selected photo +--- - statusMessages: { ready, generating, refreshing } status strings +--- - writeMetadata: function(catalog, photo) callback for catalog writes +--- (wraps withWriteAccessDo in production; faked in tests) +---@return table controller Instance with initState, generate, saveServer, saveMetadata +function controller.new(deps) + assert(deps.client, "deps.client is required") + assert(deps.common, "deps.common is required") + assert(deps.prefs, "deps.prefs is required") + assert(deps.catalog, "deps.catalog is required") + assert(deps.photo, "deps.photo is required") + + local promptBuilder = deps.promptBuilder + local statusMessages = deps.statusMessages or {} + + return { + initState = function() return controller.initState(deps, promptBuilder, statusMessages) end, + generate = function(state) return controller.generate(deps, state) end, + saveServer = function(state) return controller.saveServer(deps, state) end, + saveMetadata = function(state) return controller.saveMetadata(deps, state) end, + } +end + +-------------------------------------------------------------------------------- +-- Initial state creation +-------------------------------------------------------------------------------- + +--- Build the initial state table by reading photo metadata, loading preferences, +--- and fetching available models. +local function initState(deps, promptBuilder, statusMessages) + local client = deps.client + local common = deps.common + local prefs = deps.prefs + local photo = deps.photo + + local state = {} + + -- Read existing photo metadata + state.title = photo:getFormattedMetadata("title") or "" + state.caption = photo:getFormattedMetadata("caption") or "" + + -- Read existing LLM keywords and join with commas + local existingKw = common.getLlmKeywordsFromPhoto(photo) + state.keywords = table.concat(existingKw, ", ") + + -- Enable "use current data" when there's metadata to work from + state.useCurrentData = (state.title ~= "" or state.caption ~= "") + + -- Server host: saved preference or default + state.serverHost = prefs.ollamaServerHost or client.defaultServerHost + state.useSystemPrompt = true + + -- Fetch models and populate dropdown items. The adapter may or may not accept + -- a prefs argument; call it with no args (prefs are internal to the adapter). + local models = client.fetchModels() + if not models or #models == 0 then + models = { client.defaultModel } + end + state.modelItems = client.makeModelItems(models) + state.selectedModel = models[1] + + -- Prompt and status defaults + state.prompt = "Caption this photo" + state.response = nil + state.status = statusMessages.ready or "Ready" + state.statusKind = "success" + + return state +end + +controller.initState = initState + +-------------------------------------------------------------------------------- +-- Generate action +-------------------------------------------------------------------------------- + +--- Call the API to generate metadata for the current photo. +--- Sets status to working before the call, success/error after. +--- On failure, preserves existing editable fields (title, caption, keywords). +---@param state table Mutable state table (also used as LrBinding property table) +---@return table result { ok: bool, message?: string } +local function generate(deps, state) + local client = deps.client + local statusMessages = deps.statusMessages or {} + + -- Transition to working status + state.status = statusMessages.generating or "The llama is thinking..." + state.statusKind = "working" + + -- Snapshot current editable fields so we can restore on failure + local savedTitle = state.title + local savedCaption = state.caption + local savedKeywords = state.keywords + + local apiResponse, apiError = deps.client.generate( + deps.photo, + state.prompt, + { title = state.title, caption = state.caption }, + state.useCurrentData, + state.useSystemPrompt, + state.selectedModel, + deps.promptBuilder and deps.promptBuilder.singlePhotoSystemPrompt or nil + ) + + if apiError then + -- Restore editable fields on failure + state.title = savedTitle + state.caption = savedCaption + state.keywords = savedKeywords + state.status = "Error: " .. apiError + state.statusKind = "error" + return { ok = false, message = "Error: " .. apiError } + end + + -- Update from API response on success + state.response = apiResponse + state.title = apiResponse.title + state.caption = apiResponse.caption + if apiResponse.keywords and type(apiResponse.keywords) == "table" then + state.keywords = table.concat(apiResponse.keywords, ", ") + else + state.keywords = "" + end + state.status = statusMessages.ready or "Ready" + state.statusKind = "success" + return { ok = true } +end + +controller.generate = generate + +-------------------------------------------------------------------------------- +-- Save Server action +-------------------------------------------------------------------------------- + +--- Validate and persist the server host, then refresh the model list. +--- Callers are responsible for running this from an async task (LrBinding +--- propagation requires off-UI-thread property mutations). +---@param state table Mutable state table +---@return table result { ok: bool, message?: string } +local function saveServer(deps, state) + local statusMessages = deps.statusMessages or {} + + state.status = statusMessages.refreshing or "Refreshing models..." + state.statusKind = "working" + + local ok, msg = deps.client.saveServerAndRefresh(state, deps.prefs) + + if not ok then + state.status = "Error: " .. (msg or "Failed to save server") + state.statusKind = "error" + return { ok = false, message = state.status } + end + + state.status = msg + state.statusKind = "success" + return { ok = true, message = msg } +end + +controller.saveServer = saveServer + +-------------------------------------------------------------------------------- +-- Save Metadata action +-------------------------------------------------------------------------------- + +--- Validate server host, persist preferences, and write metadata through the +--- injected write function. Returns a structured result so callers (and tests) +--- can distinguish validation failure from successful saves. +---@param state table Mutable state table +---@return table result { ok: bool, message?: string, errorKind?: string } +local function saveMetadata(deps, state) + -- Validate and normalize server host + local ok, validatedHost = deps.client.validateServerHost(state.serverHost) + if not ok then + return { + ok = false, + errorKind = "invalid_server", + message = validatedHost, + } + end + + -- Save normalized host to preferences (only when valid) + deps.prefs.ollamaServerHost = validatedHost + + -- Parse keywords using the tested CSV parser + local keywordList = {} + if state.keywords and state.keywords ~= "" then + keywordList = deps.common.parseKeywordCsv(state.keywords) + end + + -- Execute catalog writes through injected adapter + deps.writeMetadata(deps.catalog, deps.photo, { + title = state.title, + caption = state.caption, + keywords = keywordList, + }) + + return { ok = true } +end + +controller.saveMetadata = saveMetadata + +return controller diff --git a/tests/spec/common_delegation_spec.lua b/tests/spec/common_delegation_spec.lua index bc68648..2943761 100644 --- a/tests/spec/common_delegation_spec.lua +++ b/tests/spec/common_delegation_spec.lua @@ -93,15 +93,23 @@ describe("Common.lua delegation", function() end) -------------------------------------------------------------------- - -- Production wiring: LrLlama.lua must use the tested parser + -- Production wiring: SinglePhotoController uses the tested parser -------------------------------------------------------------------- describe("production wiring", function() - it("uses Common.parseKeywordCsv and has no inline keyword parsing loop", function() - local source = assert(io.open(path .. "LrLlama.lua", "r")):read("*a") + it("SinglePhotoController uses common.parseKeywordCsv and has no inline parsing", function() + local source = assert(io.open(path .. "SinglePhotoController.lua", "r")):read("*a") assert.truthy( - string.find(source, "Common%.parseKeywordCsv"), - "LrLlama.lua should call Common.parseKeywordCsv" + string.find(source, "common%.parseKeywordCsv"), + "SinglePhotoController.lua should call common.parseKeywordCsv" + ) + assert.falsy( + string.find(source, "gmatch.*keywords.*%[%,", nil, true), + "SinglePhotoController.lua should not contain inline gmatch keyword parsing" ) + end) + + it("LrLlama.lua has no inline keyword parsing loop", function() + local source = assert(io.open(path .. "LrLlama.lua", "r")):read("*a") assert.falsy( string.find(source, "gmatch.*keywords.*%[%,", nil, true), "LrLlama.lua should not contain inline gmatch keyword parsing" diff --git a/tests/spec/lrllama_entrypoint_spec.lua b/tests/spec/lrllama_entrypoint_spec.lua new file mode 100644 index 0000000..906eb35 --- /dev/null +++ b/tests/spec/lrllama_entrypoint_spec.lua @@ -0,0 +1,382 @@ +--- lrllama_entrypoint_spec.lua — Smoke tests for LrLlama.lua entry point. +--- Because the file imports Lightroom SDK modules at top level (LrApplication, +--- LrDialogs, etc.), it cannot be loaded with loadfile() outside Lightroom. +--- These tests verify structure via source inspection and validate the client +--- adapter interface with a fake Common module. + +local path = PLUGIN_PATH + +describe("LrLlama.lua entrypoint", function() + local source + + before_each(function() + source = assert(io.open(path .. "LrLlama.lua", "r")):read("*a") + end) + + --------------------------------------------------------------- + -- A. MODULE LOADING + --------------------------------------------------------------- + describe("loads dependency modules", function() + it("loads Common.lua via loadfile", function() + assert.truthy( + string.find(source, 'loadfile.*Common%.lua'), + "Entrypoint must load Common.lua" + ) + end) + + it("loads PromptBuilder.lua via loadfile", function() + assert.truthy( + string.find(source, 'loadfile.*PromptBuilder%.lua'), + "Entrypoint must load PromptBuilder.lua" + ) + end) + + it("loads LlamaDialog.lua via loadfile", function() + assert.truthy( + string.find(source, 'loadfile.*LlamaDialog%.lua'), + "Entrypoint must load LlamaDialog.lua" + ) + end) + end) + + --------------------------------------------------------------- + -- B. ASYNC EXECUTION + --------------------------------------------------------------- + describe("async execution", function() + it("runs main in LrTasks.startAsyncTask", function() + assert.truthy( + string.find(source, "LrTasks%.startAsyncTask"), + "Entrypoint must start async task" + ) + end) + end) + + --------------------------------------------------------------- + -- C. NO-PHOTO SAFEGUARD + --------------------------------------------------------------- + describe("no-photo safeguard", function() + it("gets target photos from catalog", function() + assert.truthy( + string.find(source, "getTargetPhotos"), + "Entrypoint must query selected photos" + ) + end) + + it("checks for empty selection", function() + assert.truthy( + string.find(source, "#selectedPhotos == 0"), + "Entrypoint must guard against no selection" + ) + end) + + it("shows message when no photo selected", function() + assert.truthy( + string.find(source, "LrDialogs%.message"), + "Entrypoint must show error message for empty selection" + ) + end) + end) + + --------------------------------------------------------------- + -- D. CLIENT ADAPTER INTERFACE + --------------------------------------------------------------- + describe("client adapter", function() + it("defines makeClientAdapter function", function() + assert.truthy( + string.find(source, "function makeClientAdapter"), + "Entrypoint must define makeClientAdapter" + ) + end) + + it("adapter delegates generate to Common.sendDataToApi", function() + assert.truthy( + string.find(source, "common%.sendDataToApi"), + "Adapter must delegate generate to Common.sendDataToApi" + ) + end) + + it("adapter delegates fetchModels to Common.fetchAvailableModels", function() + assert.truthy( + string.find(source, "common%.fetchAvailableModels"), + "Adapter must delegate fetchModels" + ) + end) + + it("adapter exposes validateServerHost", function() + assert.truthy( + string.find(source, "validateServerHost.*common%.validateServerHost"), + "Adapter must expose validateServerHost from Common" + ) + end) + + it("adapter exposes saveServerAndRefresh", function() + assert.truthy( + string.find(source, "saveServerAndRefresh.*common%.saveServerAndRefresh"), + "Adapter must expose saveServerAndRefresh from Common" + ) + end) + + it("adapter exposes makeModelItems", function() + assert.truthy( + string.find(source, "makeModelItems.*common%.makeModelItems"), + "Adapter must expose makeModelItems from Common" + ) + end) + + it("adapter exposes defaultModel constant", function() + assert.truthy( + string.find(source, "defaultModel.*=.*common%.model"), + "Adapter must expose defaultModel from Common.model" + ) + end) + + it("adapter exposes defaultServerHost constant", function() + assert.truthy( + string.find(source, "defaultServerHost.*=.*common%.defaultServerHost"), + "Adapter must expose defaultServerHost" + ) + end) + end) + + --------------------------------------------------------------- + -- E. DELEGATION TO LLAMADIALOG + --------------------------------------------------------------- + describe("delegation to LlamaDialog", function() + it("calls LlamaDialog.show with client adapter", function() + assert.truthy( + string.find(source, "LlamaDialog%.show"), + "Entrypoint must call LlamaDialog.show" + ) + end) + + it("passes client to dialog", function() + assert.truthy( + string.find(source, "client.*=.*makeClientAdapter"), + "Entrypoint must pass client adapter to dialog" + ) + end) + + it("passes common to dialog", function() + assert.truthy( + string.find(source, "= Common"), + "Entrypoint must pass common utilities to dialog" + ) + end) + + it("passes promptBuilder to dialog", function() + assert.truthy( + string.find(source, "= PromptBuilder"), + "Entrypoint must pass promptBuilder to dialog" + ) + end) + + it("passes prefs to dialog", function() + assert.truthy( + string.find(source, "LrPrefs.prefsForPlugin"), + "Entrypoint must pass prefs to dialog" + ) + end) + + it("passes catalog and photo to dialog", function() + assert.truthy(string.find(source, "catalog[[:%s]]*=")) + assert.truthy(string.find(source, "photo[[:%s]]*=")) + end) + + it("passes thumbnailPath to dialog", function() + assert.truthy( + string.find(source, "thumbnailPath"), + "Entrypoint must pass thumbnail path to dialog" + ) + end) + + it("passes statusMessages to dialog", function() + assert.truthy( + string.find(source, "statusMessages"), + "Entrypoint must pass status messages to dialog" + ) + end) + + it("passes writeMetadata callback to dialog", function() + assert.truthy( + string.find(source, "writeMetadata"), + "Entrypoint must pass writeMetadata callback" + ) + end) + end) + + --------------------------------------------------------------- + -- F. WRITEMETADATA CLOSURE + --------------------------------------------------------------- + describe("writeMetadata closure", function() + it("uses catalog withWriteAccessDo for transactional writes", function() + assert.truthy( + string.find(source, "withWriteAccessDo"), + "writeMetadata must use catalog transaction" + ) + end) + + it("sets title via setRawMetadata", function() + assert.truthy( + string.find(source, 'setRawMetadata.*title'), + "writeMetadata must set title" + ) + end) + + it("sets caption via setRawMetadata", function() + assert.truthy( + string.find(source, 'setRawMetadata.*caption'), + "writeMetadata must set caption" + ) + end) + + it("adds keywords via Common.addKeywordsWithParent", function() + assert.truthy( + string.find(source, "Common%.addKeywordsWithParent"), + "writeMetadata must use addKeywordsWithParent for keywords" + ) + end) + end) + + --------------------------------------------------------------- + -- G. NO INLINE BUSINESS LOGIC + --------------------------------------------------------------- + describe("no inline business logic", function() + it("has no inline keyword parsing loop", function() + assert.falsy( + string.find(source, "gmatch.*keywords.*%[%,", nil, true), + "Entrypoint must not contain inline gmatch keyword parsing" + ) + end) + + it("has no HTTP calls — delegates to Common/adapter", function() + assert.falsy( + string.find(source, "LrHttp"), + "Entrypoint must not make HTTP calls directly" + ) + end) + + it("has no LrView usage — delegates to LlamaDialog", function() + assert.falsy( + string.find(source, "LrView"), + "Entrypoint must not construct UI directly" + ) + end) + + it("has no binding construction — delegates to LlamaDialog", function() + assert.falsy( + string.find(source, "LrBinding"), + "Entrypoint must not create bindings directly" + ) + end) + end) +end) + +describe("makeClientAdapter interface shape", function() + -- Verify the adapter interface matches what SinglePhotoController expects. + -- Build a minimal fake Common and inline the adapter logic to ensure the + -- contract is stable. + + local function makeFakeCommon() + return { + sendDataToApi = function() return {}, nil end, + fetchAvailableModels = function() return {} end, + validateServerHost = function(host) return true, host end, + saveServerAndRefresh = function(state, prefs) return true, "ok" end, + makeModelItems = function(names) return names end, + model = "gemma4:latest", + defaultServerHost = "localhost:11434", + } + end + + local function makeClientAdapter(common) + return { + generate = function(photo, userInstruction, currentData, useCurrentData, + useSystemPrompt, selectedModel, systemPromptOverride) + return common.sendDataToApi(photo, userInstruction, currentData, + useCurrentData, useSystemPrompt, + selectedModel, systemPromptOverride) + end, + fetchModels = function() + return common.fetchAvailableModels() + end, + validateServerHost = common.validateServerHost, + saveServerAndRefresh = common.saveServerAndRefresh, + makeModelItems = common.makeModelItems, + defaultModel = common.model, + defaultServerHost = common.defaultServerHost, + } + end + + local adapter + local fakeCommon + + before_each(function() + fakeCommon = makeFakeCommon() + adapter = makeClientAdapter(fakeCommon) + end) + + it("has all required methods", function() + assert.is_function(adapter.generate) + assert.is_function(adapter.fetchModels) + assert.is_function(adapter.validateServerHost) + assert.is_function(adapter.saveServerAndRefresh) + assert.is_function(adapter.makeModelItems) + end) + + it("has all required constants", function() + assert.are_same("gemma4:latest", adapter.defaultModel) + assert.are_same("localhost:11434", adapter.defaultServerHost) + end) + + it("generate delegates to sendDataToApi with correct arg count", function() + local callArgs = nil + fakeCommon.sendDataToApi = function(p, u, c, uc, us, m, s) + callArgs = { p, u, c, uc, us, m, s } + return {}, nil + end + + adapter.generate("photo", "prompt", { title = "T" }, true, true, "model", "sys") + assert.are_same(7, #callArgs) + assert.are_same("photo", callArgs[1]) + assert.are_same("prompt", callArgs[2]) + end) + + it("fetchModels delegates to fetchAvailableModels", function() + local called = false + fakeCommon.fetchAvailableModels = function() + called = true + return { "m1" } + end + local result = adapter.fetchModels() + assert.is_true(called) + assert.are_same({ "m1" }, result) + end) + + it("validateServerHost is the Common function", function() + local ok, host = adapter.validateServerHost("host:1234") + assert.is_true(ok) + assert.are_same("host:1234", host) + end) + + it("saveServerAndRefresh passes through correctly", function() + local ok, msg = adapter.saveServerAndRefresh({}, {}) + assert.is_true(ok) + assert.are_same("ok", msg) + end) + + it("matches SinglePhotoController expected interface keys", function() + -- Mirror the dependency validation in SinglePhotoController.new() + -- to catch interface drift. + local expectedKeys = { + "generate", "fetchModels", "validateServerHost", + "saveServerAndRefresh", "makeModelItems", + "defaultModel", "defaultServerHost" + } + for _, key in ipairs(expectedKeys) do + assert.is_not_nil( + adapter[key], + string.format("Adapter must have '%s' for SinglePhotoController", key) + ) + end + end) +end) diff --git a/tests/spec/single_photo_controller_spec.lua b/tests/spec/single_photo_controller_spec.lua new file mode 100644 index 0000000..dcd36ab --- /dev/null +++ b/tests/spec/single_photo_controller_spec.lua @@ -0,0 +1,532 @@ +--- single_photo_controller_spec.lua — Unit tests for SinglePhotoController. +--- Covers initial state, generate success/failure, save server, save metadata. + +local path = PLUGIN_PATH + +-------------------------------------------------------------------------------- +--- Helper: build fresh fake dependencies each test. +-------------------------------------------------------------------------------- +local function makeFakes() + -- Consolidated fake: serves as photo, common, and client simultaneously. + -- Tests pass the same table for multiple dep roles to keep things simple. + local f = {} + + -- photo fake with getFormattedMetadata + f.metadata = { title = "", caption = "" } + f.getFormattedMetadata = function(_, key) + return f.metadata[key] or "" + end + + -- common fake (parseKeywordCsv + getLlmKeywordsFromPhoto) + f.parseKeywordCsv = function(csv) + if not csv or csv == "" then return {} end + local result = {} + for kw in string.gmatch(csv, "([^,]+)") do + local trimmed = kw:match("^%s*(.-)%s*$") + if trimmed ~= "" then table.insert(result, trimmed) end + end + return result + end + f.getLlmKeywordsFromPhoto = function() + return f.keywords or {} + end + + -- client fake constants + f.defaultModel = "gemma4:latest" + f.defaultServerHost = "localhost:11434" + f.fetchModels_called = false + f.fetchModels_models = { "gemma4:latest", "llama3" } + f.generate_calls = {} + f.generate_response = nil + f.generate_error = nil + f.makeModelItems = function(names) + local items = {} + for _, n in ipairs(names) do + table.insert(items, { title = n, value = n }) + end + return items + end + + -- saveServerAndRefresh tracking + f.saveServerAndRefresh_calls = {} + f.saveServerAndRefresh_ok = true + f.saveServerAndRefresh_msg = "Loaded 2 model(s)" + + f.validateServerHost_called = false + f.validateServerHost_ok = true + f.validateServerHost_result = "localhost:11434" + + return f +end + +local function buildClient(client) + client.fetchModels = function() + client.fetchModels_called = true + return client.fetchModels_models + end + client.generate = function(photo, instruction, currentData, useCurrent, + useSystem, model, systemOverride) + table.insert(client.generate_calls, { + photo = photo, instruction = instruction, + currentData = currentData, useCurrent = useCurrent, + useSystem = useSystem, model = model, systemOverride = systemOverride + }) + return client.generate_response, client.generate_error + end + client.saveServerAndRefresh = function(state, prefs) + table.insert(client.saveServerAndRefresh_calls, { state, prefs }) + return client.saveServerAndRefresh_ok, + client.saveServerAndRefresh_msg + end + client.validateServerHost = function(input) + client.validateServerHost_called = true + return client.validateServerHost_ok, + client.validateServerHost_result + end + return client +end + +-------------------------------------------------------------------------------- +describe("SinglePhotoController", function() + local Controller + + before_each(function() + Controller = assert(loadfile(path .. "SinglePhotoController.lua"))() + end) + + --------------------------------------------------------------- + -- A. INITIAL STATE + --------------------------------------------------------------- + describe("initial state", function() + + it("empty title/caption → useCurrentData = false", function() + local f = makeFakes() + buildClient(f) + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, + catalog = {} + }) + local state = ctrl.initState() + assert.is_false(state.useCurrentData) + end) + + it("existing title → useCurrentData = true", function() + local f = makeFakes() + buildClient(f) + f.metadata.title = "Sunset" + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + local state = ctrl.initState() + assert.is_true(state.useCurrentData) + end) + + it("existing caption → useCurrentData = true", function() + local f = makeFakes() + buildClient(f) + f.metadata.caption = "A beach" + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + local state = ctrl.initState() + assert.is_true(state.useCurrentData) + end) + + it("joins existing LLM keywords with commas", function() + local f = makeFakes() + buildClient(f) + f.keywords = { "sunset", "beach" } + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + local state = ctrl.initState() + assert.are_same("sunset, beach", state.keywords) + end) + + it("uses saved server host from prefs", function() + local f = makeFakes() + buildClient(f) + local prefs = { ollamaServerHost = "myhost:9000" } + local ctrl = Controller.new({ + client = f, common = f, prefs = prefs, photo = f, catalog = {} + }) + local state = ctrl.initState() + assert.are_same("myhost:9000", state.serverHost) + end) + + it("uses default server host when prefs missing", function() + local f = makeFakes() + buildClient(f) + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + local state = ctrl.initState() + assert.are_same("localhost:11434", state.serverHost) + end) + + it("fetches available models", function() + local f = makeFakes() + buildClient(f) + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + ctrl.initState() + assert.is_true(f.fetchModels_called) + end) + + it("builds model items from fetched models", function() + local f = makeFakes() + buildClient(f) + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + local state = ctrl.initState() + assert.are_same(2, #state.modelItems) + assert.are_same("gemma4:latest", state.modelItems[1].title) + assert.are_same("llama3", state.modelItems[2].title) + end) + + it("selects first model by default", function() + local f = makeFakes() + buildClient(f) + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + local state = ctrl.initState() + assert.are_same("gemma4:latest", state.selectedModel) + end) + + it("sets default prompt and system-prompt settings", function() + local f = makeFakes() + buildClient(f) + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + local state = ctrl.initState() + assert.are_same("Caption this photo", state.prompt) + assert.is_true(state.useSystemPrompt) + end) + + end) + + --------------------------------------------------------------- + -- B. GENERATE SUCCESS + --------------------------------------------------------------- + describe("generate — success", function() + + it("calls API with all expected arguments", function() + local f = makeFakes() + buildClient(f) + f.generate_response = { title = "T", caption = "C", keywords = { "k" } } + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {}, + promptBuilder = { singlePhotoSystemPrompt = "sys" } + }) + local state = ctrl.initState() + ctrl.generate(state) + assert.are_same(1, #f.generate_calls) + end) + + it("sets status to working before API call", function() + local f = makeFakes() + buildClient(f) + f.generate_response = { title = "T", caption = "C", keywords = {} } + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + local state = ctrl.initState() + + -- Override generate to check status at call time + local capturedStatus = nil + f.generate = function(...) + capturedStatus = state.statusKind + return f.generate_response + end + ctrl.generate(state) + assert.are_same("working", capturedStatus) + end) + + it("updates title, caption from response", function() + local f = makeFakes() + buildClient(f) + f.generate_response = { title = "New T", caption = "New C", keywords = {} } + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + local state = ctrl.initState() + ctrl.generate(state) + assert.are_same("New T", state.title) + assert.are_same("New C", state.caption) + end) + + it("joins keyword array into comma-separated string", function() + local f = makeFakes() + buildClient(f) + f.generate_response = { title = "T", caption = "C", keywords = { "a", "b" } } + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + local state = ctrl.initState() + ctrl.generate(state) + assert.are_same("a, b", state.keywords) + end) + + it("retains response data in state", function() + local f = makeFakes() + buildClient(f) + f.generate_response = { title = "T", caption = "C", keywords = {} } + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + local state = ctrl.initState() + ctrl.generate(state) + assert.is_not_nil(state.response) + end) + + it("returns to ready/success status", function() + local f = makeFakes() + buildClient(f) + f.generate_response = { title = "T", caption = "C", keywords = {} } + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + local state = ctrl.initState() + ctrl.generate(state) + assert.are_same("success", state.statusKind) + end) + + it("returns a successful result table", function() + local f = makeFakes() + buildClient(f) + f.generate_response = { title = "T", caption = "C", keywords = {} } + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + local state = ctrl.initState() + local result = ctrl.generate(state) + assert.is_true(result.ok) + end) + + end) + + --------------------------------------------------------------- + -- C. GENERATE FAILURE + --------------------------------------------------------------- + describe("generate — failure", function() + + it("preserves existing editable fields on error", function() + local f = makeFakes() + buildClient(f) + f.generate_error = "network failure" + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + local state = ctrl.initState() + -- Mutate fields before generate to verify preservation + state.title = "Existing Title" + + ctrl.generate(state) + assert.are_same("Existing Title", state.title) + end) + + it("sets status to error", function() + local f = makeFakes() + buildClient(f) + f.generate_error = "boom" + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + local state = ctrl.initState() + ctrl.generate(state) + assert.are_same("error", state.statusKind) + assert.truthy(string.find(state.status, "Error")) + end) + + it("failure result contains the error", function() + local f = makeFakes() + buildClient(f) + f.generate_error = "no network" + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + local state = ctrl.initState() + local result = ctrl.generate(state) + assert.is_false(result.ok) + end) + + end) + + --------------------------------------------------------------- + -- D. SAVE SERVER SUCCESS + --------------------------------------------------------------- + describe("saveServer — success", function() + + it("passes state and prefs to saveServerAndRefresh", function() + local f = makeFakes() + buildClient(f) + local prefs = {} + local ctrl = Controller.new({ + client = f, common = f, prefs = prefs, photo = f, catalog = {} + }) + local state = ctrl.initState() + ctrl.saveServer(state) + assert.are_same(1, #f.saveServerAndRefresh_calls) + end) + + it("sets success status and message", function() + local f = makeFakes() + buildClient(f) + f.saveServerAndRefresh_msg = "Loaded 3 model(s)" + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + local state = ctrl.initState() + ctrl.saveServer(state) + assert.are_same("success", state.statusKind) + assert.are_same(f.saveServerAndRefresh_msg, state.status) + end) + + end) + + --------------------------------------------------------------- + -- E. SAVE SERVER FAILURE + --------------------------------------------------------------- + describe("saveServer — failure", function() + + it("sets error status", function() + local f = makeFakes() + buildClient(f) + f.saveServerAndRefresh_ok = false + f.saveServerAndRefresh_msg = "bad server" + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + local state = ctrl.initState() + ctrl.saveServer(state) + assert.are_same("error", state.statusKind) + end) + + it("returns failure result", function() + local f = makeFakes() + buildClient(f) + f.saveServerAndRefresh_ok = false + f.saveServerAndRefresh_msg = "bad server" + local ctrl = Controller.new({ + client = f, common = f, prefs = {}, photo = f, catalog = {} + }) + local state = ctrl.initState() + local result = ctrl.saveServer(state) + assert.is_false(result.ok) + end) + + end) + + --------------------------------------------------------------- + -- F. SAVE METADATA SUCCESS + --------------------------------------------------------------- + describe("saveMetadata — success", function() + + it("validates server host and saves to prefs", function() + local f = makeFakes() + buildClient(f) + f.validateServerHost_ok = true + f.validateServerHost_result = "localhost:11434" + local prefs = {} + local written = nil + local ctrl = Controller.new({ + client = f, common = f, prefs = prefs, photo = f, catalog = {}, + writeMetadata = function(catalog, photo, meta) + written = meta + end, + }) + local state = ctrl.initState() + ctrl.saveMetadata(state) + assert.are_same("localhost:11434", prefs.ollamaServerHost) + end) + + it("parses keywords through parseKeywordCsv", function() + local f = makeFakes() + buildClient(f) + f.validateServerHost_ok = true + f.validateServerHost_result = "localhost:11434" + local prefs = {} + local written = nil + local ctrl = Controller.new({ + client = f, common = f, prefs = prefs, photo = f, catalog = {}, + writeMetadata = function(catalog, photo, meta) + written = meta + end, + }) + local state = ctrl.initState() + state.keywords = "sunset,, beach, ,portrait" + ctrl.saveMetadata(state) + assert.are_same({ "sunset", "beach", "portrait" }, written.keywords) + end) + + it("passes title and caption to write adapter", function() + local f = makeFakes() + buildClient(f) + f.validateServerHost_ok = true + f.validateServerHost_result = "localhost:11434" + local prefs = {} + local written = nil + local ctrl = Controller.new({ + client = f, common = f, prefs = prefs, photo = f, catalog = {}, + writeMetadata = function(catalog, photo, meta) + written = meta + end, + }) + local state = ctrl.initState() + state.title = "T" + state.caption = "C" + ctrl.saveMetadata(state) + assert.are_same("T", written.title) + assert.are_same("C", written.caption) + end) + + it("returns success result", function() + local f = makeFakes() + buildClient(f) + f.validateServerHost_ok = true + f.validateServerHost_result = "localhost:11434" + local prefs = {} + local ctrl = Controller.new({ + client = f, common = f, prefs = prefs, photo = f, catalog = {}, + writeMetadata = function() end, + }) + local state = ctrl.initState() + local result = ctrl.saveMetadata(state) + assert.is_true(result.ok) + end) + + end) + + --------------------------------------------------------------- + -- G. SAVE METADATA VALIDATION FAILURE + --------------------------------------------------------------- + describe("saveMetadata — validation failure", function() + + it("returns structured validation error for invalid server", function() + local f = makeFakes() + buildClient(f) + f.validateServerHost_ok = false + f.validateServerHost_result = "must be host:port" + local prefs = {} + local writeCalled = false + local ctrl = Controller.new({ + client = f, common = f, prefs = prefs, photo = f, catalog = {}, + writeMetadata = function() writeCalled = true end, + }) + local state = ctrl.initState() + state.serverHost = "bad" + local result = ctrl.saveMetadata(state) + + assert.is_false(result.ok) + assert.are_same("invalid_server", result.errorKind) + assert.is_not_nil(result.message) + assert.is_false(writeCalled) + end) + + end) + +end) From 1fa439b9cb7a4ff065d2d00d281588ec4e54f4a6 Mon Sep 17 00:00:00 2001 From: Zack Stout Date: Fri, 17 Jul 2026 13:26:46 -0500 Subject: [PATCH 05/10] test: add behavioral delegation regression tests for Common.lua compatibility layer Refactor Common.lua to use explicit wrapper functions instead of direct reference assignments, enabling test spies to observe delegation behavior. Add createCommon(deps) factory exposed as common.new() for dependency injection in tests. - Every compatibility export now has a behavioral delegation test with fake focused modules (27 tests, up from 12 shape-only assertions) - sendDataToApi forwards all 7 argument positions including nil intermediates - fetchAvailableModels passes exact prefs object identity to fetchModels - Error propagation verified for both wrapper functions - Mutation checks confirm tests catch dropped args, wrong prefs, and misrouted delegation Co-Authored-By: Claude --- lightroom-llama.lrplugin/Common.lua | 131 ++++-- tests/spec/common_delegation_spec.lua | 556 ++++++++++++++++++++++---- 2 files changed, 575 insertions(+), 112 deletions(-) diff --git a/lightroom-llama.lrplugin/Common.lua b/lightroom-llama.lrplugin/Common.lua index 12b1d7a..9f61f68 100644 --- a/lightroom-llama.lrplugin/Common.lua +++ b/lightroom-llama.lrplugin/Common.lua @@ -1,48 +1,111 @@ --- Common.lua — Shared utilities for Lightroom Llama plugin (delegation layer). +--- Common.lua — Shared utilities for Lightroom Llama plugin (delegation layer). --- Loads the focused modules and re-exports them under the original names used by --- LrLlama.lua, BatchLrLlama.lua, and ResetMetadata.lua. This allows a zero-downtime --- migration: entry points continue calling Common.X() while logic lives in dedicated --- modules (OllamaClient, ThumbnailService, PromptBuilder, ResponseValidator, --- MetadataService). Once verified, the delegation wrappers can be removed and --- entry points updated to load modules directly. +--- +--- For testing, the exported `new` constructor accepts fake focused modules so that +--- delegation behavior can be verified without the Lightroom SDK. local LrPrefs = import "LrPrefs" local LrPathUtils = import 'LrPathUtils' --- Load focused modules +-------------------------------------------------------------------------------- +--- Factory — build a Common compatibility object from dependency modules. +--- Production code uses the default instance returned at module load time; +--- tests call `common.new(deps)` with fake modules to verify delegation. +--- +--- All delegations use explicit wrapper functions (not direct reference +--- assignments) so that test harnesses can swap out fake implementations +--- between construction and invocation and observe correct forwarding. +---@param deps table Dependency map: +--- { ollamaClient, thumbnailService, metadataService, prefsService } +--- - ollamaClient: OllamaClient module (defaultModel, defaultServerHost, +--- validateServerHost, makeModelItems, saveServerAndRefresh, fetchModels, generate) +--- - thumbnailService: ThumbnailService module (export, encodeBase64) +--- - metadataService: MetadataService module +--- (addKeywordsWithParent, getLlmKeywordsFromPhoto, removeLlmKeywords, parseKeywordCsv) +--- - prefsService: LrPrefs module with prefsForPlugin() +---@return table Common compatibility object with all expected exports +local function createCommon(deps) + local ollamaClient = assert(deps.ollamaClient, "deps.ollamaClient is required") + local thumbnailService = assert(deps.thumbnailService, "deps.thumbnailService is required") + local metadataService = assert(deps.metadataService, "deps.metadataService is required") + local prefsService = assert(deps.prefsService, "deps.prefsService is required") + + return { + -- OllamaClient constants + model = ollamaClient.defaultModel, + defaultServerHost = ollamaClient.defaultServerHost, + + -- OllamaClient functions — explicit wrappers for testability. + -- Direct reference assignment would capture the function at construction + -- time, preventing tests from verifying delegation behavior. + validateServerHost = function(...) + return ollamaClient.validateServerHost(...) + end, + makeModelItems = function(...) + return ollamaClient.makeModelItems(...) + end, + saveServerAndRefresh = function(...) + return ollamaClient.saveServerAndRefresh(...) + end, + + -- fetchAvailableModels wraps client.fetchModels to provide no-arg compatibility + fetchAvailableModels = function() + return ollamaClient.fetchModels(prefsService.prefsForPlugin()) + end, + + -- ThumbnailService functions — explicit wrappers for testability. + exportThumbnail = function(...) + return thumbnailService.export(...) + end, + base64EncodeImage = function(...) + return thumbnailService.encodeBase64(...) + end, + + -- OllamaClient.generate is the decomposition of sendDataToApi; same 7-param signature. + sendDataToApi = function(photo, prompt, currentData, useCurrentData, + useSystemPrompt, selectedModel, systemPrompt) + return ollamaClient.generate(photo, prompt, currentData, useCurrentData, + useSystemPrompt, selectedModel, systemPrompt) + end, + + -- MetadataService functions — explicit wrappers for testability. + addKeywordsWithParent = function(...) + return metadataService.addKeywordsWithParent(...) + end, + getLlmKeywordsFromPhoto = function(...) + return metadataService.getLlmKeywordsFromPhoto(...) + end, + removeLlmKeywords = function(...) + return metadataService.removeLlmKeywords(...) + end, + parseKeywordCsv = function(...) + return metadataService.parseKeywordCsv(...) + end, + } +end + +-- Load focused modules for the default production instance. local OllamaClient = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "OllamaClient.lua"))))() local ThumbnailService = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "ThumbnailService.lua"))))() local MetadataService = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "MetadataService.lua"))))() -return { - -- OllamaClient constants - model = OllamaClient.defaultModel, - defaultServerHost = OllamaClient.defaultServerHost, - - -- OllamaClient functions (direct delegation) - validateServerHost = OllamaClient.validateServerHost, - makeModelItems = OllamaClient.makeModelItems, - saveServerAndRefresh = OllamaClient.saveServerAndRefresh, - - -- fetchAvailableModels wraps client.fetchModels to provide no-arg compatibility - fetchAvailableModels = function() - return OllamaClient.fetchModels(LrPrefs.prefsForPlugin()) - end, - - -- ThumbnailService functions (direct delegation) - exportThumbnail = ThumbnailService.export, - base64EncodeImage = ThumbnailService.encodeBase64, - - -- OllamaClient.generate is the decomposition of sendDataToApi; same 7-param signature. - sendDataToApi = function(photo, prompt, currentData, useCurrentData, - useSystemPrompt, selectedModel, systemPrompt) - return OllamaClient.generate(photo, prompt, currentData, useCurrentData, - useSystemPrompt, selectedModel, systemPrompt) - end, - - -- MetadataService functions (direct delegation) - addKeywordsWithParent = MetadataService.addKeywordsWithParent, - getLlmKeywordsFromPhoto = MetadataService.getLlmKeywordsFromPhoto, - removeLlmKeywords = MetadataService.removeLlmKeywords, - parseKeywordCsv = MetadataService.parseKeywordCsv, -} +-- Create the default production compatibility object. +local common = createCommon({ + ollamaClient = OllamaClient, + thumbnailService = ThumbnailService, + metadataService = MetadataService, + prefsService = LrPrefs, +}) + +--- Create a new Common compatibility object with custom dependencies. +--- Used by tests to inject fake focused modules. Production callers ignore this. +---@param deps table Dependency map with ollamaClient, thumbnailService, metadataService, prefsService +---@return table New Common compatibility object +common.new = createCommon + +return common diff --git a/tests/spec/common_delegation_spec.lua b/tests/spec/common_delegation_spec.lua index 2943761..6e3261d 100644 --- a/tests/spec/common_delegation_spec.lua +++ b/tests/spec/common_delegation_spec.lua @@ -1,11 +1,24 @@ ---- common_delegation_spec.lua — Verify Common.lua exports all expected keys and ---- that each delegated function behaves correctly. Because loadfile() creates new ---- function objects on each call, we test behavior (output equivalence) rather ---- than pointer identity. +--- common_delegation_spec.lua — Behavioral delegation regression tests for +--- Common.lua compatibility layer. +--- +--- Every exported function is tested with fake focused modules to prove that +--- the compatibility layer forwards arguments correctly, propagates return +--- values unchanged, and doesn't silently swallow errors. Because loadfile() +--- creates new function objects each call, these tests use behavioral +--- equivalence (spy recording) rather than pointer identity. local path = PLUGIN_PATH -describe("Common.lua delegation", function() +-------------------------------------------------------------------------------- +--- Helper: pack arguments so nil intermediates don't get lost by # operator. +local function pack(...) + return { n = select("#", ...), ... } +end + +-------------------------------------------------------------------------------- +-- Suite 1: Production load test — verify default Common loads and has all keys +-------------------------------------------------------------------------------- +describe("Common.lua production load", function() local Common before_each(function() @@ -13,107 +26,494 @@ describe("Common.lua delegation", function() Common = assert(loadfile(path .. "Common.lua"))() end) - -------------------------------------------------------------------- - -- All expected keys are present and non-nil - -------------------------------------------------------------------- - describe("exports", function() - it("has all expected constants", function() - assert.is_not_nil(Common.model) - assert.is_not_nil(Common.defaultServerHost) - end) + it("exports correct default constant values", function() + assert.are_same("gemma4:latest", Common.model) + assert.are_same("localhost:11434", Common.defaultServerHost) + end) - it("has all expected functions", function() - assert.is_function(Common.validateServerHost) - assert.is_function(Common.makeModelItems) - assert.is_function(Common.saveServerAndRefresh) - assert.is_function(Common.fetchAvailableModels) - assert.is_function(Common.exportThumbnail) - assert.is_function(Common.base64EncodeImage) - assert.is_function(Common.sendDataToApi) - assert.is_function(Common.addKeywordsWithParent) - assert.is_function(Common.getLlmKeywordsFromPhoto) - assert.is_function(Common.removeLlmKeywords) - assert.is_function(Common.parseKeywordCsv) - end) + it("has all expected functions", function() + local expectedFunctions = { + "validateServerHost", + "makeModelItems", + "saveServerAndRefresh", + "fetchAvailableModels", + "exportThumbnail", + "base64EncodeImage", + "sendDataToApi", + "addKeywordsWithParent", + "getLlmKeywordsFromPhoto", + "removeLlmKeywords", + "parseKeywordCsv", + } + for _, name in ipairs(expectedFunctions) do + assert.is_function( + Common[name], + string.format("Common must export '%s' function", name) + ) + end + end) - it("exports correct default values", function() - assert.are_same("gemma4:latest", Common.model) - assert.are_same("localhost:11434", Common.defaultServerHost) - end) + it("has constructor for test dependency injection", function() + assert.is_function(Common.new, "Common must export 'new' constructor") end) +end) - -------------------------------------------------------------------- - -- Delegated functions produce correct results - -------------------------------------------------------------------- - describe("validateServerHost delegation", function() - it("accepts localhost:11434", function() - local ok, result = Common.validateServerHost("localhost:11434") - assert.is_true(ok) - assert.are_same("localhost:11434", result) +-------------------------------------------------------------------------------- +-- Suite 2: Behavioral delegation with fake modules +-------------------------------------------------------------------------------- +describe("Common.lua delegation behavior", function() + + -- Build a fresh set of fakes + common instance before each test. + local fakeOllamaClient + local fakeThumbnailService + local fakeMetadataService + local fakePrefsService + local expectedPrefs + local common + + before_each(function() + expectedPrefs = { + ollamaServerHost = "remote-server:11434", + _prefsMarker = {}, -- unique identity marker + } + + fakeOllamaClient = { + defaultModel = "test-model:42", + defaultServerHost = "test-host:1234", + validateServerHost = function() return true, "ok" end, + makeModelItems = function() return {} end, + saveServerAndRefresh = function() return true, "refreshed" end, + fetchModels = function() return {} end, + generate = function() return {}, nil end, + } + + fakeThumbnailService = { + export = function() return "/fake/thumbnail.jpg" end, + encodeBase64 = function() return "fake-base64-data" end, + } + + fakeMetadataService = { + addKeywordsWithParent = function() end, + getLlmKeywordsFromPhoto = function() return {} end, + removeLlmKeywords = function() end, + parseKeywordCsv = function() return {} end, + } + + fakePrefsService = { + prefsForPlugin = function() return expectedPrefs end, + } + + -- Load Common.lua fresh to get the constructor. We only need the `new` + -- method; the production instance loads fine under mock_sdk but we want + -- isolated fakes for behavioral tests. + _PLUGIN = { path = path } + local CommonModule = assert(loadfile(path .. "Common.lua"))() + common = CommonModule.new({ + ollamaClient = fakeOllamaClient, + thumbnailService = fakeThumbnailService, + metadataService = fakeMetadataService, + prefsService = fakePrefsService, + }) + end) + + ------------------------------------------------------------------------ + -- 1. Constants + ------------------------------------------------------------------------ + describe("constants", function() + it("exposes model from ollamaClient.defaultModel", function() + assert.are_same("test-model:42", common.model) end) - it("rejects missing port", function() - local ok = Common.validateServerHost("localhost") - assert.is_false(ok) + it("exposes defaultServerHost from ollamaClient.defaultServerHost", function() + assert.are_same("test-host:1234", common.defaultServerHost) end) + end) - it("normalizes http:// prefix", function() - local ok, result = Common.validateServerHost("http://192.168.1.5:9000/") - assert.is_true(ok) - assert.are_same("192.168.1.5:9000", result) + ------------------------------------------------------------------------ + -- 2. Direct OllamaClient aliases — validateServerHost + ------------------------------------------------------------------------ + describe("validateServerHost delegation", function() + it("forwards all arguments and propagates all return values", function() + local received + fakeOllamaClient.validateServerHost = function(...) + received = pack(...) + return "ret-1", "ret-2", "ret-3" + end + + local a, b, c = common.validateServerHost("host:1234", "extra") + assert.are_same(2, received.n) + assert.are_same("host:1234", received[1]) + assert.are_same("extra", received[2]) + assert.are_same("ret-1", a) + assert.are_same("ret-2", b) + assert.are_same("ret-3", c) end) end) + ------------------------------------------------------------------------ + -- 2b. Direct OllamaClient aliases — makeModelItems + ------------------------------------------------------------------------ describe("makeModelItems delegation", function() - it("converts model names to title/value pairs", function() - local items = Common.makeModelItems({ "gemma4:latest", "llama3" }) - assert.are_same(2, #items) - assert.are_same("gemma4:latest", items[1].title) - assert.are_same("gemma4:latest", items[1].value) + it("forwards arguments and propagates return value", function() + local received + local expectedItems = { + { title = "m1", value = "m1" }, + { title = "m2", value = "m2" }, + } + fakeOllamaClient.makeModelItems = function(models) + received = models + return expectedItems + end + + local models = { "m1", "m2" } + local result = common.makeModelItems(models) + assert.is_true(received == models, "same table passed through") + assert.are_same(expectedItems, result) end) end) - describe("sendDataToApi wrapper", function() - it("is a function, not a table or nil", function() - assert.is_function(Common.sendDataToApi) + ------------------------------------------------------------------------ + -- 2c. Direct OllamaClient aliases — saveServerAndRefresh + ------------------------------------------------------------------------ + describe("saveServerAndRefresh delegation", function() + it("forwards all arguments and propagates all return values", function() + local received + fakeOllamaClient.saveServerAndRefresh = function(...) + received = pack(...) + return true, "models-reloaded" + end + + local props = { serverHost = "localhost:11434", _marker = "props" } + local prefs = { ollamaServerHost = "localhost:11434", _marker = "prefs" } + + local a, b = common.saveServerAndRefresh(props, prefs) + assert.are_same(2, received.n) + assert.is_true(received[1] == props, "same props object") + assert.is_true(received[2] == prefs, "same prefs object") + assert.is_true(a) + assert.are_same("models-reloaded", b) end) end) - describe("fetchAvailableModels wrapper", function() - it("is a function, not a table or nil", function() - assert.is_function(Common.fetchAvailableModels) + ------------------------------------------------------------------------ + -- 3. fetchAvailableModels wrapper + ------------------------------------------------------------------------ + describe("fetchAvailableModels delegation", function() + it("calls prefsForPlugin and passes the exact result to fetchModels", function() + local receivedPrefs + fakeOllamaClient.fetchModels = function(prefs) + receivedPrefs = prefs + return { "model-a", "model-b" } + end + + local models = common.fetchAvailableModels() + -- Identity check: same object, not just equal contents + assert.is_true( + receivedPrefs == expectedPrefs, + "fetchAvailableModels must pass the exact prefs object from prefsForPlugin()" + ) + assert.are_same({ "model-a", "model-b" }, models) end) - end) - describe("parseKeywordCsv delegation", function() - it("parses tricky CSV correctly", function() - local got = Common.parseKeywordCsv("sunset,, beach, ,portrait") - assert.are_same({ "sunset", "beach", "portrait" }, got) + it("passes no extra arguments to fetchModels", function() + local argCount = 0 + fakeOllamaClient.fetchModels = function(...) + argCount = select("#", ...) + end + + common.fetchAvailableModels() + assert.are_same(1, argCount, "fetchModels should receive exactly 1 argument (prefs)") end) - end) - -------------------------------------------------------------------- - -- Production wiring: SinglePhotoController uses the tested parser - -------------------------------------------------------------------- - describe("production wiring", function() - it("SinglePhotoController uses common.parseKeywordCsv and has no inline parsing", function() - local source = assert(io.open(path .. "SinglePhotoController.lua", "r")):read("*a") + it("propagates all return values from fetchModels", function() + fakeOllamaClient.fetchModels = function() + return { "only-one" }, "second-return" + end + + local a, b = common.fetchAvailableModels() + assert.are_same({ "only-one" }, a) + assert.are_same("second-return", b) + end) + + it("surfaces errors from fetchModels instead of swallowing them", function() + fakeOllamaClient.fetchModels = function() + error("fetch-models-failure") + end + + local ok, err = pcall(common.fetchAvailableModels) + assert.is_false(ok, "should propagate errors from fetchModels") assert.truthy( - string.find(source, "common%.parseKeywordCsv"), - "SinglePhotoController.lua should call common.parseKeywordCsv" + string.find(err, "fetch-models-failure", 1, true), + string.format("error message should mention the original failure, got: %s", tostring(err)) ) - assert.falsy( - string.find(source, "gmatch.*keywords.*%[%,", nil, true), - "SinglePhotoController.lua should not contain inline gmatch keyword parsing" + end) + end) + + ------------------------------------------------------------------------ + -- 4. sendDataToApi wrapper — highest priority + ------------------------------------------------------------------------ + describe("sendDataToApi delegation", function() + it("forwards all 7 arguments in correct order", function() + local received + fakeOllamaClient.generate = function(...) + received = pack(...) + return { title = "T", caption = "C", keywords = { "k1" } }, nil + end + + local photo = { _type = "photo" } + local prompt = "PROMPT-ARGUMENT" + local currentData = { title = "CURRENT-TITLE", caption = "CURRENT-CAPTION" } + local useCurrent = true + local useSystem = false + local model = "MODEL-ARGUMENT" + local sysPrompt = "SYSTEM-PROMPT-ARGUMENT" + + common.sendDataToApi(photo, prompt, currentData, useCurrent, + useSystem, model, sysPrompt) + + assert.are_same(7, received.n, "generate must receive all 7 arguments") + assert.is_true(received[1] == photo, "arg 1: photo identity") + assert.are_same(prompt, received[2], "arg 2: prompt value") + assert.is_true(received[3] == currentData, "arg 3: currentData identity") + assert.is_true(received[4] == useCurrent, "arg 4: useCurrentData boolean") + assert.is_false(received[5], "arg 5: useSystemPrompt boolean") + assert.are_same(model, received[6], "arg 6: selectedModel value") + assert.are_same(sysPrompt, received[7], "arg 7: systemPrompt value") + end) + + it("preserves nil middle arguments without shifting later args", function() + local received + fakeOllamaClient.generate = function(...) + received = pack(...) + return {}, nil + end + + common.sendDataToApi( + { _p = "photo" }, -- 1: photo + "prompt", -- 2: prompt + nil, -- 3: currentData is nil + true, -- 4: useCurrentData + false, -- 5: useSystemPrompt + nil, -- 6: selectedModel is nil + "sys-prompt" -- 7: systemPrompt ) + + assert.are_same(7, received.n, "must forward exactly 7 positions even with nil intermediates") + assert.is_true(type(received[1]) == "table", "arg 1 should be the photo table") + assert.are_same("prompt", received[2], "arg 2 should be 'prompt'") + assert.is_nil(received[3], "arg 3 should be nil (currentData)") + assert.is_true(received[4], "arg 4 should be true (useCurrentData)") + assert.is_false(received[5], "arg 5 should be false (useSystemPrompt)") + assert.is_nil(received[6], "arg 6 should be nil (selectedModel)") + assert.are_same("sys-prompt", received[7], "arg 7 should still be system prompt") + end) + + it("propagates both return values on success", function() + local response = { + title = "Generated title", + caption = "Generated caption", + keywords = { "one", "two" }, + } + fakeOllamaClient.generate = function() + return response, nil + end + + local a, b = common.sendDataToApi(nil, nil, nil, nil, nil, nil, nil) + assert.is_true(a == response, "response table identity preserved") + assert.is_nil(b, "error should be nil on success") + end) + + it("propagates both return values on failure", function() + fakeOllamaClient.generate = function() + return nil, "generation failed" + end + + local a, b = common.sendDataToApi(nil, nil, nil, nil, nil, nil, nil) + assert.is_nil(a, "response should be nil on failure") + assert.are_same("generation failed", b) end) - it("LrLlama.lua has no inline keyword parsing loop", function() - local source = assert(io.open(path .. "LrLlama.lua", "r")):read("*a") - assert.falsy( - string.find(source, "gmatch.*keywords.*%[%,", nil, true), - "LrLlama.lua should not contain inline gmatch keyword parsing" + it("surfaces errors from generate instead of swallowing", function() + fakeOllamaClient.generate = function() + error("generate-wrapper-failure") + end + + local ok, err = pcall(common.sendDataToApi, nil, nil, nil, nil, nil, nil, nil) + assert.is_false(ok, "should propagate errors from generate") + assert.truthy( + string.find(err, "generate-wrapper-failure", 1, true), + string.format("error message should mention the original failure, got: %s", tostring(err)) ) end) end) + + ------------------------------------------------------------------------ + -- 5. ThumbnailService delegation + ------------------------------------------------------------------------ + describe("exportThumbnail delegation", function() + it("forwards photo and propagates return value", function() + local received + fakeThumbnailService.export = function(photo) + received = photo + return "/fake/thumbnail-123.jpg" + end + + local photo = { _type = "photo" } + local result = common.exportThumbnail(photo) + assert.is_true(received == photo, "same photo object forwarded") + assert.are_same("/fake/thumbnail-123.jpg", result) + end) + + it("propagates multiple return values", function() + local received + fakeThumbnailService.export = function(p) + received = p + return "/path.jpg", "extra-return" + end + + local a, b = common.exportThumbnail({ _m = 1 }) + assert.is_true(received == { _m = 1 } or received._m == 1) + assert.are_same("/path.jpg", a) + assert.are_same("extra-return", b) + end) + end) + + describe("base64EncodeImage delegation", function() + it("forwards path and propagates return value", function() + local received + fakeThumbnailService.encodeBase64 = function(imagePath) + received = imagePath + return "encoded-data-xyz" + end + + local result = common.base64EncodeImage("/some/image.jpg") + assert.are_same("/some/image.jpg", received) + assert.are_same("encoded-data-xyz", result) + end) + + it("propagates multiple return values", function() + local received + fakeThumbnailService.encodeBase64 = function(path) + received = path + return "data", "second" + end + + local a, b = common.base64EncodeImage("/path.jpg") + assert.are_same("/path.jpg", received) + assert.are_same("data", a) + assert.are_same("second", b) + end) + end) + + ------------------------------------------------------------------------ + -- 6. MetadataService delegation + ------------------------------------------------------------------------ + describe("addKeywordsWithParent delegation", function() + it("forwards all arguments with correct identity", function() + local receivedCatalog, receivedPhoto, receivedKw + fakeMetadataService.addKeywordsWithParent = function(catalog, photo, keywords) + receivedCatalog = catalog + receivedPhoto = photo + receivedKw = keywords + end + + local catalog = { _type = "catalog" } + local photo = { _type = "photo" } + local keywords = { "sunset", "portrait" } + + common.addKeywordsWithParent(catalog, photo, keywords) + assert.is_true(receivedCatalog == catalog, "same catalog object") + assert.is_true(receivedPhoto == photo, "same photo object") + assert.is_true(receivedKw == keywords, "same keywords table") + end) + end) + + describe("getLlmKeywordsFromPhoto delegation", function() + it("forwards photo and propagates return value", function() + local received + local expectedResult = { "cat", "outdoor" } + fakeMetadataService.getLlmKeywordsFromPhoto = function(photo) + received = photo + return expectedResult + end + + local photo = { _type = "photo" } + local result = common.getLlmKeywordsFromPhoto(photo) + assert.is_true(received == photo, "same photo object") + assert.are_same(expectedResult, result) + end) + end) + + describe("removeLlmKeywords delegation", function() + it("forwards catalog and photo with correct identity", function() + local receivedCatalog, receivedPhoto + fakeMetadataService.removeLlmKeywords = function(catalog, photo) + receivedCatalog = catalog + receivedPhoto = photo + end + + local catalog = { _type = "catalog" } + local photo = { _type = "photo" } + + common.removeLlmKeywords(catalog, photo) + assert.is_true(receivedCatalog == catalog, "same catalog object") + assert.is_true(receivedPhoto == photo, "same photo object") + end) + end) + + describe("parseKeywordCsv delegation", function() + it("delegates to MetadataService instead of duplicating parser logic", function() + local receivedCsv + fakeMetadataService.parseKeywordCsv = function(value) + receivedCsv = value + return { "delegated-result" } + end + + local result = common.parseKeywordCsv("sunset, beach, portrait") + assert.are_same("sunset, beach, portrait", receivedCsv) + assert.are_same({ "delegated-result" }, result) + end) + + it("propagates nil input without error", function() + local receivedCsv + fakeMetadataService.parseKeywordCsv = function(value) + receivedCsv = value + return {} + end + + common.parseKeywordCsv(nil) + assert.is_nil(receivedCsv, "nil should be forwarded to the delegate") + end) + end) + + ------------------------------------------------------------------------ + -- 7. API surface regression — all expected keys present + ------------------------------------------------------------------------ + describe("complete API surface", function() + it("has every expected compatibility key", function() + local expectedFunctions = { + "validateServerHost", + "makeModelItems", + "saveServerAndRefresh", + "fetchAvailableModels", + "exportThumbnail", + "base64EncodeImage", + "sendDataToApi", + "addKeywordsWithParent", + "getLlmKeywordsFromPhoto", + "removeLlmKeywords", + "parseKeywordCsv", + } + for _, name in ipairs(expectedFunctions) do + assert.is_function( + common[name], + string.format("common must have function '%s'", name) + ) + end + + -- Constants + assert.is_not_nil(common.model, "common must have 'model' constant") + assert.is_not_nil(common.defaultServerHost, "common must have 'defaultServerHost' constant") + end) + end) end) From 8122b003a5b815ba9ae620ca98ca18ee5d70db99 Mon Sep 17 00:00:00 2001 From: Zack Stout Date: Fri, 17 Jul 2026 13:41:44 -0500 Subject: [PATCH 06/10] test: add unit tests for MetadataService keyword functions Add 33 behavioral tests for addKeywordsWithParent, getLlmKeywordsFromPhoto, and removeLlmKeywords using plain-Lua fake catalog, photo, and keyword objects. No real Lightroom SDK required. Coverage includes: - Invalid input guards (nil, non-table types) - Parent creation with exact argument verification - Child creation under exact llm parent object (identity assertions) - Empty/nil entry handling and ipairs truncation behavior - Partial child creation failure resilience - Parent creation failure error propagation - Keyword filtering with exact parent-name matching - pcall all-or-nothing error protection characterization - removeLlmKeywords removes only llm children by identity - Error semantics asymmetry documented (get uses pcall, remove does not) Mutation verification confirmed detection of 4 representative defects. --- tests/spec/metadata_service_keywords_spec.lua | 546 ++++++++++++++++++ 1 file changed, 546 insertions(+) create mode 100644 tests/spec/metadata_service_keywords_spec.lua diff --git a/tests/spec/metadata_service_keywords_spec.lua b/tests/spec/metadata_service_keywords_spec.lua new file mode 100644 index 0000000..d2bca12 --- /dev/null +++ b/tests/spec/metadata_service_keywords_spec.lua @@ -0,0 +1,546 @@ +--- metadata_service_keywords_spec.lua — Unit tests for MetadataService catalog-bound +--- keyword functions: addKeywordsWithParent, getLlmKeywordsFromPhoto, removeLlmKeywords. +--- Uses plain-Lua fake objects; no real Lightroom SDK required. + +local path = PLUGIN_PATH + +-------------------------------------------------------------------------------- +--- Factory helpers for fake Lightroom keyword objects. +-------------------------------------------------------------------------------- +local function makeParent(name) + return { + getName = function() + return name + end, + } +end + +local function makeKeyword(name, parent) + return { + getName = function() + return name + end, + getParent = function() + return parent + end, + } +end + +-------------------------------------------------------------------------------- +--- Helper: build a fake catalog + photo with call recording. +--- Each call returns configurable values set by individual tests. +-------------------------------------------------------------------------------- +local function makeFakeCatalog() + local catalog = { + createKeyword_calls = {}, + -- Per-call return values: index into this array by call number + createKeyword_returns = {}, + } + -- MetadataService calls with colon notation: catalog:createKeyword(...) + -- so self is passed as first argument. Use dot-notation signature that + -- explicitly receives and ignores self. + catalog.createKeyword = function(_, name, _, scope, parent, system) + table.insert(catalog.createKeyword_calls, { + name = name, + scope = scope, + parent = parent, + system = system, + }) + local idx = #catalog.createKeyword_calls + return catalog.createKeyword_returns[idx] + end + return catalog +end + +local function makeFakePhoto() + local photo = { + addKeyword_calls = {}, + removeKeyword_calls = {}, + getRawMetadata_return = nil, + getRawMetadata_error = nil, + } + -- All called with colon notation: photo:xxx(...) → self is first arg + photo.addKeyword = function(_, keyword) + table.insert(photo.addKeyword_calls, keyword) + end + photo.removeKeyword = function(_, keyword) + table.insert(photo.removeKeyword_calls, keyword) + end + photo.getRawMetadata = function(_, key) + if photo.getRawMetadata_error then + error(photo.getRawMetadata_error) + end + return photo.getRawMetadata_return + end + return photo +end + +-------------------------------------------------------------------------------- + +describe("MetadataService.addKeywordsWithParent", function() + local metadata + local catalog + local photo + + before_each(function() + metadata = assert(loadfile(path .. "MetadataService.lua"))() + catalog = makeFakeCatalog() + photo = makeFakePhoto() + end) + + --------------------------------------------------------------- + --- Invalid input — early return without side effects + --------------------------------------------------------------- + describe("invalid input", function() + it("returns without calling catalog methods for nil", function() + metadata.addKeywordsWithParent(catalog, photo, nil) + assert.are_same(0, #catalog.createKeyword_calls) + assert.are_same(0, #photo.addKeyword_calls) + end) + + it("returns without calling catalog methods for a string", function() + metadata.addKeywordsWithParent(catalog, photo, "sunset") + assert.are_same(0, #catalog.createKeyword_calls) + end) + + it("returns without calling catalog methods for a number", function() + metadata.addKeywordsWithParent(catalog, photo, 42) + assert.are_same(0, #catalog.createKeyword_calls) + end) + + it("returns without calling catalog methods for a boolean", function() + metadata.addKeywordsWithParent(catalog, photo, true) + assert.are_same(0, #catalog.createKeyword_calls) + end) + end) + + --------------------------------------------------------------- + --- Empty table — documents current behavior (parent still created) + --------------------------------------------------------------- + describe("empty table", function() + before_each(function() + catalog.createKeyword_returns[1] = makeParent("llm") + end) + + it("creates the llm parent keyword", function() + metadata.addKeywordsWithParent(catalog, photo, {}) + assert.are_same(1, #catalog.createKeyword_calls) + assert.are_same("llm", catalog.createKeyword_calls[1].name) + end) + + it("does not create child keywords", function() + metadata.addKeywordsWithParent(catalog, photo, {}) + assert.are_same(1, #catalog.createKeyword_calls) + assert.are_same(0, #photo.addKeyword_calls) + end) + end) + + --------------------------------------------------------------- + --- Parent creation arguments + --------------------------------------------------------------- + describe("parent creation", function() + before_each(function() + catalog.createKeyword_returns[1] = makeParent("llm") + end) + + it("creates parent with correct arguments", function() + metadata.addKeywordsWithParent(catalog, photo, { "sunset" }) + local call = catalog.createKeyword_calls[1] + assert.are_same("llm", call.name) + assert.are_same(true, call.scope) + assert.are_same(nil, call.parent) + assert.are_same(true, call.system) + end) + end) + + --------------------------------------------------------------- + --- Success path — child creation and association + --------------------------------------------------------------- + describe("success path", function() + local llmParent + local sunsetChild + local portraitChild + + before_each(function() + llmParent = { marker = "llm-parent" } + sunsetChild = { marker = "sunset-child" } + portraitChild = { marker = "portrait-child" } + catalog.createKeyword_returns[1] = llmParent + catalog.createKeyword_returns[2] = sunsetChild + catalog.createKeyword_returns[3] = portraitChild + end) + + it("creates parent once and children under it", function() + metadata.addKeywordsWithParent(catalog, photo, { "sunset", "portrait" }) + assert.are_same(3, #catalog.createKeyword_calls) + -- Parent created with nil parent-arg + assert.are_same(nil, catalog.createKeyword_calls[1].parent) + -- Children created under exact llmParent object + assert.is_true(catalog.createKeyword_calls[2].parent == llmParent) + assert.is_true(catalog.createKeyword_calls[3].parent == llmParent) + end) + + it("adds each child keyword to the photo in order", function() + metadata.addKeywordsWithParent(catalog, photo, { "sunset", "portrait" }) + assert.are_same(2, #photo.addKeyword_calls) + assert.is_true(photo.addKeyword_calls[1] == sunsetChild) + assert.is_true(photo.addKeyword_calls[2] == portraitChild) + end) + + it("passes correct arguments for child creation", function() + metadata.addKeywordsWithParent(catalog, photo, { "sunset" }) + local childCall = catalog.createKeyword_calls[2] + assert.are_same("sunset", childCall.name) + assert.are_same(true, childCall.scope) + assert.is_true(childCall.parent == llmParent) + assert.are_same(true, childCall.system) + end) + end) + + --------------------------------------------------------------- + --- Empty and nil entries + --------------------------------------------------------------- + describe("empty and nil entries", function() + before_each(function() + catalog.createKeyword_returns[1] = makeParent("llm") + end) + + it("skips empty string entries", function() + local child1 = { marker = "child1" } + local child2 = { marker = "child2" } + catalog.createKeyword_returns[2] = child1 + catalog.createKeyword_returns[3] = child2 + metadata.addKeywordsWithParent(catalog, photo, { "sunset", "", "portrait" }) + -- 1 parent + 2 children (empty skipped) + assert.are_same(3, #catalog.createKeyword_calls) + assert.are_same(2, #photo.addKeyword_calls) + end) + + it("stops at nil hole due to ipairs semantics", function() + local child1 = { marker = "child1" } + catalog.createKeyword_returns[2] = child1 + -- ipairs stops at first nil, so "portrait" after nil is never reached + metadata.addKeywordsWithParent(catalog, photo, { "sunset", nil, "portrait" }) + -- 1 parent + 1 child (only "sunset" processed) + assert.are_same(2, #catalog.createKeyword_calls) + assert.are_same(1, #photo.addKeyword_calls) + end) + end) + + --------------------------------------------------------------- + --- Partial child creation failure + --------------------------------------------------------------- + describe("partial child failure", function() + before_each(function() + catalog.createKeyword_returns[1] = makeParent("llm") + -- First child creation fails (returns nil) + catalog.createKeyword_returns[2] = nil + -- Second child succeeds + catalog.createKeyword_returns[3] = { marker = "portrait-child" } + end) + + it("skips failed child but continues processing", function() + metadata.addKeywordsWithParent(catalog, photo, { "sunset", "portrait" }) + -- 1 parent + 2 children attempted + assert.are_same(3, #catalog.createKeyword_calls) + -- Only successful child added to photo + assert.are_same(1, #photo.addKeyword_calls) + end) + + it("does not call addKeyword for nil child", function() + metadata.addKeywordsWithParent(catalog, photo, { "sunset", "portrait" }) + -- The first addKeyword would be for sunset (which returned nil) — must not be called + assert.are_same(1, #photo.addKeyword_calls) + end) + end) + + --------------------------------------------------------------- + --- Parent creation failure + --------------------------------------------------------------- + describe("parent creation failure", function() + before_each(function() + catalog.createKeyword_returns[1] = nil + end) + + it("raises error when parent cannot be created", function() + assert.has_error( + function() + metadata.addKeywordsWithParent(catalog, photo, { "sunset" }) + end, + "Failed to create or get 'llm' parent keyword" + ) + end) + + it("does not create children when parent fails", function() + pcall(function() + metadata.addKeywordsWithParent(catalog, photo, { "sunset" }) + end) + -- Only the parent creation was attempted + assert.are_same(1, #catalog.createKeyword_calls) + assert.are_same(0, #photo.addKeyword_calls) + end) + end) +end) + +-------------------------------------------------------------------------------- + +describe("MetadataService.getLlmKeywordsFromPhoto", function() + local metadata + local photo + + before_each(function() + metadata = assert(loadfile(path .. "MetadataService.lua"))() + photo = makeFakePhoto() + end) + + --------------------------------------------------------------- + --- Keyword filtering + --------------------------------------------------------------- + describe("keyword filtering", function() + it("returns only llm-parented keywords in order", function() + local llmParent = makeParent("llm") + local peopleParent = makeParent("people") + photo.getRawMetadata_return = { + makeKeyword("dog", peopleParent), -- not llm + makeKeyword("sunset", llmParent), -- llm + makeKeyword("landscape"), -- no parent (top-level) + makeKeyword("portrait", llmParent), -- llm + } + local result = metadata.getLlmKeywordsFromPhoto(photo) + assert.are_same({ "sunset", "portrait" }, result) + end) + + it("returns empty table when no keywords", function() + photo.getRawMetadata_return = nil + assert.are_same({}, metadata.getLlmKeywordsFromPhoto(photo)) + end) + + it("returns empty table for empty keyword list", function() + photo.getRawMetadata_return = {} + assert.are_same({}, metadata.getLlmKeywordsFromPhoto(photo)) + end) + + it("requires exact parent name match — rejects LLM", function() + local llmParent = makeParent("llm") + local upperParent = makeParent("LLM") + photo.getRawMetadata_return = { + makeKeyword("good", llmParent), + makeKeyword("bad", upperParent), + } + assert.are_same({ "good" }, metadata.getLlmKeywordsFromPhoto(photo)) + end) + + it("requires exact parent name match — rejects llm-child and my-llm", function() + local llmParent = makeParent("llm") + local childParent = makeParent("llm-child") + local myParent = makeParent("my-llm") + photo.getRawMetadata_return = { + makeKeyword("keep", llmParent), + makeKeyword("drop1", childParent), + makeKeyword("drop2", myParent), + } + assert.are_same({ "keep" }, metadata.getLlmKeywordsFromPhoto(photo)) + end) + end) + + --------------------------------------------------------------- + --- pcall error protection — all-or-nothing behavior + --------------------------------------------------------------- + describe("pcall error protection", function() + it("returns empty table when getRawMetadata raises", function() + photo.getRawMetadata_error = "metadata unavailable" + assert.are_same({}, metadata.getLlmKeywordsFromPhoto(photo)) + end) + + it("returns empty table when getParent raises on any keyword", function() + local llmParent = makeParent("llm") + local badKeyword = { + getName = function() return "broken" end, + getParent = function() error("getParent failed") end, + } + photo.getRawMetadata_return = { + makeKeyword("sunset", llmParent), + badKeyword, + } + -- pcall wraps entire loop — returns {} on any error + assert.are_same( + {}, + metadata.getLlmKeywordsFromPhoto(photo), + "pcall catches getParent error and returns empty table" + ) + end) + + it("returns empty table when parent getName raises", function() + local badParent = { + getName = function() error("getName failed") end, + } + photo.getRawMetadata_return = { + makeKeyword("sunset", badParent), + } + assert.are_same( + {}, + metadata.getLlmKeywordsFromPhoto(photo), + "pcall catches parent:getName error" + ) + end) + + it("returns empty table when keyword getName raises after parent match", function() + -- This tests the case where parent matches "llm" but the keyword's + -- own getName fails. The pcall wraps the entire loop, so partial + -- results are discarded — all-or-nothing behavior. + local goodParent = makeParent("llm") + local badKeyword = { + getParent = function() return goodParent end, + getName = function() error("getName failed") end, + } + photo.getRawMetadata_return = { + makeKeyword("sunset", goodParent), + badKeyword, + } + assert.are_same( + {}, + metadata.getLlmKeywordsFromPhoto(photo), + "pcall discards partial results when keyword getName fails" + ) + end) + end) +end) + +-------------------------------------------------------------------------------- + +describe("MetadataService.removeLlmKeywords", function() + local metadata + local catalog + local photo + + before_each(function() + metadata = assert(loadfile(path .. "MetadataService.lua"))() + catalog = makeFakeCatalog() + photo = makeFakePhoto() + end) + + --------------------------------------------------------------- + --- Removal filtering + --------------------------------------------------------------- + describe("removal filtering", function() + it("removes only llm-parented keywords", function() + local llmParent = makeParent("llm") + local peopleParent = makeParent("people") + local sunsetKeyword = makeKeyword("sunset", llmParent) + local dogKeyword = makeKeyword("dog", peopleParent) + local landscapeKw = makeKeyword("landscape") -- top-level + local portraitKw = makeKeyword("portrait", llmParent) + + photo.getRawMetadata_return = { + sunsetKeyword, -- index 1 — remove + dogKeyword, -- index 2 — keep + landscapeKw, -- index 3 — keep (top-level) + portraitKw, -- index 4 — remove + } + + metadata.removeLlmKeywords(catalog, photo) + + assert.are_same(2, #photo.removeKeyword_calls) + assert.is_true(photo.removeKeyword_calls[1] == sunsetKeyword) + assert.is_true(photo.removeKeyword_calls[2] == portraitKw) + end) + + it("does not remove keywords with similar parent names", function() + local llmParent = makeParent("llm") + local upperParent = makeParent("LLM") + local childParent = makeParent("llm-child") + local myParent = makeParent("my-llm") + + photo.getRawMetadata_return = { + makeKeyword("keep1", llmParent), -- remove (exact match) + makeKeyword("keep2", upperParent), -- keep + makeKeyword("keep3", childParent), -- keep + makeKeyword("keep4", myParent), -- keep + } + + metadata.removeLlmKeywords(catalog, photo) + + assert.are_same(1, #photo.removeKeyword_calls) + end) + + it("returns early when no keyword metadata", function() + photo.getRawMetadata_return = nil + metadata.removeLlmKeywords(catalog, photo) + assert.are_same(0, #photo.removeKeyword_calls) + end) + + it("does nothing for empty keyword list", function() + photo.getRawMetadata_return = {} + metadata.removeLlmKeywords(catalog, photo) + assert.are_same(0, #photo.removeKeyword_calls) + end) + end) + + --------------------------------------------------------------- + --- Catalog argument behavior + --------------------------------------------------------------- + describe("catalog argument", function() + it("accepts catalog without calling any catalog methods", function() + local llmParent = makeParent("llm") + photo.getRawMetadata_return = { + makeKeyword("sunset", llmParent), + } + metadata.removeLlmKeywords(catalog, photo) + -- removeLlmKeywords accepts catalog but does not use it + assert.are_same(0, #catalog.createKeyword_calls) + end) + + it("does not delete keyword definitions from catalog", function() + -- Use a catalog that throws on any method call to verify nothing is called + local throwCatalog = { + createKeyword_calls = {}, + createKeyword = function() + error("catalog method should not be called") + end, + deleteKeyword = function() + error("deleteKeyword should not be called") + end, + } + local llmParent = makeParent("llm") + photo.getRawMetadata_return = { + makeKeyword("sunset", llmParent), + } + -- Must not raise — function only touches photo, not catalog + metadata.removeLlmKeywords(throwCatalog, photo) + assert.are_same(1, #photo.removeKeyword_calls) + end) + end) +end) + +-------------------------------------------------------------------------------- + +describe("Error semantics characterization", function() + --- Documents the asymmetry: getLlmKeywordsFromPhoto uses pcall (returns {} on + --- error), while removeLlmKeywords does not (propagates errors). This is + --- intentional behavior — do not change it without an explicit decision. + + local metadata + + before_each(function() + metadata = assert(loadfile(path .. "MetadataService.lua"))() + end) + + it("getLlmKeywordsFromPhoto returns {} on metadata error (pcall)", function() + local photo = makeFakePhoto() + photo.getRawMetadata_error = "I/O error reading metadata" + local result = metadata.getLlmKeywordsFromPhoto(photo) + assert.are_same({}, result) + end) + + it("removeLlmKeywords propagates metadata error (no pcall)", function() + local photo = makeFakePhoto() + photo.getRawMetadata_error = "I/O error reading metadata" + local catalog = makeFakeCatalog() + assert.has_error( + function() + metadata.removeLlmKeywords(catalog, photo) + end, + "I/O error reading metadata" + ) + end) +end) From ba9dcd8dd733a5af5644565a0881f9e68904c408 Mon Sep 17 00:00:00 2001 From: Zack Stout Date: Fri, 17 Jul 2026 16:15:33 -0500 Subject: [PATCH 07/10] fix: replace xpcall with pcall for Lightroom sandbox compatibility Lightroom's embedded Lua VM doesn't expose xpcall or debug.traceback. Replace the exception-handling wrapper in runWithCleanup() with pcall, which is available in the sandbox and still surfaces error messages with line numbers. --- lightroom-llama.lrplugin/OllamaClient.lua | 170 +++++++++++++++++----- 1 file changed, 132 insertions(+), 38 deletions(-) diff --git a/lightroom-llama.lrplugin/OllamaClient.lua b/lightroom-llama.lrplugin/OllamaClient.lua index 26c0a44..3030168 100644 --- a/lightroom-llama.lrplugin/OllamaClient.lua +++ b/lightroom-llama.lrplugin/OllamaClient.lua @@ -19,9 +19,64 @@ local ThumbnailService = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "Thumb local PromptBuilder = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "PromptBuilder.lua"))))() local ResponseValidator = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "ResponseValidator.lua"))))() +-------------------------------------------------------------------------------- +--- Helpers for exception-safe cleanup +-------------------------------------------------------------------------------- + +--- Pack variadic returns preserving nil positions (Lua 5.1 compatible). +---@return table results Table with `.n` count and packed values +local function pack(...) + return { n = select("#", ...), ... } +end + +-- Lua 5.1: unpack is a global; Lua 5.2+: it moved to table.unpack. +local unpack = unpack or table.unpack + +--- Run *operation* under pcall, then always attempt *cleanupFn*. +--- Guarantees the temp thumbnail is deleted regardless of how the +--- generation pipeline exits (success, returned failure, or exception). +--- +--- Error preservation: +--- - Generation fails + cleanup succeeds → return original error +--- - Generation fails + cleanup fails → return original error; log cleanup +--- - Generation succeeds + cleanup fails → return cleanup error (no false success) +--- The `results` table is filled by `pack()` inside pcall so multiple return +--- values from the operation survive the protected call. +---@param thumbnailPath string Path to delete after operation +---@param operation function Generation logic returning zero or more values +---@param cleanupFn function Cleanup function accepting the path +---@param activeLogger table Logger for dual-failure diagnostics +---@return ... The return values of *operation* on success, or `nil, error` on failure +local function runWithCleanup(thumbnailPath, operation, cleanupFn, activeLogger) + local results = { n = 0 } + + local ok, errInfo = pcall(function() + results = pack(operation()) + end) + + -- Always attempt cleanup regardless of operation outcome. + local cleanupOk, cleanupErr = pcall(cleanupFn, thumbnailPath) + + if not ok then + -- Generation threw an exception. + if not cleanupOk then + activeLogger:error( + "Thumbnail cleanup also failed: " .. tostring(cleanupErr) + ) + end + return nil, errInfo + end + + -- Generation succeeded but cleanup failed — do not report success. + if not cleanupOk then + return nil, "Failed to clean up thumbnail: " .. tostring(cleanupErr) + end + + return unpack(results, 1, results.n) +end + -------------------------------------------------------------------------------- --- Constructor — create a client with injectable dependencies. ---- Each dep falls back to the real Lightroom SDK when omitted, so production --- callers need no changes while tests can provide lightweight fakes. ---@param deps table|nil Optional dependency overrides: --- { http, prefs, tasks, json, thumbnailService, promptBuilder, responseValidator, logger } @@ -208,48 +263,43 @@ local function createClient(deps) -- Generate pipeline -------------------------------------------------------------------------------- - --- High-level generate: export thumbnail, encode, build prompt, POST, validate response. - --- Orchestrates ThumbnailService + PromptBuilder + ResponseValidator internally. - --- This is the decomposition of the former Common.sendDataToApi mega-function. - --- Retries thumbnail export up to 3 times. Cleans up the temp file after the - --- HTTP request regardless of success or failure. Validates the JSON response - --- has title (string), caption (string), and keywords (array of non-empty strings). - ---@param photo LrPhoto Photo object from the catalog - ---@param userInstruction string User-facing prompt text - ---@param currentData table|nil Existing title/caption to prepend when useCurrentData is true - ---@param useCurrentData boolean Whether to include current metadata in the prompt - ---@param useSystemPrompt boolean Whether to send a system prompt alongside the user prompt - ---@param selectedModel string|nil Model name; falls back to the default if nil - ---@param systemPromptOverride string|nil Override for the built-in system prompt - ---@return table|nil response Parsed JSON on success ({title, caption, keywords}) - ---@return string|nil err Error message, or nil on success - function client.generate(photo, userInstruction, currentData, useCurrentData, - useSystemPrompt, selectedModel, systemPromptOverride) - activeLogger:info("Sending data to API") - - -- 1. Export thumbnail with retry (up to 3 attempts) - local thumbnailPath = nil + --- Export thumbnail with retry logic (up to 3 attempts, 500ms between failures). + ---@param photo LrPhoto Photo object from the catalog + ---@return string|nil Path on success, nil after all retries exhausted + local function exportWithRetries(photo) for attempt = 1, 3 do - thumbnailPath = thumbnailService.export(photo) - if thumbnailPath then - break + local path = thumbnailService.export(photo) + if path then + return path end activeLogger:warn("Thumbnail export attempt " .. attempt .. " failed, retrying...") tasks.sleep(0.5) -- Wait 500ms before retry end + return nil + end - if not thumbnailPath then - return nil, "Failed to export thumbnail after 3 attempts" - end - - -- 2. Encode image as Base64 + --- Execute the HTTP POST step: encode, build prompt, assemble body, send request. + --- Returns the raw HTTP response string (or nil on failure). Cleanup is NOT + --- performed here — it is the responsibility of `runWithCleanup`. + ---@param thumbnailPath string Path to exported JPEG + ---@param userInstruction string User-facing prompt text + ---@param currentData table|nil Existing title/caption + ---@param useCurrentData boolean Include current metadata in prompt + ---@param useSystemPrompt boolean Send system prompt + ---@param selectedModel string|nil Model name + ---@param systemPromptOverride string|nil Custom system prompt + ---@return string|nil rawResponse HTTP response body, or nil + ---@return string|nil err Error message + local function executePost(thumbnailPath, userInstruction, currentData, + useCurrentData, useSystemPrompt, selectedModel, + systemPromptOverride) + -- Encode image as Base64 local encodedImage = thumbnailService.encodeBase64(thumbnailPath) if not encodedImage then - thumbnailService.cleanup(thumbnailPath) return nil, "Failed to encode image" end - -- 3. Build user prompt and request body + -- Build user prompt and request body local builtPrompt = promptBuilder.buildUserPrompt(userInstruction, currentData, useCurrentData) local requestBody = promptBuilder.assembleRequestBody( builtPrompt, selectedModel or client.defaultModel, @@ -257,7 +307,7 @@ local function createClient(deps) ) requestBody.images = {encodedImage} - -- 4. Send HTTP POST + -- Send HTTP POST local url = client.getBaseUrl(prefsService.prefsForPlugin()) .. "/api/generate" local jsonPayload = json:encode(requestBody) @@ -266,15 +316,59 @@ local function createClient(deps) value = "application/json" }}) - -- 5. Clean up thumbnail file (always, regardless of success) - thumbnailService.cleanup(thumbnailPath) - - -- 6. Parse and validate response if not response then return nil, "Failed to send data to the API" end - return responseValidator.validateAndParse(response) + -- Return raw response for validation (after cleanup in caller). + return response, nil + end + + --- High-level generate: export thumbnail, encode, build prompt, POST, validate response. + --- Orchestrates ThumbnailService + PromptBuilder + ResponseValidator internally. + --- This is the decomposition of the former Common.sendDataToApi mega-function. + --- Retries thumbnail export up to 3 times. Cleans up the temp file after the + --- HTTP request regardless of success or failure (even when downstream code throws). + --- Validation runs after cleanup so the thumbnail is always removed first. + ---@param photo LrPhoto Photo object from the catalog + ---@param userInstruction string User-facing prompt text + ---@param currentData table|nil Existing title/caption to prepend when useCurrentData is true + ---@param useCurrentData boolean Whether to include current metadata in the prompt + ---@param useSystemPrompt boolean Whether to send a system prompt alongside the user prompt + ---@param selectedModel string|nil Model name; falls back to the default if nil + ---@param systemPromptOverride string|nil Override for the built-in system prompt + ---@return table|nil response Parsed JSON on success ({title, caption, keywords}) + ---@return string|nil err Error message, or nil on success + function client.generate(photo, userInstruction, currentData, useCurrentData, + useSystemPrompt, selectedModel, systemPromptOverride) + activeLogger:info("Sending data to API") + + -- Export thumbnail with retry; early return avoids cleanup when nothing created. + local thumbnailPath = exportWithRetries(photo) + if not thumbnailPath then + return nil, "Failed to export thumbnail after 3 attempts" + end + + -- Run the HTTP POST under exception-safe cleanup: thumbnail is deleted after + -- the request regardless of success or exception. + local rawResponse, err = runWithCleanup( + thumbnailPath, + function() + return executePost(thumbnailPath, userInstruction, currentData, + useCurrentData, useSystemPrompt, selectedModel, + systemPromptOverride) + end, + function(path) thumbnailService.cleanup(path) end, + activeLogger + ) + + -- If POST/cleanup failed, return immediately. + if not rawResponse then + return nil, err + end + + -- Validate after cleanup so the thumbnail is always removed first. + return responseValidator.validateAndParse(rawResponse) end return client From 5b1aa47c0df01f80010a987b588c20950132d9e5 Mon Sep 17 00:00:00 2001 From: Zack Stout Date: Fri, 17 Jul 2026 16:16:39 -0500 Subject: [PATCH 08/10] test: improve OllamaClient generate specs with event tracking and sandbox-safe patterns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add event recording to fakes for ordering assertions - Fix string.find typos (string.findtostring → string.find(tostring(...))) - Fix logger fake calling convention (colon notation matches production :error, :warn) - Expand test coverage: retry timing, error preservation, cleanup ordering --- tests/spec/ollama_client_generate_spec.lua | 847 +++++++++++++-------- 1 file changed, 512 insertions(+), 335 deletions(-) diff --git a/tests/spec/ollama_client_generate_spec.lua b/tests/spec/ollama_client_generate_spec.lua index b866ed1..3ce9aa1 100644 --- a/tests/spec/ollama_client_generate_spec.lua +++ b/tests/spec/ollama_client_generate_spec.lua @@ -1,425 +1,602 @@ --- ollama_client_generate_spec.lua — Unit tests for OllamaClient.generate() pipeline. ---- Covers success path, retry behavior, error propagation, and temp-file cleanup. +--- Covers success path, retry behavior, error propagation, exception-safe cleanup, +--- event ordering, and error-preservation policy. local path = PLUGIN_PATH -------------------------------------------------------------------------------- ---- Helper: build fakes aligned with how OllamaClient calls each dependency. ---- Dot-notation deps get no self; json uses colon so it does. +--- Helper: build fakes with event recording aligned to OllamaClient internals. +--- Dot-notation deps (http.post, thumbnailService.export) get no self. +--- Colon-notation deps (json.encode) receive self as first arg. -------------------------------------------------------------------------------- local function makeFakes() - local http = { - post_calls = {}, - post_response = nil, - } + local events = {} + + local http = { post_calls = {}, _response = nil } http.post = function(url, body, headers) + table.insert(events, "post") table.insert(http.post_calls, { url = url, body = body, headers = headers }) - return http.post_response + return http._response end local prefs = {} prefs.prefsForPlugin = function() - return { ollamaServerHost = prefs.ollamaServerHost or "localhost:11434" } + return { ollamaServerHost = "localhost:11434" } end local tasks = { sleep_calls = {} } - tasks.sleep = function(seconds) - table.insert(tasks.sleep_calls, seconds) + tasks.sleep = function(s) + table.insert(tasks.sleep_calls, s) end - -- JSON uses colon notation (json:encode / json:decode) — self is first arg. - local json = { - encode_calls = {}, - encode_return = nil, - } + -- JSON uses colon notation (json:encode) — self is first arg. + local json = { encode_calls = {}, _return = nil } json.encode = function(_, tbl) + table.insert(events, "json-encode") table.insert(json.encode_calls, tbl) - return json.encode_return or "{}" + return json._return or "{}" end - local thumbnailService = { - export_return = nil, - encodeBase64_return = nil, - cleanup_calls = {}, - export_calls = {}, - } - -- Dot-notation: no implicit self (OllamaClient calls thumbnailService.export(photo)) - thumbnailService.export = function(photo) - table.insert(thumbnailService.export_calls, photo) - return thumbnailService.export_return + -- thumbnailService uses dot notation — no implicit self. + local ts = { cleanup_calls = {}, export_calls = {}, _export_return = nil } + ts.export = function(photo) + table.insert(events, "export") + table.insert(ts.export_calls, photo) + return ts._export_return end - thumbnailService.encodeBase64 = function(imagePath) - return thumbnailService.encodeBase64_return + ts.encodeBase64 = function(imagePath) + table.insert(events, "encode") + return ts._encode_return end - thumbnailService.cleanup = function(imagePath) - table.insert(thumbnailService.cleanup_calls, imagePath) + ts.cleanup = function(imagePath) + table.insert(events, "cleanup") + table.insert(ts.cleanup_calls, imagePath) end - local promptBuilder = { + -- promptBuilder uses dot notation — no implicit self. + local pb = { buildUserPrompt_calls = {}, assembleRequestBody_calls = {}, - buildUserPrompt_return = "default prompt", - assembleRequestBody_return = { model = "test", prompt = "p" }, + _buildReturn = "default prompt", + _assembleReturn = { model = "test", prompt = "p" }, } - -- Dot-notation: no implicit self (OllamaClient calls promptBuilder.buildUserPrompt(...)) - promptBuilder.buildUserPrompt = function(instruction, currentData, useCurrent) - table.insert(promptBuilder.buildUserPrompt_calls, { + pb.buildUserPrompt = function(instruction, currentData, useCurrent) + table.insert(events, "build-prompt") + table.insert(pb.buildUserPrompt_calls, { instruction = instruction, currentData = currentData, - useCurrent = useCurrent + useCurrent = useCurrent, }) - return promptBuilder.buildUserPrompt_return + return pb._buildReturn end - promptBuilder.assembleRequestBody = function(userPrompt, model, useSys, override) - table.insert(promptBuilder.assembleRequestBody_calls, { + pb.assembleRequestBody = function(userPrompt, model, useSys, override) + table.insert(events, "assemble-body") + table.insert(pb.assembleRequestBody_calls, { userPrompt = userPrompt, model = model, useSystemPrompt = useSys, - systemPromptOverride = override + systemPromptOverride = override, }) - return promptBuilder.assembleRequestBody_return + return pb._assembleReturn end - local responseValidator = { - validateAndParse_calls = {}, - validateAndParse_metadata = nil, - validateAndParse_error = nil, - } - -- Dot-notation: no implicit self (OllamaClient calls responseValidator.validateAndParse(...)) - responseValidator.validateAndParse = function(rawResponse) - table.insert(responseValidator.validateAndParse_calls, rawResponse) - return responseValidator.validateAndParse_metadata, - responseValidator.validateAndParse_error + -- responseValidator uses dot notation. + local rv = { calls = {}, _metadata = nil, _error = nil } + rv.validateAndParse = function(rawResponse) + table.insert(events, "validate") + table.insert(rv.calls, rawResponse) + return rv._metadata, rv._error end - local logger = { - info_calls = {}, warn_calls = {}, error_calls = {}, - enable = function() end, - } - logger.info = function(msg) table.insert(logger.info_calls, msg) end - logger.warn = function(msg) table.insert(logger.warn_calls, msg) end - logger.error = function(msg) table.insert(logger.error_calls, msg) end + -- logger — colon notation for info/warn/error (OllamaClient calls :info, :warn, :error). + local logger = { info_calls = {}, warn_calls = {}, error_calls = {}, enable = function() end } + logger.info = function(_self, msg) table.insert(logger.info_calls, msg) end + logger.warn = function(_self, msg) table.insert(logger.warn_calls, msg) end + logger.error = function(_self, msg) table.insert(logger.error_calls, msg) end return { + events = events, http = http, prefs = prefs, tasks = tasks, json = json, - thumbnailService = thumbnailService, - promptBuilder = promptBuilder, - responseValidator = responseValidator, + thumbnailService = ts, + promptBuilder = pb, + responseValidator = rv, logger = logger, } end +-------------------------------------------------------------------------------- +--- Assert helpers +-------------------------------------------------------------------------------- +local function assertEventOrder(expected) + return function(f) + for i, name in ipairs(expected) do + assert.are_same(name, f.events[i], + string.format("Expected event[%d] = '%s', got '%s'", i, name, tostring(f.events[i]))) + end + end +end + -------------------------------------------------------------------------------- describe("OllamaClient — generate()", function() + local Client + local c + local f + -------------------------------------------------------------------- - -- Happy path - -------------------------------------------------------------------- - describe("success path", function() - it("orchestrates the full pipeline on first attempt", function() - local f = makeFakes() - f.thumbnailService.export_return = "/tmp/thumb.jpg" - f.thumbnailService.encodeBase64_return = "base64data" - f.http.post_response = '{"response":"{\"title\":\"T\",\"caption\":\"C\",\"keywords\":[\"k\"]}"}' - f.responseValidator.validateAndParse_metadata = { title = "T", caption = "C", keywords = { "k" } } - f.responseValidator.validateAndParse_error = nil - - local Client = assert(loadfile(path .. "OllamaClient.lua"))() - local c = Client.new(f) - - local result, err = c.generate( - { uuid = "photo-1" }, -- photo - "Caption this", -- userInstruction - nil, -- currentData - false, -- useCurrentData - true, -- useSystemPrompt - "gemma4:latest", -- selectedModel - nil -- systemPromptOverride - ) - - -- Thumbnail exported once - assert.are_same(1, #f.thumbnailService.export_calls) - assert.are_same("photo-1", f.thumbnailService.export_calls[1].uuid) - - -- Prompt built with correct args - assert.are_same(1, #f.promptBuilder.buildUserPrompt_calls) - local bpCall = f.promptBuilder.buildUserPrompt_calls[1] - assert.are_same("Caption this", bpCall.instruction) - assert.is_nil(bpCall.currentData) - assert.is_false(bpCall.useCurrent) - - -- Request body assembled - assert.are_same(1, #f.promptBuilder.assembleRequestBody_calls) - - -- Image attached to request body, then encoded + POSTed - assert.are_same(1, #f.json.encode_calls) - local encodedBody = f.json.encode_calls[1] - assert.is_true(type(encodedBody.images) == "table") - assert.are_same("base64data", encodedBody.images[1]) - - -- HTTP POST sent to correct URL - assert.are_same(1, #f.http.post_calls) - assert.are_same("http://localhost:11434/api/generate", f.http.post_calls[1].url) - - -- Cleanup always runs - assert.are_same(1, #f.thumbnailService.cleanup_calls) - assert.are_same("/tmp/thumb.jpg", f.thumbnailService.cleanup_calls[1]) - - -- ResponseValidator called with raw HTTP response - assert.are_same(1, #f.responseValidator.validateAndParse_calls) - - -- Result from validateAndParse is returned - assert.are_same("T", result.title) - assert.is_nil(err) - end) - - it("falls back to default model when selectedModel is nil", function() - local f = makeFakes() - f.thumbnailService.export_return = "/tmp/t.jpg" - f.thumbnailService.encodeBase64_return = "img" - f.http.post_response = '{}' - - local Client = assert(loadfile(path .. "OllamaClient.lua"))() - local c = Client.new(f) - - c.generate({ uuid = "p" }, "hi", nil, false, true, nil, nil) - - local rbCall = f.promptBuilder.assembleRequestBody_calls[1] - assert.are_same("gemma4:latest", rbCall.model) - end) - - it("passes systemPromptOverride through to assembleRequestBody", function() - local f = makeFakes() - f.thumbnailService.export_return = "/tmp/t.jpg" - f.thumbnailService.encodeBase64_return = "img" - f.http.post_response = '{}' - - local Client = assert(loadfile(path .. "OllamaClient.lua"))() - local c = Client.new(f) - - c.generate({ uuid = "p" }, "hi", nil, false, true, "gemma4:latest", "custom system") - - local rbCall = f.promptBuilder.assembleRequestBody_calls[1] - assert.are_same("custom system", rbCall.systemPromptOverride) - end) + -- 1. Happy path: full pipeline with event ordering + -------------------------------------------------------------------- + it("orchestrates the full pipeline on first attempt", function() + f = makeFakes() + f.thumbnailService._export_return = "/tmp/thumb.jpg" + f.thumbnailService._encode_return = "base64data" + f.http._response = '{"response":"{\"title\":\"T\",\"caption\":\"C\",\"keywords\":[\"k\"]}"}' + f.responseValidator._metadata = { title = "T", caption = "C", keywords = { "k" } } + + Client = assert(loadfile(path .. "OllamaClient.lua"))() + c = Client.new(f) + + local result, err = c.generate( + { uuid = "photo-1" }, + "Caption this", + nil, false, true, "gemma4:latest", nil + ) + + -- Event order: export → encode → build-prompt → assemble-body → json-encode + -- → post → cleanup → validate + assertEventOrder({ + "export", "encode", "build-prompt", "assemble-body", + "json-encode", "post", "cleanup", "validate" + })(f) + + -- Cleanup exactly once with correct path. + assert.are_same(1, #f.thumbnailService.cleanup_calls) + assert.are_same("/tmp/thumb.jpg", f.thumbnailService.cleanup_calls[1]) + + -- Result from validator returned unchanged. + assert.are_same("T", result.title) + assert.is_nil(err) end) -------------------------------------------------------------------- - -- Thumbnail export retry + -- 2. Encoding returns nil → cleanup, no POST/validate -------------------------------------------------------------------- - describe("thumbnail retry", function() - it("retries up to 3 times when export fails", function() - local f = makeFakes() - -- First two calls return nil, third succeeds - local attempt = 0 - f.thumbnailService.export = function(photo) - attempt = attempt + 1 - if attempt < 3 then - return nil - end - return "/tmp/t.jpg" - end - f.thumbnailService.encodeBase64_return = "img" - f.http.post_response = '{}' - - local Client = assert(loadfile(path .. "OllamaClient.lua"))() - local c = Client.new(f) - - c.generate({ uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil) - - assert.are_same(3, attempt) - -- Sleep called between attempts (2 sleeps for 3 attempts) - assert.are_same(2, #f.tasks.sleep_calls) - end) - - it("returns error after 3 failed exports", function() - local f = makeFakes() - -- Always return nil from export - f.thumbnailService.export = function() return nil end - - local Client = assert(loadfile(path .. "OllamaClient.lua"))() - local c = Client.new(f) - - local result, err = c.generate( - { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil - ) - - assert.is_nil(result) - assert.are_same("Failed to export thumbnail after 3 attempts", err) - end) - - it("sleeps between retry attempts", function() - local f = makeFakes() - local attempt = 0 - f.thumbnailService.export = function() - attempt = attempt + 1 - return nil - end - - local Client = assert(loadfile(path .. "OllamaClient.lua"))() - local c = Client.new(f) - - c.generate({ uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil) - - -- 3 attempts → 3 sleeps (sleep after each failed attempt including last) - assert.are_same(3, #f.tasks.sleep_calls) - assert.are_same(0.5, f.tasks.sleep_calls[1]) - end) + it("cleans up when encoding returns nil", function() + f = makeFakes() + f.thumbnailService._export_return = "/tmp/t.jpg" + f.thumbnailService._encode_return = nil + + Client = assert(loadfile(path .. "OllamaClient.lua"))() + c = Client.new(f) + + local result, err = c.generate( + { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil + ) + + assert.is_nil(result) + assert.are_same("Failed to encode image", err) + assert.are_same(1, #f.thumbnailService.cleanup_calls) + assert.are_same("/tmp/t.jpg", f.thumbnailService.cleanup_calls[1]) + assert.are_same(0, #f.http.post_calls) + assert.are_same(0, #f.responseValidator.calls) end) -------------------------------------------------------------------- - -- Base64 encoding failure + -- 3. Encoding throws → cleanup still runs -------------------------------------------------------------------- - describe("encoding failure", function() - it("returns error and cleans up when encode fails", function() - local f = makeFakes() - f.thumbnailService.export_return = "/tmp/t.jpg" - f.thumbnailService.encodeBase64_return = nil -- encoding failed - - local Client = assert(loadfile(path .. "OllamaClient.lua"))() - local c = Client.new(f) - - local result, err = c.generate( - { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil - ) - - assert.is_nil(result) - assert.are_same("Failed to encode image", err) - -- Cleanup happens even on failure - assert.are_same(1, #f.thumbnailService.cleanup_calls) - assert.are_same("/tmp/t.jpg", f.thumbnailService.cleanup_calls[1]) - end) + it("cleans up when encoding throws", function() + f = makeFakes() + f.thumbnailService._export_return = "/tmp/t.jpg" + f.thumbnailService.encodeBase64 = function(_) + table.insert(f.events, "encode") + error("encode exploded") + end + + Client = assert(loadfile(path .. "OllamaClient.lua"))() + c = Client.new(f) + + local result, err = c.generate( + { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil + ) + + assert.is_nil(result) + assert.is_not_nil(err) + assert.is_not_nil(string.find(tostring(err), "encode exploded", 1, true)) + -- Cleanup despite exception. + assert.are_same(1, #f.thumbnailService.cleanup_calls) + assert.are_same("/tmp/t.jpg", f.thumbnailService.cleanup_calls[1]) + assert.are_same(0, #f.http.post_calls) end) -------------------------------------------------------------------- - -- HTTP POST failure + -- 4. Prompt builder throws → cleanup still runs -------------------------------------------------------------------- - describe("HTTP failure", function() - it("returns error when http.post returns nil", function() - local f = makeFakes() - f.thumbnailService.export_return = "/tmp/t.jpg" - f.thumbnailService.encodeBase64_return = "img" - f.http.post_response = nil -- network error - - local Client = assert(loadfile(path .. "OllamaClient.lua"))() - local c = Client.new(f) - - local result, err = c.generate( - { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil - ) - - assert.is_nil(result) - assert.are_same("Failed to send data to the API", err) - -- Cleanup still runs - assert.are_same(1, #f.thumbnailService.cleanup_calls) - end) + it("cleans up when prompt builder throws", function() + f = makeFakes() + f.thumbnailService._export_return = "/tmp/t.jpg" + f.thumbnailService._encode_return = "img" + f.promptBuilder.buildUserPrompt = function() + table.insert(f.events, "build-prompt") + error("prompt builder error") + end + + Client = assert(loadfile(path .. "OllamaClient.lua"))() + c = Client.new(f) + + local result, err = c.generate( + { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil + ) + + assert.is_nil(result) + assert.is_not_nil(string.find(tostring(err), "prompt builder error", 1, true)) + assert.are_same(1, #f.thumbnailService.cleanup_calls) + assert.are_same("/tmp/t.jpg", f.thumbnailService.cleanup_calls[1]) end) -------------------------------------------------------------------- - -- Response validation delegation + -- 5. Request-body assembly throws → cleanup still runs -------------------------------------------------------------------- - describe("response validation", function() - it("passes raw HTTP response to validateAndParse", function() - local f = makeFakes() - f.thumbnailService.export_return = "/tmp/t.jpg" - f.thumbnailService.encodeBase64_return = "img" - f.http.post_response = '{"response":"raw inner"}' - - local Client = assert(loadfile(path .. "OllamaClient.lua"))() - local c = Client.new(f) - - c.generate({ uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil) + it("cleans up when request-body assembly throws", function() + f = makeFakes() + f.thumbnailService._export_return = "/tmp/t.jpg" + f.thumbnailService._encode_return = "img" + f.promptBuilder.assembleRequestBody = function() + table.insert(f.events, "assemble-body") + error("assembly error") + end + + Client = assert(loadfile(path .. "OllamaClient.lua"))() + c = Client.new(f) + + local result, err = c.generate( + { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil + ) + + assert.is_nil(result) + assert.is_not_nil(string.find(tostring(err), "assembly error", 1, true)) + assert.are_same(1, #f.thumbnailService.cleanup_calls) + assert.are_same("/tmp/t.jpg", f.thumbnailService.cleanup_calls[1]) + end) - assert.are_same(1, #f.responseValidator.validateAndParse_calls) - assert.are_same('{"response":"raw inner"}', f.responseValidator.validateAndParse_calls[1]) - end) + -------------------------------------------------------------------- + -- 6. JSON encoding throws → cleanup still runs + -------------------------------------------------------------------- + it("cleans up when JSON encoding throws", function() + f = makeFakes() + f.thumbnailService._export_return = "/tmp/t.jpg" + f.thumbnailService._encode_return = "img" + f.json.encode = function(_) + table.insert(f.events, "json-encode") + error("json encoding failed") + end + + Client = assert(loadfile(path .. "OllamaClient.lua"))() + c = Client.new(f) + + local result, err = c.generate( + { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil + ) + + assert.is_nil(result) + assert.is_not_nil(string.find(tostring(err), "json encoding failed", 1, true)) + assert.are_same(1, #f.thumbnailService.cleanup_calls) + assert.are_same("/tmp/t.jpg", f.thumbnailService.cleanup_calls[1]) + assert.are_same(0, #f.http.post_calls) + end) - it("returns validation error when response is invalid", function() - local f = makeFakes() - f.thumbnailService.export_return = "/tmp/t.jpg" - f.thumbnailService.encodeBase64_return = "img" - f.http.post_response = 'invalid json' - f.responseValidator.validateAndParse_metadata = nil - f.responseValidator.validateAndParse_error = "Bad response" + -------------------------------------------------------------------- + -- 7. HTTP POST returns nil → cleanup before error return + -------------------------------------------------------------------- + it("cleans up when http.post returns nil", function() + f = makeFakes() + f.thumbnailService._export_return = "/tmp/t.jpg" + f.thumbnailService._encode_return = "img" + f.http._response = nil + + Client = assert(loadfile(path .. "OllamaClient.lua"))() + c = Client.new(f) + + local result, err = c.generate( + { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil + ) + + assert.is_nil(result) + assert.are_same("Failed to send data to the API", err) + -- Cleanup runs before error return. + assertEventOrder({ + "export", "encode", "build-prompt", "assemble-body", + "json-encode", "post", "cleanup" + })(f) + assert.are_same(1, #f.thumbnailService.cleanup_calls) + end) - local Client = assert(loadfile(path .. "OllamaClient.lua"))() - local c = Client.new(f) + -------------------------------------------------------------------- + -- 8. HTTP POST throws → cleanup still runs + -------------------------------------------------------------------- + it("cleans up when http.post throws", function() + f = makeFakes() + f.thumbnailService._export_return = "/tmp/t.jpg" + f.thumbnailService._encode_return = "img" + f.http.post = function(_, _, _) + table.insert(f.events, "post") + error("post exploded") + end + + Client = assert(loadfile(path .. "OllamaClient.lua"))() + c = Client.new(f) + + local result, err = c.generate( + { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil + ) + + assert.is_nil(result) + assert.is_not_nil(string.find(tostring(err), "post exploded", 1, true)) + assert.are_same(1, #f.thumbnailService.cleanup_calls) + assert.are_same("/tmp/t.jpg", f.thumbnailService.cleanup_calls[1]) + end) - local result, err = c.generate( - { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil - ) + -------------------------------------------------------------------- + -- 9. Validator returns error → cleanup happened before validation + -------------------------------------------------------------------- + it("cleans up before validator and returns validation error", function() + f = makeFakes() + f.thumbnailService._export_return = "/tmp/t.jpg" + f.thumbnailService._encode_return = "img" + f.http._response = '{"response":"raw inner"}' + f.responseValidator._metadata = nil + f.responseValidator._error = "invalid metadata" + + Client = assert(loadfile(path .. "OllamaClient.lua"))() + c = Client.new(f) + + local result, err = c.generate( + { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil + ) + + assert.is_nil(result) + assert.are_same("invalid metadata", err) + -- Cleanup before validate in event order. + local cleanupIdx = nil + local validateIdx = nil + for i, e in ipairs(f.events) do + if e == "cleanup" then cleanupIdx = i end + if e == "validate" then validateIdx = i end + end + assert.is_not_nil(cleanupIdx) + assert.is_not_nil(validateIdx) + assert.are_true(cleanupIdx < validateIdx, + "Cleanup must occur before validation") + end) - assert.is_nil(result) - assert.are_same("Bad response", err) - end) + -------------------------------------------------------------------- + -- 10. Validator throws → cleanup has occurred already (validation + -- is outside runWithCleanup, so exception propagates after cleanup). + -------------------------------------------------------------------- + it("cleans up before validator even if validator throws", function() + f = makeFakes() + f.thumbnailService._export_return = "/tmp/t.jpg" + f.thumbnailService._encode_return = "img" + f.http._response = '{"response":"raw"}' + f.responseValidator.validateAndParse = function(_) + table.insert(f.events, "validate") + error("validation exploded") + end + + Client = assert(loadfile(path .. "OllamaClient.lua"))() + c = Client.new(f) + + -- Validation is outside runWithCleanup, so a thrown exception + -- propagates. Cleanup already happened inside runWithCleanup. + local ok, err = pcall(c.generate, c, + { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil + ) + + assert.is_false(ok) + assert.is_not_nil(string.find(tostring(err), "validation exploded", 1, true)) + -- Cleanup ran before validation was even attempted. + assert.are_same(1, #f.thumbnailService.cleanup_calls) end) -------------------------------------------------------------------- - -- Cleanup verification + -- 11. Cleanup fails after successful generation → no false success -------------------------------------------------------------------- - describe("cleanup", function() - it("cleans up thumbnail even when HTTP fails", function() - local f = makeFakes() - f.thumbnailService.export_return = "/tmp/x.jpg" - f.thumbnailService.encodeBase64_return = "img" - f.http.post_response = nil + it("returns cleanup error when cleanup fails after generation success", function() + f = makeFakes() + f.thumbnailService._export_return = "/tmp/t.jpg" + f.thumbnailService._encode_return = "img" + f.http._response = '{"response":"{\"title\":\"T\",\"caption\":\"C\",\"keywords\":[\"k\"]}"}' + f.responseValidator._metadata = { title = "T", caption = "C", keywords = { "k" } } + -- Make cleanup throw. + f.thumbnailService.cleanup = function(_) + table.insert(f.events, "cleanup") + error("disk full") + end + + Client = assert(loadfile(path .. "OllamaClient.lua"))() + c = Client.new(f) + + local result, err = c.generate( + { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil + ) + + assert.is_nil(result) + assert.is_not_nil(string.find(tostring(err), "Failed to clean up thumbnail", 1, true)) + assert.is_not_nil(string.find(tostring(err), "disk full", 1, true)) + end) - local Client = assert(loadfile(path .. "OllamaClient.lua"))() - local c = Client.new(f) + -------------------------------------------------------------------- + -- 12. Cleanup fails AND generation failed → original error preserved + -------------------------------------------------------------------- + it("preserves generation error when both generation and cleanup fail", function() + f = makeFakes() + f.thumbnailService._export_return = "/tmp/t.jpg" + f.thumbnailService._encode_return = "img" + -- POST throws (generation fails) + f.http.post = function(_, _, _) + table.insert(f.events, "post") + error("network timeout") + end + -- Cleanup also throws. + f.thumbnailService.cleanup = function(_) + table.insert(f.events, "cleanup") + error("disk full") + end + + Client = assert(loadfile(path .. "OllamaClient.lua"))() + c = Client.new(f) + + local result, err = c.generate( + { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil + ) + + assert.is_nil(result) + -- Original generation error is preserved. + assert.is_not_nil(string.find(tostring(err), "network timeout", 1, true), + "Expected original generation error in result") + -- Cleanup failure logged via logger:error (not surfaced). + assert.are_same(1, #f.logger.error_calls) + assert.is_not_nil(string.find(f.logger.error_calls[1], "disk full", 1, true)) + end) - c.generate({ uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil) + -------------------------------------------------------------------- + -- 13. Export fails all 3 attempts → no cleanup + -------------------------------------------------------------------- + it("does not clean up when export fails all attempts", function() + f = makeFakes() + f.thumbnailService.export = function(photo) + table.insert(f.events, "export") + table.insert(f.thumbnailService.export_calls, photo) + return nil + end + + Client = assert(loadfile(path .. "OllamaClient.lua"))() + c = Client.new(f) + + local result, err = c.generate( + { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil + ) + + assert.is_nil(result) + assert.are_same("Failed to export thumbnail after 3 attempts", err) + -- Export called 3 times. + assert.are_same(3, #f.thumbnailService.export_calls) + -- No cleanup (nothing to clean up). + assert.are_same(0, #f.thumbnailService.cleanup_calls) + end) - assert.are_same(1, #f.thumbnailService.cleanup_calls) - assert.are_same("/tmp/x.jpg", f.thumbnailService.cleanup_calls[1]) - end) + -------------------------------------------------------------------- + -- 14. Export succeeds on second attempt → cleanup runs once + -------------------------------------------------------------------- + it("cleans up when export succeeds on second attempt", function() + f = makeFakes() + local attempt = 0 + f.thumbnailService.export = function(photo) + table.insert(f.events, "export") + table.insert(f.thumbnailService.export_calls, photo) + attempt = attempt + 1 + if attempt < 2 then return nil end + return "/tmp/t.jpg" + end + f.thumbnailService._encode_return = "img" + f.http._response = '{}' + + Client = assert(loadfile(path .. "OllamaClient.lua"))() + c = Client.new(f) + + local result, err = c.generate( + { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil + ) + + -- Export called twice. + assert.are_same(2, #f.thumbnailService.export_calls) + -- One sleep between attempts. + assert.are_same(1, #f.tasks.sleep_calls) + -- Cleanup runs once with correct path. + assert.are_same(1, #f.thumbnailService.cleanup_calls) + assert.are_same("/tmp/t.jpg", f.thumbnailService.cleanup_calls[1]) + end) - it("cleans up thumbnail even when validation fails", function() - local f = makeFakes() - f.thumbnailService.export_return = "/tmp/y.jpg" - f.thumbnailService.encodeBase64_return = "img" - f.http.post_response = 'bad' - f.responseValidator.validateAndParse_metadata = nil - f.responseValidator.validateAndParse_error = "validation failed" + -------------------------------------------------------------------- + -- 15. Export succeeds on third attempt → cleanup runs once + -------------------------------------------------------------------- + it("cleans up when export succeeds on third attempt", function() + f = makeFakes() + local attempt = 0 + f.thumbnailService.export = function(photo) + table.insert(f.events, "export") + table.insert(f.thumbnailService.export_calls, photo) + attempt = attempt + 1 + if attempt < 3 then return nil end + return "/tmp/t.jpg" + end + f.thumbnailService._encode_return = "img" + f.http._response = '{}' + + Client = assert(loadfile(path .. "OllamaClient.lua"))() + c = Client.new(f) + + local result, err = c.generate( + { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil + ) + + -- Export called three times. + assert.are_same(3, #f.thumbnailService.export_calls) + -- Two sleeps between attempts. + assert.are_same(2, #f.tasks.sleep_calls) + -- Cleanup runs once. + assert.are_same(1, #f.thumbnailService.cleanup_calls) + assert.are_same("/tmp/t.jpg", f.thumbnailService.cleanup_calls[1]) + end) - local Client = assert(loadfile(path .. "OllamaClient.lua"))() - local c = Client.new(f) + -------------------------------------------------------------------- + -- 16. No double cleanup — explicit assertion across scenarios + -------------------------------------------------------------------- + it("never calls cleanup more than once in any post-export scenario", function() + -- This meta-test re-runs several configurations and asserts + -- cleanup count == 1 for every scenario where export succeeded. + local scenarios = { + -- Name, setup function + { + "success", + function(fake) + fake.thumbnailService._encode_return = "img" + fake.http._response = '{"response":"{\"title\":\"T\",\"caption\":\"C\",\"keywords\":[\"k\"]}"}' + fake.responseValidator._metadata = { title = "T" } + end, + }, + { + "encode-nil", + function(fake) + fake.thumbnailService._encode_return = nil + end, + }, + { + "http-nil", + function(fake) + fake.thumbnailService._encode_return = "img" + fake.http._response = nil + end, + }, + } + + for _, scenario in ipairs(scenarios) do + local name, setup = scenario[1], scenario[2] + f = makeFakes() + f.thumbnailService._export_return = "/tmp/t.jpg" + setup(f) + + Client = assert(loadfile(path .. "OllamaClient.lua"))() + c = Client.new(f) c.generate({ uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil) - assert.are_same(1, #f.thumbnailService.cleanup_calls) - assert.are_same("/tmp/y.jpg", f.thumbnailService.cleanup_calls[1]) - end) - end) - - -------------------------------------------------------------------- - -- Prompt construction - -------------------------------------------------------------------- - describe("prompt construction", function() - it("builds prompt with current data when useCurrentData is true", function() - local f = makeFakes() - f.thumbnailService.export_return = "/tmp/t.jpg" - f.thumbnailService.encodeBase64_return = "img" - f.http.post_response = '{}' - - local Client = assert(loadfile(path .. "OllamaClient.lua"))() - local c = Client.new(f) - - c.generate( - { uuid = "p" }, - "Caption this", - { title = "Old Title", caption = "Old Caption" }, - true, -- useCurrentData - false, - "gemma4:latest", - nil - ) - - local bpCall = f.promptBuilder.buildUserPrompt_calls[1] - assert.is_true(bpCall.useCurrent) - assert.are_same("Old Title", bpCall.currentData.title) - end) + assert.are_same( + 1, #f.thumbnailService.cleanup_calls, + string.format("[%s] cleanup called %d times (expected 1)", + name, #f.thumbnailService.cleanup_calls)) + end end) end) From 81d29f1ba3c2f2646a43f263b0dcb45a05c11862 Mon Sep 17 00:00:00 2001 From: Zack Stout Date: Fri, 17 Jul 2026 16:28:05 -0500 Subject: [PATCH 09/10] fix: remove pcall from runWithCleanup to allow SDK functions to yield Lightroom's LrTasks.startAsyncTask wraps task callbacks in protected calls. Nesting our own pcall inside that context prevented SDK functions (http.post, encodeBase64, etc.) from yielding to the Lightroom event loop, causing 'Yielding is not allowed within a C or metamethod call' errors. SDK functions return nil on error rather than throwing Lua exceptions, so the pcall was only catching theoretical user-code exceptions at the cost of breaking normal operation. Exceptions now propagate naturally to Lightroom's own async task error handler. - Simplified runWithCleanup to sequential operation + cleanup (no pcall) - Removed unused pack/unpack helpers - Removed activeLogger parameter (no longer needed without dual-failure logging) - Updated tests to expect exception propagation via pcall at test level --- lightroom-llama.lrplugin/OllamaClient.lua | 64 ++++----------- tests/spec/ollama_client_generate_spec.lua | 94 ++++++++++------------ 2 files changed, 56 insertions(+), 102 deletions(-) diff --git a/lightroom-llama.lrplugin/OllamaClient.lua b/lightroom-llama.lrplugin/OllamaClient.lua index 3030168..cef37fa 100644 --- a/lightroom-llama.lrplugin/OllamaClient.lua +++ b/lightroom-llama.lrplugin/OllamaClient.lua @@ -23,56 +23,21 @@ local ResponseValidator = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "Resp --- Helpers for exception-safe cleanup -------------------------------------------------------------------------------- ---- Pack variadic returns preserving nil positions (Lua 5.1 compatible). ----@return table results Table with `.n` count and packed values -local function pack(...) - return { n = select("#", ...), ... } -end - --- Lua 5.1: unpack is a global; Lua 5.2+: it moved to table.unpack. -local unpack = unpack or table.unpack - ---- Run *operation* under pcall, then always attempt *cleanupFn*. ---- Guarantees the temp thumbnail is deleted regardless of how the ---- generation pipeline exits (success, returned failure, or exception). ---- ---- Error preservation: ---- - Generation fails + cleanup succeeds → return original error ---- - Generation fails + cleanup fails → return original error; log cleanup ---- - Generation succeeds + cleanup fails → return cleanup error (no false success) ---- The `results` table is filled by `pack()` inside pcall so multiple return ---- values from the operation survive the protected call. +--- Run *operation*, then always attempt *cleanupFn*. +--- Does NOT use pcall — Lightroom's LrTasks.startAsyncTask already runs +--- in a protected context, and nesting pcall inside that context prevents +--- SDK functions (http.post, encodeBase64, etc.) from yielding to the +--- Lightroom event loop, causing "Yielding is not allowed" errors. +--- Cleanup always runs after the operation so the temp thumbnail is removed +--- before validation or error propagation. ---@param thumbnailPath string Path to delete after operation ----@param operation function Generation logic returning zero or more values +---@param operation function Generation logic returning (result, error) ---@param cleanupFn function Cleanup function accepting the path ----@param activeLogger table Logger for dual-failure diagnostics ----@return ... The return values of *operation* on success, or `nil, error` on failure -local function runWithCleanup(thumbnailPath, operation, cleanupFn, activeLogger) - local results = { n = 0 } - - local ok, errInfo = pcall(function() - results = pack(operation()) - end) - - -- Always attempt cleanup regardless of operation outcome. - local cleanupOk, cleanupErr = pcall(cleanupFn, thumbnailPath) - - if not ok then - -- Generation threw an exception. - if not cleanupOk then - activeLogger:error( - "Thumbnail cleanup also failed: " .. tostring(cleanupErr) - ) - end - return nil, errInfo - end - - -- Generation succeeded but cleanup failed — do not report success. - if not cleanupOk then - return nil, "Failed to clean up thumbnail: " .. tostring(cleanupErr) - end - - return unpack(results, 1, results.n) +---@return ... The return values of *operation* +local function runWithCleanup(thumbnailPath, operation, cleanupFn) + local result, err = operation() + cleanupFn(thumbnailPath) + return result, err end -------------------------------------------------------------------------------- @@ -358,8 +323,7 @@ local function createClient(deps) useCurrentData, useSystemPrompt, selectedModel, systemPromptOverride) end, - function(path) thumbnailService.cleanup(path) end, - activeLogger + function(path) thumbnailService.cleanup(path) end ) -- If POST/cleanup failed, return immediately. diff --git a/tests/spec/ollama_client_generate_spec.lua b/tests/spec/ollama_client_generate_spec.lua index 3ce9aa1..2888d57 100644 --- a/tests/spec/ollama_client_generate_spec.lua +++ b/tests/spec/ollama_client_generate_spec.lua @@ -185,9 +185,13 @@ describe("OllamaClient — generate()", function() end) -------------------------------------------------------------------- - -- 3. Encoding throws → cleanup still runs + -- 3. Encoding throws → exception propagates (no pcall in Lightroom). + -- In production, SDK functions return nil on error rather than + -- throwing, so this path is defensive-only. Tests catch with + -- pcall at the test level since cleanup cannot run when an + -- intermediate step throws without protected calls. -------------------------------------------------------------------- - it("cleans up when encoding throws", function() + it("propagates exceptions from encoding", function() f = makeFakes() f.thumbnailService._export_return = "/tmp/t.jpg" f.thumbnailService.encodeBase64 = function(_) @@ -198,23 +202,21 @@ describe("OllamaClient — generate()", function() Client = assert(loadfile(path .. "OllamaClient.lua"))() c = Client.new(f) - local result, err = c.generate( + local ok, err = pcall(c.generate, c, { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil ) - assert.is_nil(result) - assert.is_not_nil(err) + assert.is_false(ok) assert.is_not_nil(string.find(tostring(err), "encode exploded", 1, true)) - -- Cleanup despite exception. - assert.are_same(1, #f.thumbnailService.cleanup_calls) - assert.are_same("/tmp/t.jpg", f.thumbnailService.cleanup_calls[1]) + -- Note: cleanup cannot run when intermediate steps throw + -- without pcall (which Lightroom's sandbox doesn't support). assert.are_same(0, #f.http.post_calls) end) -------------------------------------------------------------------- - -- 4. Prompt builder throws → cleanup still runs + -- 4. Prompt builder throws → same propagation behavior. -------------------------------------------------------------------- - it("cleans up when prompt builder throws", function() + it("propagates exceptions from prompt builder", function() f = makeFakes() f.thumbnailService._export_return = "/tmp/t.jpg" f.thumbnailService._encode_return = "img" @@ -226,20 +228,18 @@ describe("OllamaClient — generate()", function() Client = assert(loadfile(path .. "OllamaClient.lua"))() c = Client.new(f) - local result, err = c.generate( + local ok, err = pcall(c.generate, c, { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil ) - assert.is_nil(result) + assert.is_false(ok) assert.is_not_nil(string.find(tostring(err), "prompt builder error", 1, true)) - assert.are_same(1, #f.thumbnailService.cleanup_calls) - assert.are_same("/tmp/t.jpg", f.thumbnailService.cleanup_calls[1]) end) -------------------------------------------------------------------- - -- 5. Request-body assembly throws → cleanup still runs + -- 5. Request-body assembly throws → same propagation behavior. -------------------------------------------------------------------- - it("cleans up when request-body assembly throws", function() + it("propagates exceptions from request-body assembly", function() f = makeFakes() f.thumbnailService._export_return = "/tmp/t.jpg" f.thumbnailService._encode_return = "img" @@ -251,20 +251,18 @@ describe("OllamaClient — generate()", function() Client = assert(loadfile(path .. "OllamaClient.lua"))() c = Client.new(f) - local result, err = c.generate( + local ok, err = pcall(c.generate, c, { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil ) - assert.is_nil(result) + assert.is_false(ok) assert.is_not_nil(string.find(tostring(err), "assembly error", 1, true)) - assert.are_same(1, #f.thumbnailService.cleanup_calls) - assert.are_same("/tmp/t.jpg", f.thumbnailService.cleanup_calls[1]) end) -------------------------------------------------------------------- - -- 6. JSON encoding throws → cleanup still runs + -- 6. JSON encoding throws → same propagation behavior. -------------------------------------------------------------------- - it("cleans up when JSON encoding throws", function() + it("propagates exceptions from JSON encoding", function() f = makeFakes() f.thumbnailService._export_return = "/tmp/t.jpg" f.thumbnailService._encode_return = "img" @@ -276,14 +274,12 @@ describe("OllamaClient — generate()", function() Client = assert(loadfile(path .. "OllamaClient.lua"))() c = Client.new(f) - local result, err = c.generate( + local ok, err = pcall(c.generate, c, { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil ) - assert.is_nil(result) + assert.is_false(ok) assert.is_not_nil(string.find(tostring(err), "json encoding failed", 1, true)) - assert.are_same(1, #f.thumbnailService.cleanup_calls) - assert.are_same("/tmp/t.jpg", f.thumbnailService.cleanup_calls[1]) assert.are_same(0, #f.http.post_calls) end) @@ -314,9 +310,9 @@ describe("OllamaClient — generate()", function() end) -------------------------------------------------------------------- - -- 8. HTTP POST throws → cleanup still runs + -- 8. HTTP POST throws → exception propagates (same as above). -------------------------------------------------------------------- - it("cleans up when http.post throws", function() + it("propagates exceptions from http.post", function() f = makeFakes() f.thumbnailService._export_return = "/tmp/t.jpg" f.thumbnailService._encode_return = "img" @@ -328,14 +324,12 @@ describe("OllamaClient — generate()", function() Client = assert(loadfile(path .. "OllamaClient.lua"))() c = Client.new(f) - local result, err = c.generate( + local ok, err = pcall(c.generate, c, { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil ) - assert.is_nil(result) + assert.is_false(ok) assert.is_not_nil(string.find(tostring(err), "post exploded", 1, true)) - assert.are_same(1, #f.thumbnailService.cleanup_calls) - assert.are_same("/tmp/t.jpg", f.thumbnailService.cleanup_calls[1]) end) -------------------------------------------------------------------- @@ -401,9 +395,12 @@ describe("OllamaClient — generate()", function() end) -------------------------------------------------------------------- - -- 11. Cleanup fails after successful generation → no false success + -- 11. Cleanup fails after successful generation → exception propagates. + -- Without pcall, a cleanup failure is an exception that bubbles up. + -- In production, LrFileUtils.delete (the cleanup implementation) + -- returns nil on error rather than throwing, so this is defensive. -------------------------------------------------------------------- - it("returns cleanup error when cleanup fails after generation success", function() + it("propagates exceptions when cleanup fails after generation", function() f = makeFakes() f.thumbnailService._export_return = "/tmp/t.jpg" f.thumbnailService._encode_return = "img" @@ -418,47 +415,40 @@ describe("OllamaClient — generate()", function() Client = assert(loadfile(path .. "OllamaClient.lua"))() c = Client.new(f) - local result, err = c.generate( + local ok, err = pcall(c.generate, c, { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil ) - assert.is_nil(result) - assert.is_not_nil(string.find(tostring(err), "Failed to clean up thumbnail", 1, true)) + assert.is_false(ok) assert.is_not_nil(string.find(tostring(err), "disk full", 1, true)) end) -------------------------------------------------------------------- - -- 12. Cleanup fails AND generation failed → original error preserved + -- 12. Cleanup fails AND generation failed → first exception wins. + -- Without pcall, when generation throws, cleanup never runs, so + -- the generation error is what propagates (cleanup is never reached). -------------------------------------------------------------------- - it("preserves generation error when both generation and cleanup fail", function() + it("propagates generation error when generation throws", function() f = makeFakes() f.thumbnailService._export_return = "/tmp/t.jpg" f.thumbnailService._encode_return = "img" - -- POST throws (generation fails) + -- POST throws (generation fails before cleanup is reached) f.http.post = function(_, _, _) table.insert(f.events, "post") error("network timeout") end - -- Cleanup also throws. - f.thumbnailService.cleanup = function(_) - table.insert(f.events, "cleanup") - error("disk full") - end Client = assert(loadfile(path .. "OllamaClient.lua"))() c = Client.new(f) - local result, err = c.generate( + local ok, err = pcall(c.generate, c, { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil ) - assert.is_nil(result) - -- Original generation error is preserved. + assert.is_false(ok) + -- Generation error propagates; cleanup is never reached. assert.is_not_nil(string.find(tostring(err), "network timeout", 1, true), - "Expected original generation error in result") - -- Cleanup failure logged via logger:error (not surfaced). - assert.are_same(1, #f.logger.error_calls) - assert.is_not_nil(string.find(f.logger.error_calls[1], "disk full", 1, true)) + "Expected original generation error to propagate") end) -------------------------------------------------------------------- From b8e9e5727021658f9ccf15eff4a8d16bea9fd1a1 Mon Sep 17 00:00:00 2001 From: Zack Stout Date: Fri, 17 Jul 2026 21:18:25 -0500 Subject: [PATCH 10/10] refactor: replace pcall-wrapped runWithCleanup with explicit cleanup steps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wrapping SDK calls (encodeBase64, http.post) in pcall breaks when those functions yield internally — Lightroom's non-blocking I/O crosses the protected boundary and pcall returns early, silently swallowing real results. Tests passed because mocks don't yield. Instead, inline explicit cleanup at every exit point: - SDK calls run unwrapped; nil-return paths trigger best-effort cleanup via tryCleanup() helper (pcall-wrapped LrFileUtils.delete, which is local disk I/O and safe to protect) - Pure-Lua steps (prompt builder, JSON encode) remain pcall-safe since they cannot yield - Error preservation: operation error always primary on failure paths; cleanup error becomes primary only on the success path Trade-off: if an SDK function genuinely throws (not yields or returns nil), cleanup is skipped. This is a true SDK crash — far better than breaking the happy path for every generate call. Changes: - Remove pack/unpack/runWithCleanup (~50 lines) - Add tryCleanup helper for error-path cleanup - Inline executePost into generate() with explicit cleanup at each step - Update tests 3 and 8 to expect SDK exception propagation - Replace encode-throw/http-post-throw in meta-test with prompt-throw/ json-throw (pure-Lua exceptions that DO trigger cleanup) --- lightroom-llama.lrplugin/OllamaClient.lua | 155 +++++----- tests/spec/ollama_client_generate_spec.lua | 138 ++++++--- tests/spec/thumbnail_service_spec.lua | 335 +++++++++++++++++++++ 3 files changed, 499 insertions(+), 129 deletions(-) create mode 100644 tests/spec/thumbnail_service_spec.lua diff --git a/lightroom-llama.lrplugin/OllamaClient.lua b/lightroom-llama.lrplugin/OllamaClient.lua index cef37fa..021a911 100644 --- a/lightroom-llama.lrplugin/OllamaClient.lua +++ b/lightroom-llama.lrplugin/OllamaClient.lua @@ -19,27 +19,6 @@ local ThumbnailService = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "Thumb local PromptBuilder = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "PromptBuilder.lua"))))() local ResponseValidator = (assert(loadfile(LrPathUtils.child(_PLUGIN.path, "ResponseValidator.lua"))))() --------------------------------------------------------------------------------- ---- Helpers for exception-safe cleanup --------------------------------------------------------------------------------- - ---- Run *operation*, then always attempt *cleanupFn*. ---- Does NOT use pcall — Lightroom's LrTasks.startAsyncTask already runs ---- in a protected context, and nesting pcall inside that context prevents ---- SDK functions (http.post, encodeBase64, etc.) from yielding to the ---- Lightroom event loop, causing "Yielding is not allowed" errors. ---- Cleanup always runs after the operation so the temp thumbnail is removed ---- before validation or error propagation. ----@param thumbnailPath string Path to delete after operation ----@param operation function Generation logic returning (result, error) ----@param cleanupFn function Cleanup function accepting the path ----@return ... The return values of *operation* -local function runWithCleanup(thumbnailPath, operation, cleanupFn) - local result, err = operation() - cleanupFn(thumbnailPath) - return result, err -end - -------------------------------------------------------------------------------- --- Constructor — create a client with injectable dependencies. --- callers need no changes while tests can provide lightweight fakes. @@ -243,58 +222,37 @@ local function createClient(deps) return nil end - --- Execute the HTTP POST step: encode, build prompt, assemble body, send request. - --- Returns the raw HTTP response string (or nil on failure). Cleanup is NOT - --- performed here — it is the responsibility of `runWithCleanup`. - ---@param thumbnailPath string Path to exported JPEG - ---@param userInstruction string User-facing prompt text - ---@param currentData table|nil Existing title/caption - ---@param useCurrentData boolean Include current metadata in prompt - ---@param useSystemPrompt boolean Send system prompt - ---@param selectedModel string|nil Model name - ---@param systemPromptOverride string|nil Custom system prompt - ---@return string|nil rawResponse HTTP response body, or nil - ---@return string|nil err Error message - local function executePost(thumbnailPath, userInstruction, currentData, - useCurrentData, useSystemPrompt, selectedModel, - systemPromptOverride) - -- Encode image as Base64 - local encodedImage = thumbnailService.encodeBase64(thumbnailPath) - if not encodedImage then - return nil, "Failed to encode image" - end - - -- Build user prompt and request body - local builtPrompt = promptBuilder.buildUserPrompt(userInstruction, currentData, useCurrentData) - local requestBody = promptBuilder.assembleRequestBody( - builtPrompt, selectedModel or client.defaultModel, - useSystemPrompt, systemPromptOverride - ) - requestBody.images = {encodedImage} - - -- Send HTTP POST - local url = client.getBaseUrl(prefsService.prefsForPlugin()) .. "/api/generate" - local jsonPayload = json:encode(requestBody) - - local response = http.post(url, jsonPayload, {{ - field = "Content-Type", - value = "application/json" - }}) - - if not response then - return nil, "Failed to send data to the API" + --- Best-effort helper: attempt to clean up the thumbnail and log any failure. + --- Returns true if cleanup succeeded, false if logged. + --- LrFileUtils.delete is a local-disk operation — it does not yield, so + --- pcall here is safe (avoids the "yield through protected boundary" issue + --- that plagues network I/O). + ---@param path string Path to delete + ---@return bool succeeded True if cleanup completed without error + local function tryCleanup(path) + local ok, err = pcall(function() thumbnailService.cleanup(path) end) + if not ok and activeLogger then + activeLogger:error( + "Thumbnail cleanup also failed: " .. tostring(err) + ) end - - -- Return raw response for validation (after cleanup in caller). - return response, nil + return ok end - --- High-level generate: export thumbnail, encode, build prompt, POST, validate response. + --- High-level generate: export thumbnail, encode, build prompt, POST, validate. --- Orchestrates ThumbnailService + PromptBuilder + ResponseValidator internally. --- This is the decomposition of the former Common.sendDataToApi mega-function. - --- Retries thumbnail export up to 3 times. Cleans up the temp file after the - --- HTTP request regardless of success or failure (even when downstream code throws). - --- Validation runs after cleanup so the thumbnail is always removed first. + --- Retries thumbnail export up to 3 times. Cleans up the temp file at every + --- exit point regardless of success or failure. + --- + --- Explicit cleanup (rather than wrapping SDK calls in pcall) avoids the + --- "yield through protected boundary" problem: Lightroom SDK functions like + --- http.post and LrStringUtils.encodeBase64 can yield internally, and pcall + --- cannot survive those yields — it returns early with a false error, silently + --- swallowing the real result. Explicit cleanup at each step is equally safe + --- for return-based failures and avoids the yield problem entirely. + --- Pure-Lua steps (prompt builder, JSON encoding) are still wrapped in pcall + --- because they cannot yield. ---@param photo LrPhoto Photo object from the catalog ---@param userInstruction string User-facing prompt text ---@param currentData table|nil Existing title/caption to prepend when useCurrentData is true @@ -314,25 +272,52 @@ local function createClient(deps) return nil, "Failed to export thumbnail after 3 attempts" end - -- Run the HTTP POST under exception-safe cleanup: thumbnail is deleted after - -- the request regardless of success or exception. - local rawResponse, err = runWithCleanup( - thumbnailPath, - function() - return executePost(thumbnailPath, userInstruction, currentData, - useCurrentData, useSystemPrompt, selectedModel, - systemPromptOverride) - end, - function(path) thumbnailService.cleanup(path) end - ) - - -- If POST/cleanup failed, return immediately. - if not rawResponse then - return nil, err + -- Step 1: Encode image as Base64 (SDK — may yield, don't wrap in pcall). + local encodedImage = thumbnailService.encodeBase64(thumbnailPath) + if not encodedImage then + tryCleanup(thumbnailPath) + return nil, "Failed to encode image" + end + + -- Step 2: Build prompt, assemble body, JSON-encode (pure Lua — pcall-safe). + local jsonPayload + local ok, err = pcall(function() + local builtPrompt = promptBuilder.buildUserPrompt( + userInstruction, currentData, useCurrentData) + local requestBody = promptBuilder.assembleRequestBody( + builtPrompt, selectedModel or client.defaultModel, + useSystemPrompt, systemPromptOverride) + requestBody.images = {encodedImage} + jsonPayload = json:encode(requestBody) + end) + if not ok then + tryCleanup(thumbnailPath) + return nil, tostring(err) + end + + -- Step 3: HTTP POST (SDK — may yield, don't wrap in pcall). + local url = client.getBaseUrl(prefsService.prefsForPlugin()) .. "/api/generate" + local response = http.post(url, jsonPayload, {{ + field = "Content-Type", + value = "application/json" + }}) + if not response then + tryCleanup(thumbnailPath) + return nil, "Failed to send data to the API" + end + + -- Step 4: Cleanup thumbnail before validation — even on success. + -- LrFileUtils.delete is local-disk I/O (no yield), so pcall is safe. + -- If cleanup fails here, no point validating — return the cleanup error. + local cleanupOk, cleanupErr = pcall(function() + thumbnailService.cleanup(thumbnailPath) + end) + if not cleanupOk then + return nil, "Failed to clean up thumbnail: " .. tostring(cleanupErr) end - -- Validate after cleanup so the thumbnail is always removed first. - return responseValidator.validateAndParse(rawResponse) + -- Step 5: Validate after cleanup — thumbnail removed regardless of outcome. + return responseValidator.validateAndParse(response) end return client diff --git a/tests/spec/ollama_client_generate_spec.lua b/tests/spec/ollama_client_generate_spec.lua index 2888d57..c19d802 100644 --- a/tests/spec/ollama_client_generate_spec.lua +++ b/tests/spec/ollama_client_generate_spec.lua @@ -185,13 +185,11 @@ describe("OllamaClient — generate()", function() end) -------------------------------------------------------------------- - -- 3. Encoding throws → exception propagates (no pcall in Lightroom). - -- In production, SDK functions return nil on error rather than - -- throwing, so this path is defensive-only. Tests catch with - -- pcall at the test level since cleanup cannot run when an - -- intermediate step throws without protected calls. + -- 3. Encoding throws → SDK crashes propagate (no pcall around + -- yielding SDK functions). Cleanup is a best-effort trade-off: + -- normal nil-returns trigger cleanup; true SDK crashes do not. -------------------------------------------------------------------- - it("propagates exceptions from encoding", function() + it("propagates when encoding SDK function throws", function() f = makeFakes() f.thumbnailService._export_return = "/tmp/t.jpg" f.thumbnailService.encodeBase64 = function(_) @@ -202,21 +200,22 @@ describe("OllamaClient — generate()", function() Client = assert(loadfile(path .. "OllamaClient.lua"))() c = Client.new(f) + -- SDK throws propagate because we avoid wrapping yielding SDK calls in pcall. local ok, err = pcall(c.generate, c, { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil ) assert.is_false(ok) assert.is_not_nil(string.find(tostring(err), "encode exploded", 1, true)) - -- Note: cleanup cannot run when intermediate steps throw - -- without pcall (which Lightroom's sandbox doesn't support). + -- Cleanup cannot run when SDK functions throw — acceptable trade-off + -- that avoids the "yield through protected boundary" problem. assert.are_same(0, #f.http.post_calls) end) -------------------------------------------------------------------- - -- 4. Prompt builder throws → same propagation behavior. + -- 4. Prompt builder throws → cleanup runs, error returned. -------------------------------------------------------------------- - it("propagates exceptions from prompt builder", function() + it("cleans up when prompt builder throws", function() f = makeFakes() f.thumbnailService._export_return = "/tmp/t.jpg" f.thumbnailService._encode_return = "img" @@ -228,18 +227,21 @@ describe("OllamaClient — generate()", function() Client = assert(loadfile(path .. "OllamaClient.lua"))() c = Client.new(f) - local ok, err = pcall(c.generate, c, + local result, err = c.generate( { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil ) - assert.is_false(ok) + assert.is_nil(result) assert.is_not_nil(string.find(tostring(err), "prompt builder error", 1, true)) + -- Cleanup runs despite exception. + assert.are_same(1, #f.thumbnailService.cleanup_calls) + assert.are_same("/tmp/t.jpg", f.thumbnailService.cleanup_calls[1]) end) -------------------------------------------------------------------- - -- 5. Request-body assembly throws → same propagation behavior. + -- 5. Request-body assembly throws → cleanup runs, error returned. -------------------------------------------------------------------- - it("propagates exceptions from request-body assembly", function() + it("cleans up when request-body assembly throws", function() f = makeFakes() f.thumbnailService._export_return = "/tmp/t.jpg" f.thumbnailService._encode_return = "img" @@ -251,18 +253,21 @@ describe("OllamaClient — generate()", function() Client = assert(loadfile(path .. "OllamaClient.lua"))() c = Client.new(f) - local ok, err = pcall(c.generate, c, + local result, err = c.generate( { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil ) - assert.is_false(ok) + assert.is_nil(result) assert.is_not_nil(string.find(tostring(err), "assembly error", 1, true)) + -- Cleanup runs despite exception. + assert.are_same(1, #f.thumbnailService.cleanup_calls) + assert.are_same("/tmp/t.jpg", f.thumbnailService.cleanup_calls[1]) end) -------------------------------------------------------------------- - -- 6. JSON encoding throws → same propagation behavior. + -- 6. JSON encoding throws → cleanup runs, error returned. -------------------------------------------------------------------- - it("propagates exceptions from JSON encoding", function() + it("cleans up when JSON encoding throws", function() f = makeFakes() f.thumbnailService._export_return = "/tmp/t.jpg" f.thumbnailService._encode_return = "img" @@ -274,12 +279,15 @@ describe("OllamaClient — generate()", function() Client = assert(loadfile(path .. "OllamaClient.lua"))() c = Client.new(f) - local ok, err = pcall(c.generate, c, + local result, err = c.generate( { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil ) - assert.is_false(ok) + assert.is_nil(result) assert.is_not_nil(string.find(tostring(err), "json encoding failed", 1, true)) + -- Cleanup runs despite exception. + assert.are_same(1, #f.thumbnailService.cleanup_calls) + assert.are_same("/tmp/t.jpg", f.thumbnailService.cleanup_calls[1]) assert.are_same(0, #f.http.post_calls) end) @@ -310,9 +318,10 @@ describe("OllamaClient — generate()", function() end) -------------------------------------------------------------------- - -- 8. HTTP POST throws → exception propagates (same as above). + -- 8. HTTP POST throws → SDK crashes propagate (no pcall around + -- yielding SDK functions). Cleanup is best-effort for SDK crashes. -------------------------------------------------------------------- - it("propagates exceptions from http.post", function() + it("propagates when http.post throws", function() f = makeFakes() f.thumbnailService._export_return = "/tmp/t.jpg" f.thumbnailService._encode_return = "img" @@ -324,6 +333,7 @@ describe("OllamaClient — generate()", function() Client = assert(loadfile(path .. "OllamaClient.lua"))() c = Client.new(f) + -- SDK throws propagate because we avoid wrapping yielding SDK calls in pcall. local ok, err = pcall(c.generate, c, { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil ) @@ -395,60 +405,80 @@ describe("OllamaClient — generate()", function() end) -------------------------------------------------------------------- - -- 11. Cleanup fails after successful generation → exception propagates. - -- Without pcall, a cleanup failure is an exception that bubbles up. - -- In production, LrFileUtils.delete (the cleanup implementation) - -- returns nil on error rather than throwing, so this is defensive. + -- 11. Cleanup fails after successful generation → error returned, not thrown. -------------------------------------------------------------------- - it("propagates exceptions when cleanup fails after generation", function() + it("returns cleanup error when cleanup fails after successful generation", function() f = makeFakes() f.thumbnailService._export_return = "/tmp/t.jpg" f.thumbnailService._encode_return = "img" f.http._response = '{"response":"{\"title\":\"T\",\"caption\":\"C\",\"keywords\":[\"k\"]}"}' f.responseValidator._metadata = { title = "T", caption = "C", keywords = { "k" } } - -- Make cleanup throw. - f.thumbnailService.cleanup = function(_) + -- Make cleanup throw (but still record the call). + f.thumbnailService.cleanup = function(imagePath) table.insert(f.events, "cleanup") + table.insert(f.thumbnailService.cleanup_calls, imagePath) error("disk full") end Client = assert(loadfile(path .. "OllamaClient.lua"))() c = Client.new(f) - local ok, err = pcall(c.generate, c, + local result, err = c.generate( { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil ) - assert.is_false(ok) + -- pcall catches cleanup exception and returns it as error. + assert.is_nil(result) + assert.is_not_nil(string.find(tostring(err), "Failed to clean up thumbnail:", 1, true)) assert.is_not_nil(string.find(tostring(err), "disk full", 1, true)) + assert.are_same(1, #f.thumbnailService.cleanup_calls) + -- Validation should not have been called (cleanup failed before validation). + assert.are_same(0, #f.responseValidator.calls) end) -------------------------------------------------------------------- - -- 12. Cleanup fails AND generation failed → first exception wins. - -- Without pcall, when generation throws, cleanup never runs, so - -- the generation error is what propagates (cleanup is never reached). + -- 12. Both generation AND cleanup fail → original error primary, cleanup logged. -------------------------------------------------------------------- - it("propagates generation error when generation throws", function() + it("preserves generation error when both generation and cleanup fail", function() f = makeFakes() f.thumbnailService._export_return = "/tmp/t.jpg" f.thumbnailService._encode_return = "img" - -- POST throws (generation fails before cleanup is reached) - f.http.post = function(_, _, _) - table.insert(f.events, "post") - error("network timeout") + -- JSON encoding throws (generation fails) + f.json.encode = function(_) + table.insert(f.events, "json-encode") + error("json encoding failed") + end + -- Cleanup also throws (but still records the call). + f.thumbnailService.cleanup = function(imagePath) + table.insert(f.events, "cleanup") + table.insert(f.thumbnailService.cleanup_calls, imagePath) + error("disk full") end Client = assert(loadfile(path .. "OllamaClient.lua"))() c = Client.new(f) - local ok, err = pcall(c.generate, c, + local result, err = c.generate( { uuid = "p" }, "hi", nil, false, true, "gemma4:latest", nil ) - assert.is_false(ok) - -- Generation error propagates; cleanup is never reached. - assert.is_not_nil(string.find(tostring(err), "network timeout", 1, true), - "Expected original generation error to propagate") + -- Original generation error is primary. + assert.is_nil(result) + assert.is_not_nil(string.find(tostring(err), "json encoding failed", 1, true), + "Expected original generation error to be returned") + -- Cleanup failure was logged, not surfaced as primary error. + assert.are_same( + 1, #f.logger.error_calls, + "Cleanup failure should be logged" + ) + assert.is_not_nil( + string.find(tostring(f.logger.error_calls[1]), "disk full", 1, true), + "Logged message should contain cleanup error" + ) + -- Cleanup was attempted exactly once. + assert.are_same(1, #f.thumbnailService.cleanup_calls) + -- POST should not have been called (JSON encoding failed first). + assert.are_same(0, #f.http.post_calls) end) -------------------------------------------------------------------- @@ -570,6 +600,26 @@ describe("OllamaClient — generate()", function() fake.http._response = nil end, }, + { + "prompt-throw", + function(fake) + fake.thumbnailService._encode_return = "img" + fake.promptBuilder.buildUserPrompt = function() + table.insert(fake.events, "build-prompt") + error("prompt builder error") + end + end, + }, + { + "json-throw", + function(fake) + fake.thumbnailService._encode_return = "img" + fake.json.encode = function(_) + table.insert(fake.events, "json-encode") + error("json encoding failed") + end + end, + }, } for _, scenario in ipairs(scenarios) do diff --git a/tests/spec/thumbnail_service_spec.lua b/tests/spec/thumbnail_service_spec.lua new file mode 100644 index 0000000..c12b0c2 --- /dev/null +++ b/tests/spec/thumbnail_service_spec.lua @@ -0,0 +1,335 @@ +--- thumbnail_service_spec.lua — Direct unit tests for ThumbnailService module. +--- Covers cleanup(), encodeBase64(), and export() with mocked SDK dependencies. + +local path = PLUGIN_PATH + +-------------------------------------------------------------------------------- +--- Helper: load ThumbnailService with configurable fake SDK modules. +--- Overrides import() temporarily so that the module captures our fakes as +--- its local LrFileUtils, LrPathUtils, LrStringUtils, LrLogger references. +-------------------------------------------------------------------------------- +local function loadWithFakes(fakeLrFileUtils, fakeLrPathUtils, fakeLrStringUtils) + local oldImport = _G.import + + -- LrLogger is a constructor: LrLogger('name') → logger instance. + local loggerFactory = setmetatable({}, { + __call = function(_, _) + return { + info = function() end, + warn = function() end, + error = function() end, + enable = function() end, + } + end, + }) + + _G.import = function(className) + if className == "LrFileUtils" then return fakeLrFileUtils end + if className == "LrPathUtils" then return fakeLrPathUtils end + if className == "LrStringUtils" then return fakeLrStringUtils end + if className == "LrLogger" then return loggerFactory end + return oldImport(className) + end + + local thumbnail = dofile(path .. "ThumbnailService.lua") + + -- Restore immediately so other tests are unaffected. + _G.import = oldImport + return thumbnail +end + +-------------------------------------------------------------------------------- +describe("ThumbnailService — cleanup()", function() + + it("deletes an existing file", function() + local fakeFs = { delete_calls = {} } + fakeFs.exists = function(p) return true end + fakeFs.delete = function(p) table.insert(fakeFs.delete_calls, p) end + + local ts = loadWithFakes(fakeFs, {}, {}) + ts.cleanup("/tmp/thumb.jpg") + + assert.are_same(1, #fakeFs.delete_calls) + assert.are_same("/tmp/thumb.jpg", fakeFs.delete_calls[1]) + end) + + it("does nothing when the file does not exist", function() + local fakeFs = { delete_calls = {} } + fakeFs.exists = function(_) return false end + fakeFs.delete = function(p) table.insert(fakeFs.delete_calls, p) end + + local ts = loadWithFakes(fakeFs, {}, {}) + ts.cleanup("/tmp/gone.jpg") + + assert.are_same(0, #fakeFs.delete_calls) + end) + + it("does nothing when given a nil path", function() + local fakeFs = { delete_calls = {} } + fakeFs.exists = function(p) return true end + fakeFs.delete = function(p) table.insert(fakeFs.delete_calls, p) end + + local ts = loadWithFakes(fakeFs, {}, {}) + ts.cleanup(nil) + + assert.are_same(0, #fakeFs.delete_calls) + end) + + it("passes the exact path to deletion", function() + local fakeFs = { delete_calls = {} } + local expectedPath = "/var/folders/xx/unique_name.jpg" + fakeFs.exists = function(p) return p == expectedPath end + fakeFs.delete = function(p) table.insert(fakeFs.delete_calls, p) end + + local ts = loadWithFakes(fakeFs, {}, {}) + ts.cleanup(expectedPath) + + assert.are_same(1, #fakeFs.delete_calls) + assert.are_same(expectedPath, fakeFs.delete_calls[1]) + end) +end) + +-------------------------------------------------------------------------------- +describe("ThumbnailService — encodeBase64()", function() + + it("returns nil when the file does not exist", function() + local fakeFs = {} + fakeFs.exists = function(_) return false end + + local ts = loadWithFakes(fakeFs, {}, {}) + local result = ts.encodeBase64("/tmp/nope.jpg") + + assert.is_nil(result) + end) + + it("returns nil when io.open fails for a file that exists", function() + -- Mock exists=true but the path doesn't exist on disk, so io.open fails. + local fakeFs = {} + fakeFs.exists = function(_) return true end + + local ts = loadWithFakes(fakeFs, {}, {}) + local result = ts.encodeBase64("/nonexistent/doesnotexist.jpg") + + assert.is_nil(result) + end) + + it("returns nil for an empty file", function() + -- Create a real empty temp file. + local tmpname = path .. "empty_test_tmp_" .. os.time() .. ".jpg" + local f = io.open(tmpname, "wb") + if f then f:close() end + + local fakeFs = {} + fakeFs.exists = function(p) return p == tmpname end + + local fakeStrUtils = {} + fakeStrUtils.encodeBase64 = function(data) return "encoded" end + + local ts = loadWithFakes(fakeFs, {}, fakeStrUtils) + local result = ts.encodeBase64(tmpname) + + assert.is_nil(result) + os.remove(tmpname) + end) + + it("returns encoded value for a valid file", function() + -- Create a real temp file with known content. + local tmpname = path .. "valid_test_tmp_" .. os.time() .. ".jpg" + local f = io.open(tmpname, "wb") + if f then f:write("\xFF\xD8\xFF\xE0"); f:close() end + + local fakeFs = {} + fakeFs.exists = function(p) return p == tmpname end + + local encodedBytes = nil + local fakeStrUtils = {} + fakeStrUtils.encodeBase64 = function(data) + encodedBytes = data + return "base64encoded" + end + + local ts = loadWithFakes(fakeFs, {}, fakeStrUtils) + local result = ts.encodeBase64(tmpname) + + assert.are_same("base64encoded", result) + assert.are_same("\xFF\xD8\xFF\xE0", encodedBytes) + os.remove(tmpname) + end) + + it("returns nil when the Base64 encoder returns nil", function() + local tmpname = path .. "encoder_fail_tmp_" .. os.time() .. ".jpg" + local f = io.open(tmpname, "wb") + if f then f:write("data"); f:close() end + + local fakeFs = {} + fakeFs.exists = function(p) return p == tmpname end + + local fakeStrUtils = {} + fakeStrUtils.encodeBase64 = function(_) return nil end + + local ts = loadWithFakes(fakeFs, {}, fakeStrUtils) + local result = ts.encodeBase64(tmpname) + + assert.is_nil(result) + os.remove(tmpname) + end) +end) + +-------------------------------------------------------------------------------- +describe("ThumbnailService — export()", function() + + it("returns nil when the temp directory does not exist", function() + local fakePathUtils = {} + fakePathUtils.getStandardFilePath = function(kind) return "/nonexistent" end + + local fakeFs = {} + fakeFs.exists = function(_) return false end + fakeFs.chooseUniqueFileName = function(template) return template end + + local ts = loadWithFakes(fakeFs, fakePathUtils, {}) + local result = ts.export({ uuid = "p1" }) + + assert.is_nil(result) + end) + + it("returns nil when the photo API returns failure", function() + local fakePathUtils = {} + fakePathUtils.getStandardFilePath = function(kind) return "/tmp" end + + local fakeFs = {} + fakeFs.exists = function(_) return true end + fakeFs.chooseUniqueFileName = function(template) return template end + + local photo = { + requestJpegThumbnail = function(_, width, height, cb) + return false, "no thumbnail available" + end, + } + + local ts = loadWithFakes(fakeFs, fakePathUtils, {}) + local result = ts.export(photo) + + assert.is_nil(result) + end) + + it("returns nil when the callback receives no JPEG data", function() + local fakePathUtils = {} + fakePathUtils.getStandardFilePath = function(kind) return "/tmp" end + + local fakeFs = {} + fakeFs.exists = function(_) return true end + fakeFs.chooseUniqueFileName = function(template) return template end + + local photo = { + requestJpegThumbnail = function(_, width, height, cb) + cb(nil) + return true, "ok" + end, + } + + local ts = loadWithFakes(fakeFs, fakePathUtils, {}) + local result = ts.export(photo) + + assert.is_nil(result) + end) + + it("returns the path on successful export", function() + local fakePathUtils = {} + fakePathUtils.getStandardFilePath = function(kind) return "/tmp" end + + local writtenBytes = nil + local capturedPath = nil + local originalIoOpen = io.open + -- Intercept io.open to capture written data. + io.open = function(filepath, mode) + capturedPath = filepath + return { + write = function(_, data) writtenBytes = data end, + close = function() end, + } + end + + local fakeFs = {} + -- exists is called twice: once for temp dir check, once for final verification + fakeFs.exists = function(_) return true end + fakeFs.chooseUniqueFileName = function(template) return "/tmp/thumb_1.jpg" end + + local photo = { + requestJpegThumbnail = function(_, width, height, cb) + assert.are_same(512, width, "Should request 512px width") + assert.are_same(512, height, "Should request 512px height") + cb("JPEG_BINARY_DATA") + return true, "ok" + end, + } + + local ts = loadWithFakes(fakeFs, fakePathUtils, {}) + local result = ts.export(photo) + + io.open = originalIoOpen + + assert.are_same("/tmp/thumb_1.jpg", result) + assert.are_same("JPEG_BINARY_DATA", writtenBytes) + end) + + it("returns nil when io.open fails inside the callback", function() + local fakePathUtils = {} + fakePathUtils.getStandardFilePath = function(kind) return "/tmp" end + + local originalIoOpen = io.open + io.open = function(_, _) return nil end -- simulate file-open failure + + local fakeFs = {} + fakeFs.exists = function(_) return true end + fakeFs.chooseUniqueFileName = function(template) return "/tmp/thumb_1.jpg" end + + local photo = { + requestJpegThumbnail = function(_, width, height, cb) + cb("JPEG_BINARY_DATA") + return true, "ok" + end, + } + + local ts = loadWithFakes(fakeFs, fakePathUtils, {}) + local result = ts.export(photo) + + io.open = originalIoOpen + + assert.is_nil(result) + end) + + it("requests 512x512 dimensions", function() + local fakePathUtils = {} + fakePathUtils.getStandardFilePath = function(kind) return "/tmp" end + + local dims = { w = nil, h = nil } + local originalIoOpen = io.open + io.open = function(_, _) + return { + write = function(_, _) end, + close = function() end, + } + end + + local fakeFs = {} + fakeFs.exists = function(_) return true end + fakeFs.chooseUniqueFileName = function(template) return "/tmp/thumb_1.jpg" end + + local photo = { + requestJpegThumbnail = function(_, width, height, cb) + dims.w = width + dims.h = height + cb("data") + return true, "ok" + end, + } + + local ts = loadWithFakes(fakeFs, fakePathUtils, {}) + ts.export(photo) + + io.open = originalIoOpen + + assert.are_same(512, dims.w) + assert.are_same(512, dims.h) + end) +end)