Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 73 additions & 1 deletion .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,80 @@ jobs:

- name: Run unit tests
run: |
pipenv run pytest tests/ -k "not e2e" --verbose
pipenv run pytest tests/ -k "not e2e and not test_sdk" --verbose

- name: Run lint
run: |
make lint

sdk-tests:
runs-on: ubuntu-latest

services:
falkordb:
image: falkordb/falkordb:latest
ports:
- 6379:6379
options: >-
--health-cmd "redis-cli ping"
--health-interval 10s
--health-timeout 5s
--health-retries 5

postgres:
image: postgres:15
env:
POSTGRES_USER: postgres
POSTGRES_PASSWORD: postgres
POSTGRES_DB: testdb
ports:
- 5432:5432
options: >-
--health-cmd pg_isready
--health-interval 10s
--health-timeout 5s
--health-retries 5

mysql:
image: mysql:8
env:
MYSQL_ROOT_PASSWORD: root
MYSQL_DATABASE: testdb
ports:
- 3306:3306
options: >-
--health-cmd "mysqladmin ping"
--health-interval 10s
--health-timeout 5s
--health-retries 5

steps:
- uses: actions/checkout@v4

- name: Set up Python
uses: actions/setup-python@v5
with:
python-version: '3.12'

- name: Install pipenv
run: |
python -m pip install --upgrade pip
pip install pipenv

- name: Install dependencies
run: |
pipenv sync --dev

- name: Create test environment file
run: |
cp .env.example .env
echo "FASTAPI_SECRET_KEY=test-secret-key" >> .env

Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
- name: Run SDK tests
env:
FALKORDB_URL: redis://localhost:6379
TEST_POSTGRES_URL: postgresql://postgres:postgres@localhost:5432/testdb
TEST_MYSQL_URL: mysql://root:root@localhost:3306/testdb
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
run: |
pipenv run pytest tests/test_sdk/ -v

Copilot AI Feb 16, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SDK tests run unconditionally even if OPENAI_API_KEY is not set. Looking at tests/test_sdk/conftest.py, the has_llm_key fixture will skip tests that require an LLM key, but the job will still run and potentially waste CI minutes. Consider adding a conditional check or making the job continue-on-error when the secret is not available, or split the tests into those that require API keys and those that don't.

Copilot uses AI. Check for mistakes.
71 changes: 58 additions & 13 deletions Makefile
Original file line number Diff line number Diff line change
@@ -1,19 +1,37 @@
.PHONY: help install test test-unit test-e2e test-e2e-headed lint format clean setup-dev build lint-frontend
.PHONY: help install test test-unit test-e2e test-e2e-headed lint format clean setup-dev build lint-frontend test-sdk docker-test-services docker-test-stop build-package

help: ## Show this help message
@echo 'Usage: make [target]'
@echo ''
@echo 'Targets:'
@awk 'BEGIN {FS = ":.*?## "} /^[a-zA-Z_-]+:.*?## / {printf " %-15s %s\n", $$1, $$2}' $(MAKEFILE_LIST)

install: ## Install dependencies
@awk 'BEGIN {FS = ":.*?## "} /^[a-zA-Z_-]+:.*?## / {printf " %-20s %s\n", $$1, $$2}' $(MAKEFILE_LIST)

# Check if uv is available, fallback to pipenv
UV := $(shell command -v uv 2> /dev/null)
ifdef UV
PKG_MANAGER = uv
PIP_CMD = uv pip
SYNC_CMD = uv sync
RUN_CMD = uv run
else
PKG_MANAGER = pipenv
PIP_CMD = pipenv run pip
SYNC_CMD = pipenv sync --dev
RUN_CMD = pipenv run
endif

install: ## Install dependencies (uses uv if available, else pipenv)
ifdef UV
uv sync --all-extras
else
pipenv sync --dev
endif
npm install --prefix ./app


setup-dev: install ## Set up development environment
pipenv run playwright install chromium
pipenv run playwright install-deps
$(RUN_CMD) playwright install chromium
$(RUN_CMD) playwright install-deps
@echo "Development environment setup complete!"
@echo "Don't forget to copy .env.example to .env and configure your settings"

Expand All @@ -23,26 +41,34 @@ build-dev:
build-prod:
npm --prefix ./app run build

build-package: ## Build distributable package (wheel + sdist)
ifdef UV
uv build
else
pip install build && python -m build
endif
@echo "Built packages in dist/"

test: build-dev test-unit test-e2e ## Run all tests

test-unit: ## Run unit tests only
pipenv run python -m pytest tests/ -k "not e2e" --verbose
$(RUN_CMD) python -m pytest tests/ -k "not e2e" --verbose
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved


test-e2e: build-dev ## Run E2E tests headless
pipenv run python -m pytest tests/e2e/ --browser chromium --video=on --screenshot=on
$(RUN_CMD) python -m pytest tests/e2e/ --browser chromium --video=on --screenshot=on


test-e2e-headed: build-dev ## Run E2E tests with browser visible
pipenv run python -m pytest tests/e2e/ --browser chromium --headed
$(RUN_CMD) python -m pytest tests/e2e/ --browser chromium --headed


test-e2e-debug: build-dev ## Run E2E tests with debugging enabled
pipenv run python -m pytest tests/e2e/ --browser chromium --slowmo=1000
$(RUN_CMD) python -m pytest tests/e2e/ --browser chromium --slowmo=1000

lint: ## Run linting (backend + frontend)
@echo "Running backend lint (pylint)"
pipenv run pylint $(shell git ls-files '*.py') || true
$(RUN_CMD) pylint $(shell git ls-files '*.py') || true
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
@echo "Running frontend lint (eslint)"
make lint-frontend

Expand All @@ -57,18 +83,37 @@ clean: ## Clean up test artifacts
rm -rf playwright-report/
rm -rf tests/e2e/screenshots/
rm -rf __pycache__/
rm -rf dist/
rm -rf *.egg-info/
find . -name "*.pyc" -delete
find . -name "*.pyo" -delete

run-dev: build-dev ## Run development server
pipenv run uvicorn api.index:app --host $${HOST:-127.0.0.1} --port $${PORT:-5000} --reload
$(RUN_CMD) uvicorn api.index:app --host $${HOST:-127.0.0.1} --port $${PORT:-5000} --reload

run-prod: build-prod ## Run production server
pipenv run uvicorn api.index:app --host $${HOST:-0.0.0.0} --port $${PORT:-5000}
$(RUN_CMD) uvicorn api.index:app --host $${HOST:-0.0.0.0} --port $${PORT:-5000}

docker-falkordb: ## Start FalkorDB in Docker for testing
docker run -d --name falkordb-test -p 6379:6379 falkordb/falkordb:latest

docker-stop: ## Stop test containers
docker stop falkordb-test || true
docker rm falkordb-test || true

# SDK Testing
docker-test-services: ## Start all test services (FalkorDB + PostgreSQL + MySQL)
docker compose -f docker-compose.test.yml up -d
@echo "Waiting for services to be ready..."
@sleep 10

docker-test-stop: ## Stop all test services
docker compose -f docker-compose.test.yml down -v

test-sdk: ## Run SDK integration tests (requires docker-test-services)
$(RUN_CMD) pytest tests/test_sdk/ -v

test-sdk-quick: ## Run SDK tests without LLM (models and connection only)
$(RUN_CMD) pytest tests/test_sdk/test_queryweaver.py::TestModels tests/test_sdk/test_queryweaver.py::TestQueryWeaverInit -v

test-all: test-unit test-sdk test-e2e ## Run all tests
Comment on lines 30 to +97

Copilot AI Feb 16, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unit tests are filtered with -k "not e2e and not test_sdk" which means SDK tests are excluded from the unit test run. However, the new test-all target runs test-unit test-sdk test-e2e sequentially. This means the SDK tests are run separately but might overlap with unit test concerns. Consider clarifying the test taxonomy - are SDK tests integration tests, or a separate category? The filtering suggests they should be kept separate, which is sensible, but the documentation doesn't explain why.

Copilot uses AI. Check for mistakes.
Comment on lines +91 to +97

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

test-all doesn't satisfy test-sdk's prerequisites.

On a clean checkout, make test-all reaches test-sdk without starting Postgres/MySQL/FalkorDB, so the aggregate target fails unless the user remembered to run docker-test-services first.

♻️ Proposed fix
-test-sdk: ## Run SDK integration tests (requires docker-test-services)
+test-sdk: docker-test-services ## Run SDK integration tests
 	uv run python -m pytest tests/test_sdk/ -v
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test-sdk: ## Run SDK integration tests (requires docker-test-services)
uv run python -m pytest tests/test_sdk/ -v
test-sdk-quick: ## Run SDK tests without LLM (models and connection only)
uv run python -m pytest tests/test_sdk/test_queryweaver.py::TestModels tests/test_sdk/test_queryweaver.py::TestQueryWeaverInit -v
test-all: test-unit test-sdk test-e2e ## Run all tests
test-sdk: docker-test-services ## Run SDK integration tests
uv run python -m pytest tests/test_sdk/ -v
test-sdk-quick: ## Run SDK tests without LLM (models and connection only)
uv run python -m pytest tests/test_sdk/test_queryweaver.py::TestModels tests/test_sdk/test_queryweaver.py::TestQueryWeaverInit -v
test-all: test-unit test-sdk test-e2e ## Run all tests
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Makefile` around lines 91 - 97, The test-all target currently runs test-sdk
without ensuring docker-test-services are started; update the Makefile so
test-all depends on docker-test-services (or add docker-test-services as a
prerequisite to test-sdk) so that running make test-all will first invoke
docker-test-services before test-sdk; modify the target dependencies referencing
the existing Makefile targets test-all, test-sdk, and docker-test-services
accordingly to guarantee services start on a clean checkout.

73 changes: 73 additions & 0 deletions api/core/schema_loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@
from typing import AsyncGenerator, Optional

from pydantic import BaseModel
from redis import RedisError

from api.extensions import db

from api.core.errors import InvalidArgumentError
from api.loaders.base_loader import BaseLoader
from api.loaders.postgres_loader import PostgresLoader
from api.loaders.mysql_loader import MySQLLoader
from queryweaver_sdk.models import DatabaseConnection

# Use the same delimiter as in the JavaScript frontend for streaming chunks
MESSAGE_DELIMITER = "|||FALKORDB_MESSAGE_BOUNDARY|||"
Expand Down Expand Up @@ -162,3 +164,74 @@ async def list_databases(user_id: str, general_prefix: Optional[str] = None) ->
filtered_graphs = filtered_graphs + demo_graphs

return filtered_graphs


# =============================================================================
# SDK Non-Streaming Functions
# =============================================================================

async def load_database_sync(url: str, user_id: str):
"""
Load a database schema and return structured result (non-streaming).

SDK-friendly version that returns DatabaseConnection instead of streaming.

Args:
url: Database connection URL (PostgreSQL or MySQL).
user_id: User identifier for namespacing.

Returns:
DatabaseConnection with connection status.
"""
# Validate URL format
if not url or len(url.strip()) == 0:
raise InvalidArgumentError("Invalid URL format")

# Determine database type and loader
loader: type[BaseLoader] = BaseLoader
if url.startswith("postgres://") or url.startswith("postgresql://"):
loader = PostgresLoader
elif url.startswith("mysql://"):
loader = MySQLLoader
else:
raise InvalidArgumentError("Invalid database URL format. Must be PostgreSQL or MySQL.")

tables_loaded = 0
last_message = ""
success = False

try:
async for progress_success, progress_message in loader.load(user_id, url):
success = progress_success
last_message = progress_message
if success and "table" in progress_message.lower():
# Try to extract table count from message
tables_loaded += 1
Comment on lines +203 to +207

Copilot AI Feb 16, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The table counting logic is flawed. The code increments tables_loaded for every progress message containing "table" (case-insensitive), but schema loaders emit multiple messages per table (e.g., "Loading table X", "Processed table X columns", etc.). This will result in an inflated count that doesn't represent the actual number of tables. Consider extracting the count from the final message or tracking unique table names instead.

Copilot uses AI. Check for mistakes.

if success:
# Extract database name from the message or URL
# The loader typically returns the graph_id in the final message
db_name = url.split("/")[-1].split("?")[0] # Extract DB name from URL

return DatabaseConnection(
database_id=db_name,
success=True,
tables_loaded=tables_loaded,
message="Database connected and schema loaded successfully",
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved

return DatabaseConnection(
database_id="",
success=False,
tables_loaded=0,
message=last_message or "Failed to load database schema",
)

except (RedisError, ConnectionError, OSError) as e:
logging.exception("Error loading database: %s", str(e))
return DatabaseConnection(
database_id="",
success=False,
tables_loaded=0,
message=f"Error connecting to database: {str(e)}",
)
Comment on lines +228 to +235

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid returning raw exception text (may include credentials).

str(e) can include connection strings or user info. Prefer a generic message and keep the detailed error only in logs.

🔧 Proposed fix
         return DatabaseConnection(
             database_id="",
             success=False,
             tables_loaded=0,
-            message=f"Error connecting to database: {str(e)}",
+            message="Error connecting to database",
         )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
except (RedisError, ConnectionError, OSError) as e:
logging.exception("Error loading database: %s", str(e))
return DatabaseConnection(
database_id="",
success=False,
tables_loaded=0,
message=f"Error connecting to database: {str(e)}",
)
except (RedisError, ConnectionError, OSError) as e:
logging.exception("Error loading database: %s", str(e))
return DatabaseConnection(
database_id="",
success=False,
tables_loaded=0,
message="Error connecting to database",
)
🧰 Tools
🪛 Ruff (0.14.14)

[warning] 231-231: Redundant exception object included in logging.exception call

(TRY401)


[warning] 236-236: Use explicit conversion flag

Replace with conversion flag

(RUF010)

🤖 Prompt for AI Agents
In `@api/core/schema_loader.py` around lines 230 - 237, The except block currently
embeds raw exception text (str(e)) into the DatabaseConnection.message which may
leak credentials; instead, update the except handler that catches (RedisError,
ConnectionError, OSError) to log the full exception (logging.exception already
does this) but return a generic error message in the DatabaseConnection (e.g.,
"Error connecting to database") and preserve success=False, tables_loaded=0;
reference the DatabaseConnection construction in this except block and the
exception variable e when making the change.

4 changes: 2 additions & 2 deletions api/core/text2sql.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import time

from pydantic import BaseModel
from redis import ResponseError
from redis import ResponseError, RedisError

from api.core.errors import GraphNotFoundError, InternalError, InvalidArgumentError
from api.core.schema_loader import load_database
Expand Down Expand Up @@ -936,6 +936,6 @@ async def delete_database(user_id: str, graph_id: str):
return {"success": True, "graph": graph_id}
except ResponseError as re:
raise GraphNotFoundError("Failed to delete graph, Graph not found") from re
except Exception as e: # pylint: disable=broad-exception-caught
except (RedisError, ConnectionError) as e:
logging.exception("Failed to delete graph %s: %s", sanitize_log_input(namespaced), e)
raise InternalError("Failed to delete graph") from e
Loading
Loading