-
Notifications
You must be signed in to change notification settings - Fork 22
fix: ingest resolves a schema scope and qualifies non-default schemas (DEV-1758) #294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 6 commits
704b09f
558c06d
fb4762e
6a3d49e
757d2d4
6212820
ae9939c
73a9084
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ | |
| from typing import Any | ||
|
|
||
| from fastapi import FastAPI, HTTPException | ||
| from pydantic import BaseModel, ConfigDict, Field | ||
| from pydantic import BaseModel, ConfigDict, Field, model_validator | ||
|
|
||
| from slayer.mcp.server import create_mcp_server | ||
| from slayer.core.errors import ( | ||
|
|
@@ -103,11 +103,28 @@ class IngestRequest(BaseModel): | |
| datasource: str | ||
| include_tables: list[str] | None = None | ||
| exclude_tables: list[str] | None = None | ||
| # Kept for backward compatibility; folded into ``schemas=[schema_name]``. | ||
| schema_name: str | None = None | ||
| schemas: list[str] | None = None | ||
| all_schemas: bool = False | ||
| # Ingest recognised ELT/migration internals visible rather than hidden. | ||
| # Governs models this call creates; unhide an existing one via edit_model. | ||
| surface_internals: bool = False | ||
|
|
||
| @model_validator(mode="after") | ||
| def _one_way_to_say_it(self) -> "IngestRequest": | ||
| """Reject conflicting scope arguments at the edge, so every caller of | ||
| the endpoint gets the engine's rule (not whichever the handler reads | ||
| first).""" | ||
| from slayer.engine.ingestion import _resolve_scope_args | ||
|
|
||
| _resolve_scope_args( | ||
| schema=self.schema_name, | ||
| schemas=self.schemas, | ||
| all_schemas=self.all_schemas, | ||
| ) | ||
| return self | ||
|
Comment on lines
+114
to
+126
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Keep new imports at module scope. The changed code adds ingestion imports inside request and tool handlers. Move them to module scope. If an import cycle blocks this, extract the shared resolver into a dependency-neutral module.
As per coding guidelines: “Keep imports at the top of files.” 📍 Affects 2 files
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
|
|
||
| class ValidateModelsRequest(BaseModel): | ||
| data_source: str | None = None | ||
|
|
@@ -653,6 +670,8 @@ async def ingest(request: IngestRequest) -> dict[str, Any]: | |
| include_tables=request.include_tables, | ||
| exclude_tables=request.exclude_tables, | ||
| schema=request.schema_name, | ||
| schemas=request.schemas, | ||
| all_schemas=request.all_schemas, | ||
| surface_internals=request.surface_internals, | ||
| ) | ||
| except SQLAlchemyError as exc: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the contradictory lead-in sentence.
Line 107 starts with "With neither flag" and then lists
--schema/--all-schemasas the first precedence rule. The two statements conflict. State the precedence order without the "neither flag" condition.📝 Proposed wording fix
📝 Committable suggestion
🤖 Prompt for AI Agents