diff --git a/pyvolca/README.md b/pyvolca/README.md index ccfd8d18..315b7810 100644 --- a/pyvolca/README.md +++ b/pyvolca/README.md @@ -638,7 +638,8 @@ to filtering endpoints. Load a database into memory so it answers queries. -Has no effect if the database is already loaded. +Declared dependencies are loaded first; has no effect if the +database is already loaded. ##### `Client.refresh_stubs()` @@ -719,6 +720,8 @@ Args: Unload a database from memory to free RAM. The disk copy is kept. +Refused if another loaded database still depends on it. + ##### `Client.use(db_name: str) -> 'Client'` Return a new client targeting a different database. diff --git a/pyvolca/src/volca/client.py b/pyvolca/src/volca/client.py index 87351995..b45eed14 100644 --- a/pyvolca/src/volca/client.py +++ b/pyvolca/src/volca/client.py @@ -626,14 +626,17 @@ def list_databases(self) -> list[DatabaseInfo]: def load_database(self, db_name: str) -> dict: """Load a database into memory so it answers queries. - Has no effect if the database is already loaded. + Declared dependencies are loaded first; has no effect if the + database is already loaded. """ - # No operationId (infrastructure endpoint). Direct HTTP. - return self._json(self._session.post(f"{self.base_url}/api/v1/db/{db_name}/load")) + return self._call("load_database", db_name=db_name) def unload_database(self, db_name: str) -> dict: - """Unload a database from memory to free RAM. The disk copy is kept.""" - return self._json(self._session.post(f"{self.base_url}/api/v1/db/{db_name}/unload")) + """Unload a database from memory to free RAM. The disk copy is kept. + + Refused if another loaded database still depends on it. + """ + return self._call("unload_database", db_name=db_name) # -- Database write operations -- # diff --git a/pyvolca/tests/test_db_write.py b/pyvolca/tests/test_db_write.py index 45ab03a8..4065b1e4 100644 --- a/pyvolca/tests/test_db_write.py +++ b/pyvolca/tests/test_db_write.py @@ -2,10 +2,10 @@ These endpoints (copy / delete / relink / export / add-/remove-dependency) carry no operationId — they bypass the OpenAPI dispatcher and build their -URLs directly, like ``load_database`` / ``unload_database``. They also do -not exist in any released engine binary, so these tests never touch a live -engine: they mock ``Client._session`` and assert on the wire shape -(URL, JSON body, base64 decoding, format validation, error surfacing). +URLs directly. They also do not exist in any released engine binary, so these +tests never touch a live engine: they mock ``Client._session`` and assert on +the wire shape (URL, JSON body, base64 decoding, format validation, error +surfacing). """ from __future__ import annotations diff --git a/pyvolca/tests/test_drift.py b/pyvolca/tests/test_drift.py index b5878629..b8a1f055 100644 --- a/pyvolca/tests/test_drift.py +++ b/pyvolca/tests/test_drift.py @@ -21,6 +21,8 @@ # and engine-side renames. WRAPPER_OPERATIONS = [ "list_databases", + "load_database", + "unload_database", "list_presets", "search_activities", "search_flows", @@ -68,9 +70,11 @@ def test_all_resources_have_operationid(live_spec): if live_spec is None: pytest.skip("No built engine binary found.") live_ops = _parse_spec(live_spec) - # We expect at least the 17 Resources that have an apiPath entry - # (list_geographies is MCP-only and correctly has no operationId). - assert len(live_ops) >= 17, ( + # 22 of the 25 Resource constructors carry an apiPath, so each stamps an + # operationId; the 3 MCP-only ones (list_geographies / compare_impacts / + # list_scoring_sets) correctly have none. A looser bound would silently + # tolerate a path-template mismatch dropping an operationId. + assert len(live_ops) >= 22, ( f"Only {len(live_ops)} operations have operationIds in the live spec. " - f"Expected ≥17. Check API/OpenApi.hs enrichWithResources." + f"Expected ≥22. Check API/OpenApi.hs enrichWithResources." ) diff --git a/src/API/DatabaseHandlers.hs b/src/API/DatabaseHandlers.hs index bcf5e82a..fc69b491 100644 --- a/src/API/DatabaseHandlers.hs +++ b/src/API/DatabaseHandlers.hs @@ -170,14 +170,11 @@ getDatabases = do loadDatabaseHandler :: Text -> AppM LoadDatabaseResponse loadDatabaseHandler dbName = do dbManager <- asks aeDbManager - eitherResult <- liftIO $ try $ loadDatabase dbManager dbName - case eitherResult of - Left (ex :: SomeException) -> - return $ LoadFailed ("Server exception: " <> T.pack (show ex)) - Right (Left err) -> return $ LoadFailed err - Right (Right (loadedDb, depResults)) -> do - let status = makeStatusFromLoadedDb loadedDb - return $ LoadSucceeded status depResults + result <- liftIO $ loadDatabase dbManager dbName + case result of + Left err -> return $ LoadFailed err + Right (loadedDb, depResults) -> + return $ LoadSucceeded (makeStatusFromLoadedDb loadedDb) depResults -- | Unload a database from memory unloadDatabaseHandler :: Text -> AppM ActivateResponse diff --git a/src/API/MCP.hs b/src/API/MCP.hs index 004fd20c..e795832a 100644 --- a/src/API/MCP.hs +++ b/src/API/MCP.hs @@ -5,7 +5,7 @@ Implements Streamable HTTP transport (MCP spec 2025-03-26). POST /mcp handles initialize, tools/list, tools/call (JSON or SSE response). GET /mcp opens an SSE stream for server-initiated messages (stateless: closes immediately). -} -module API.MCP (mcpApp, toolDefinitions, selectMethod) where +module API.MCP (mcpApp, toolDefinitions, callTool, selectMethod) where import Control.Concurrent.STM (readTVarIO) import Data.Aeson @@ -337,6 +337,8 @@ parseCallParams _ = Nothing callTool :: DatabaseManager -> [ClassificationPreset] -> Maybe Text -> Value -> Text -> KeyMap Value -> IO Value callTool dbManager presets mBaseUrl rid name args = case name of "list_databases" -> callListDatabases dbManager rid + "load_database" -> callLoadDatabase dbManager rid args + "unload_database" -> callUnloadDatabase dbManager rid args "list_presets" -> callListPresets presets rid "search_activities" -> withDb dbManager rid args $ callSearchActivities presets rid args "search_flows" -> withDb dbManager rid args $ callSearchFlows rid args @@ -490,6 +492,38 @@ callListDatabases dbManager rid = do entries = map mkDbEntry (M.elems loaded) return $ toolSuccessJson rid $ object ["databases" .= entries] +{- | Load a configured database into the working set. Wraps +'DM.loadDatabase', which also auto-loads declared dependencies. Any +dependency that fails to load is surfaced in the 'dependencies' array +(as a DepLoadFailed entry) rather than swallowed. +-} +callLoadDatabase :: DatabaseManager -> Value -> KeyMap Value -> IO Value +callLoadDatabase dbManager rid args = runTool rid $ do + dbName <- except (requireText "database" args) + (_loaded, deps) <- ExceptT (DM.loadDatabase dbManager dbName) + pure $ + toolSuccessJson rid $ + object + [ "status" .= ("loaded" :: Text) + , "database" .= dbName + , "dependencies" .= deps + ] + +{- | Unload a database from the working set. Wraps 'DM.unloadDatabase', +which refuses (returns 'Left') when another loaded database still +depends on it. +-} +callUnloadDatabase :: DatabaseManager -> Value -> KeyMap Value -> IO Value +callUnloadDatabase dbManager rid args = runTool rid $ do + dbName <- except (requireText "database" args) + ExceptT (DM.unloadDatabase dbManager dbName) + pure $ + toolSuccessJson rid $ + object + [ "status" .= ("unloaded" :: Text) + , "database" .= dbName + ] + callListPresets :: [ClassificationPreset] -> Value -> IO Value callListPresets presets rid = return $ diff --git a/src/API/Resources.hs b/src/API/Resources.hs index ebaab6be..614057ca 100644 --- a/src/API/Resources.hs +++ b/src/API/Resources.hs @@ -38,13 +38,17 @@ import Network.HTTP.Types.Method (StdMethod (..)) {- | Every operation VoLCA exposes through its user-facing surfaces. -Note: this covers the *queryable* operations. Infrastructure endpoints -(database load/unload, method-collection management, auth, version) are -not represented here — they live in Routes.hs only, because they have -no analyst-facing equivalent across all surfaces. +This covers the analyst-facing operations. Loading and unloading a database +belong here too: they change which databases are in the working set, not the +(read-only) data itself, so they are a legitimate analyst action and are +exposed on every surface. Genuinely state-mutating or infrastructure endpoints +(method-collection management, upload/delete/relink/copy, auth, version) stay +in Routes.hs only — they have no analyst-facing equivalent across all surfaces. -} data Resource = ListDatabases + | LoadDatabase + | UnloadDatabase | ListPresets | SearchActivities | SearchFlows @@ -124,6 +128,8 @@ spec. A build-time drift test (ResourcesDriftSpec) asserts that every apiPath :: Resource -> Maybe (StdMethod, [Text]) apiPath r = case r of ListDatabases -> Just (GET, ["db"]) + LoadDatabase -> Just (POST, ["db", "{dbName}", "load"]) + UnloadDatabase -> Just (POST, ["db", "{dbName}", "unload"]) ListPresets -> Just (GET, ["classification-presets"]) SearchActivities -> Just (GET, ["db", "{dbName}", "activities"]) SearchFlows -> Just (GET, ["db", "{dbName}", "flows"]) @@ -164,6 +170,8 @@ apiPathText r = do mcpName :: Resource -> Text mcpName r = case r of ListDatabases -> "list_databases" + LoadDatabase -> "load_database" + UnloadDatabase -> "unload_database" ListPresets -> "list_presets" SearchActivities -> "search_activities" SearchFlows -> "search_flows" @@ -200,6 +208,8 @@ not a top-level command). For those, 'cliName' returns the nested form. cliName :: Resource -> Text cliName r = case r of ListDatabases -> "database list" + LoadDatabase -> "database load" + UnloadDatabase -> "database unload" ListPresets -> "presets" SearchActivities -> "activities" SearchFlows -> "flows" @@ -249,6 +259,16 @@ description r = case r of ListDatabases -> "LCA / ACV — list all loaded LCA databases (Agribalyse, ecoinvent, …). \ \Call this first to discover which databases are available before searching." + LoadDatabase -> + "LCA / ACV — load a configured database into memory so it can be queried. \ + \Its declared dependencies are loaded first (needed for cross-database flow \ + \linking). A database must be loaded before search/score/impact tools can \ + \target it; use list_databases to see which are configured. No effect if it \ + \is already loaded." + UnloadDatabase -> + "LCA / ACV — unload a database from memory to free RAM. The on-disk data is \ + \kept and the database can be reloaded later with load_database. Refuses if \ + \another loaded database still depends on it — unload the dependents first." ListPresets -> "LCA / ACV — list named classification filter presets configured in this \ \instance. Each preset bundles multiple (system, value, mode) classification \ @@ -516,6 +536,8 @@ pSummaryOnly = params :: Resource -> [Param] params r = case r of ListDatabases -> [] + LoadDatabase -> [pDatabase] + UnloadDatabase -> [pDatabase] ListPresets -> [] SearchActivities -> [ pDatabase diff --git a/src/CLI/Client.hs b/src/CLI/Client.hs index 70500e1c..445a33ed 100644 --- a/src/CLI/Client.hs +++ b/src/CLI/Client.hs @@ -82,6 +82,13 @@ executeRemoteCommand mgr rc globalOpts cmd = do case cmd of Database DbList -> apiGet mgr rc "/api/v1/db" >>= output fmt jp + Database (DbLoad name) -> + -- Load endpoint takes no body; the empty object is ignored server-side. + apiPost mgr rc ("/api/v1/db/" ++ T.unpack name ++ "/load") (object []) + >>= outputLoad fmt jp + Database (DbUnload name) -> + apiPost mgr rc ("/api/v1/db/" ++ T.unpack name ++ "/unload") (object []) + >>= outputStatus fmt jp "unload" Database (DbUpload args) -> executeUpload mgr rc fmt jp "/api/v1/db/upload" args Database (DbDelete name) -> @@ -383,6 +390,20 @@ outputStatus fmt jp action (Right val) = case parseMaybe parseStatus val of parseStatus :: Value -> Parser (Bool, Text) parseStatus = withObject "StatusResponse" $ \o -> (,) <$> o .: "success" <*> o .: "message" +{- | @database load@ returns HTTP 200 even on failure, with a bare +@{"error": …}@ body ('LoadDatabaseResponse'\'s @LoadFailed@) and no @success@ +field for 'outputStatus' to check. Mirror 'outputStatus': fail loudly when that +key is present, so a failed remote load exits non-zero instead of printing the +error and returning success. +-} +outputLoad :: OutputFormat -> Maybe Text -> Either String Value -> IO () +outputLoad _ _ (Left err) = reportError err >> exitFailure +outputLoad fmt jp (Right val) = case val of + Object o + | Just (String err) <- KM.lookup "error" o -> + reportError ("load failed: " ++ T.unpack err) >> exitFailure + _ -> output fmt jp (Right val) + -- | Format and output a result output :: OutputFormat -> Maybe Text -> Either String Value -> IO () output _ _ (Left err) = reportError err >> exitFailure diff --git a/src/CLI/Command.hs b/src/CLI/Command.hs index 661aad0b..e9e89d2b 100644 --- a/src/CLI/Command.hs +++ b/src/CLI/Command.hs @@ -97,6 +97,10 @@ executeCommand (CLIConfig globalOpts _) cmd manager = do exitFailure Database DbList -> DM.listDatabases manager >>= out . toJSON + Database (DbLoad name) -> + executeDbLoad registry outputFormat manager name + Database (DbUnload name) -> + executeDbUnload registry outputFormat manager name Database (DbUpload args) -> executeDbUpload registry outputFormat manager args Database (DbDelete name) -> @@ -513,6 +517,34 @@ executeDbDeleteActivities registry fmt manager args = do reportProgress Info $ "Deleted " ++ show deleted ++ " activities from " ++ T.unpack (ddaDb args) outputResult registry fmt $ object ["database" .= ddaDb args, "deleted" .= deleted] +{- | Execute database load: bring a configured database (and its declared +dependencies) into memory. Any failed dependency is surfaced in the output. +-} +executeDbLoad :: PluginRegistry -> OutputFormat -> DatabaseManager -> Text -> IO () +executeDbLoad registry fmt manager name = do + result <- DM.loadDatabase manager name + case result of + Left err -> do + reportError $ "Load failed: " ++ T.unpack err + exitFailure + Right (_loaded, deps) -> do + reportProgress Info $ "Loaded database: " ++ T.unpack name + outputResult registry fmt $ object ["loaded" .= name, "dependencies" .= deps] + +{- | Execute database unload: drop a database from memory (refused while another +loaded database still depends on it). +-} +executeDbUnload :: PluginRegistry -> OutputFormat -> DatabaseManager -> Text -> IO () +executeDbUnload registry fmt manager name = do + result <- DM.unloadDatabase manager name + case result of + Left err -> do + reportError $ "Unload failed: " ++ T.unpack err + exitFailure + Right () -> do + reportProgress Info $ "Unloaded database: " ++ T.unpack name + outputResult registry fmt $ object ["unloaded" .= name] + -- | Execute database copy command executeDbCopy :: PluginRegistry -> OutputFormat -> DatabaseManager -> Text -> Text -> IO () executeDbCopy registry fmt manager srcName newName = do diff --git a/src/CLI/Parser.hs b/src/CLI/Parser.hs index a6745e3e..619910c4 100644 --- a/src/CLI/Parser.hs +++ b/src/CLI/Parser.hs @@ -115,6 +115,8 @@ databaseParser = <$> optional ( subparser ( OA.command "list" (info (pure DbList) (progDesc "List databases")) + <> OA.command "load" (info (DbLoad <$> textArg "DB" "Name of the configured database to load" <**> helper) (progDesc "Load a configured database into memory")) + <> OA.command "unload" (info (DbUnload <$> textArg "DB" "Name of the loaded database to unload" <**> helper) (progDesc "Unload a database from memory")) <> OA.command "upload" (info (DbUpload <$> uploadArgsParser) (progDesc "Upload a database from a local file")) <> OA.command "delete" (info (DbDelete <$> deleteNameParser) (progDesc "Delete a database")) <> OA.command "delete-activities" (info (DbDeleteActivities <$> deleteActivitiesArgsParser <**> helper) (progDesc "Delete the whole filtered set of activities from a loaded database")) diff --git a/src/CLI/Types.hs b/src/CLI/Types.hs index 2ea0e234..209c131d 100644 --- a/src/CLI/Types.hs +++ b/src/CLI/Types.hs @@ -62,6 +62,10 @@ data Command -- | Database management actions data DatabaseAction = DbList + | -- | Load a configured database into memory (auto-loads its dependencies) + DbLoad Text + | -- | Unload a database from memory (refused if a loaded database depends on it) + DbUnload Text | DbUpload UploadArgs | DbDelete Text | {- | Delete the activities matched by a filter (the whole matching set, diff --git a/src/Database/Manager.hs b/src/Database/Manager.hs index 7294eb08..504307bf 100644 --- a/src/Database/Manager.hs +++ b/src/Database/Manager.hs @@ -1880,19 +1880,27 @@ Pre-loads declared dependencies (from TOML config) so cross-DB linking works, then loads the target database. -} loadDatabase :: DatabaseManager -> Text -> IO (Either Text (LoadedDatabase, [DepLoadResult])) -loadDatabase manager dbName = do - -- Pre-load declared dependencies so they're available for cross-DB linking - availableDbs <- readTVarIO (dmAvailableDbs manager) - let configDeps = maybe [] dcDepends (M.lookup dbName availableDbs) - depResults1 <- autoLoadDeps manager configDeps - - result <- loadDatabaseSingle manager dbName - case result of - Left err -> return (Left err) - Right loaded -> do - -- Also auto-load any runtime-discovered dependencies - depResults2 <- autoLoadDeps manager (dbDependsOn (ldDatabase loaded)) - return (Right (loaded, depResults1 ++ depResults2)) +loadDatabase manager dbName = fmap flattenLoad (try go) + where + -- A fresh load parses/reads from disk and can throw. Fold any exception + -- into the Left this function already returns, so every surface (REST, + -- MCP, CLI) gets one total Either to handle instead of each having to + -- remember its own catch. + flattenLoad :: Either SomeException (Either Text a) -> Either Text a + flattenLoad = either (Left . T.pack . show) id + go = do + -- Pre-load declared dependencies so they're available for cross-DB linking + availableDbs <- readTVarIO (dmAvailableDbs manager) + let configDeps = maybe [] dcDepends (M.lookup dbName availableDbs) + depResults1 <- autoLoadDeps manager configDeps + + result <- loadDatabaseSingle manager dbName + case result of + Left err -> return (Left err) + Right loaded -> do + -- Also auto-load any runtime-discovered dependencies + depResults2 <- autoLoadDeps manager (dbDependsOn (ldDatabase loaded)) + return (Right (loaded, depResults1 ++ depResults2)) {- | Stage an uploaded database (parse + cross-DB link, no matrices yet) When a valid cache exists, reconstructs staged state from the cached Database diff --git a/test/MCPDispatchSpec.hs b/test/MCPDispatchSpec.hs new file mode 100644 index 00000000..b449cfa4 --- /dev/null +++ b/test/MCPDispatchSpec.hs @@ -0,0 +1,77 @@ +{-# LANGUAGE OverloadedStrings #-} + +{- | The database load/unload MCP tools are advertised in 'toolDefinitions' +*and* routed by 'callTool'. The two live in different places (the resource +registry vs. the dispatch case), so a name typo would compile yet strand a +tool at runtime with an "Unknown tool" reply. These tests pin both ends. +-} +module MCPDispatchSpec (spec) where + +import Data.Aeson (Value (..)) +import qualified Data.Aeson.KeyMap as KM +import Data.Foldable (toList) +import Data.Maybe (listToMaybe) +import Data.Text (Text) +import qualified Data.Text as T +import Test.Hspec + +import API.MCP (callTool, toolDefinitions) +import Config (defaultConfig) +import Database.Manager (initDatabaseManager) + +-- | The tool definition advertised under a given MCP name. +toolByName :: Text -> Maybe Value +toolByName name = + listToMaybe + [t | t@(Object o) <- toolDefinitions, KM.lookup "name" o == Just (String name)] + +-- | The 'required' parameter names declared in a tool's input schema. +requiredOf :: Value -> [Text] +requiredOf (Object o) = case KM.lookup "inputSchema" o of + Just (Object s) -> case KM.lookup "required" s of + Just (Array arr) -> [t | String t <- toList arr] + _ -> [] + _ -> [] +requiredOf _ = [] + +{- | The text payload of a tool reply (@result.content[0].text@), or 'Nothing' +when the reply doesn't have that shape — so a malformed reply fails a test +instead of silently passing a @""@ that satisfies any "doesn't contain X". +-} +resultText :: Value -> Maybe Text +resultText v = do + Object o <- Just v + Object r <- KM.lookup "result" o + Array arr <- KM.lookup "content" r + Object c <- listToMaybe (toList arr) + String t <- KM.lookup "text" c + pure t + +-- | Whether a tool reply is flagged as an error. +isError :: Value -> Bool +isError (Object o) = case KM.lookup "result" o of + Just (Object r) -> KM.lookup "isError" r == Just (Bool True) + _ -> False +isError _ = False + +call :: Text -> IO Value +call name = do + manager <- initDatabaseManager defaultConfig True Nothing + callTool manager [] Nothing Null name (KM.singleton "database" (String "no-such-db")) + +spec :: Spec +spec = describe "MCP database load/unload tools" $ do + it "are advertised with a required 'database' parameter" $ do + fmap requiredOf (toolByName "load_database") `shouldBe` Just ["database"] + fmap requiredOf (toolByName "unload_database") `shouldBe` Just ["database"] + + it "are routed by callTool (no 'Unknown tool' gap)" $ do + loadResp <- call "load_database" + unloadResp <- call "unload_database" + resultText loadResp `shouldSatisfy` maybe False (not . T.isPrefixOf "Unknown tool:") + resultText unloadResp `shouldSatisfy` maybe False (not . T.isPrefixOf "Unknown tool:") + + it "surface the engine error when unloading a database that is not loaded" $ do + resp <- call "unload_database" + isError resp `shouldBe` True + resultText resp `shouldSatisfy` maybe False ("Database not loaded:" `T.isInfixOf`) diff --git a/volca.cabal b/volca.cabal index f89ceee1..cff81ac3 100644 --- a/volca.cabal +++ b/volca.cabal @@ -271,6 +271,7 @@ test-suite lca-tests , SimaProWriterSpec , ILCDWriterSpec , MCPSchemaSpec + , MCPDispatchSpec , BatchImpactsSpec , MCPEnrichSpec , MCPColumnarSpec