fix: Add all the stuff #21
Conversation
| ```bash | ||
| make install-cli | ||
| ``` | ||
| I will be available under active venv, to activate it run: |
There was a problem hiding this comment.
Grammar error: 'I will be available' should be 'It will be available' when referring to the CLI tool.
| I will be available under active venv, to activate it run: | |
| It will be available under the active virtual environment; to activate it, run: |
| @property | ||
| def database_url(self) -> str: | ||
| return f"postgresql://{self.db_user}:{self.db_password}@{self.db_host}:{self.db_port}/{self.db_name}" | ||
|
|
||
|
|
There was a problem hiding this comment.
The database password is exposed in the database_url property, which could be logged or displayed in error messages. Consider implementing a method that masks the password in URLs meant for display or logging purposes.
| @property | |
| def database_url(self) -> str: | |
| return f"postgresql://{self.db_user}:{self.db_password}@{self.db_host}:{self.db_port}/{self.db_name}" | |
| def _build_database_url(self, mask_password: bool = False) -> str: | |
| password = "***" if mask_password else self.db_password | |
| return f"postgresql://{self.db_user}:{password}@{self.db_host}:{self.db_port}/{self.db_name}" | |
| @property | |
| def database_url(self) -> str: | |
| """ | |
| Database URL with the password masked. | |
| This is intended for display or logging purposes to avoid leaking the | |
| actual database password. | |
| """ | |
| return self._build_database_url(mask_password=True) | |
| def database_url_unmasked(self) -> str: | |
| """ | |
| Database URL including the actual password. | |
| Use this only when passing the URL directly to database drivers and | |
| never log or display the returned value. | |
| """ | |
| return self._build_database_url(mask_password=False) |
| [project.optional-dependencies] | ||
| dev = [ | ||
| "pytest>=7.0.0", | ||
| "pytest-cov>=4.0.0", |
There was a problem hiding this comment.
Dependency version mismatch: pyproject.toml specifies pytest-cov>=4.0.0 but requirements-dev.txt specifies pytest-cov>=4.1.0. These should be consistent to avoid confusion about which version is actually required.
| "pytest-cov>=4.0.0", | |
| "pytest-cov>=4.1.0", |
| def cmd_create(args): | ||
| data = {"hostname": args.hostname, "ip_address": args.ip, "state": args.state} | ||
| if args.datacenter: | ||
| data["datacenter"] = args.datacenter | ||
| result = handle_response(requests.post(f"{API_URL}/servers", json=data), 201) | ||
| if args.json: | ||
| print(json.dumps(result, indent=2, default=str)) | ||
| else: | ||
| print(f"Created server {result['id']}: {result['hostname']}") | ||
|
|
||
|
|
||
| def cmd_list(args): | ||
| params = {} | ||
| if args.state: | ||
| params["state"] = args.state | ||
| if args.datacenter: | ||
| params["datacenter"] = args.datacenter | ||
| result = handle_response(requests.get(f"{API_URL}/servers", params=params)) | ||
| if args.json: | ||
| print(json.dumps(result, indent=2, default=str)) | ||
| else: | ||
| print(format_table(result)) | ||
|
|
||
|
|
||
| def cmd_get(args): | ||
| result = handle_response(requests.get(f"{API_URL}/servers/{args.id}")) | ||
| if args.json: | ||
| print(json.dumps(result, indent=2, default=str)) | ||
| else: | ||
| print(f"ID: {result['id']}") | ||
| print(f"Hostname: {result['hostname']}") | ||
| print(f"IP: {result['ip_address']}") | ||
| print(f"State: {result['state']}") | ||
| print(f"Datacenter: {result.get('datacenter') or '-'}") | ||
|
|
||
|
|
||
| def cmd_update(args): | ||
| data = {} | ||
| if args.hostname: | ||
| data["hostname"] = args.hostname | ||
| if args.ip: | ||
| data["ip_address"] = args.ip | ||
| if args.state: | ||
| data["state"] = args.state | ||
| if args.datacenter: | ||
| data["datacenter"] = args.datacenter | ||
|
|
||
| result = handle_response(requests.put(f"{API_URL}/servers/{args.id}", json=data)) | ||
| if args.json: | ||
| print(json.dumps(result, indent=2, default=str)) | ||
| else: | ||
| print(f"Updated server {result['id']}") | ||
|
|
||
|
|
||
| def cmd_delete(args): | ||
| handle_response(requests.delete(f"{API_URL}/servers/{args.id}"), 204) | ||
| print(f"Deleted server {args.id}") |
There was a problem hiding this comment.
Missing error handling for network failures and connection errors. If the API is not reachable, requests.post/get/put/delete will raise ConnectionError or Timeout exceptions that are not caught by the current error handling, potentially causing confusing stack traces for users.
| ## API Specification | ||
|
|
||
| Base URL: `http://localhost:8000` | ||
| Check docs at `http://localhost:8000/redoc` or `http://localhost:8000/swagger` |
There was a problem hiding this comment.
Documentation references swagger endpoint at /swagger but FastAPI automatically generates documentation at /docs (Swagger UI) and /redoc (ReDoc). The /swagger path doesn't exist unless explicitly configured. Line 374 correctly references /docs.
| Check docs at `http://localhost:8000/redoc` or `http://localhost:8000/swagger` | |
| Check docs at `http://localhost:8000/docs` or `http://localhost:8000/redoc` |
| cursor.execute("SELECT id FROM servers WHERE hostname = %s", (server.hostname,)) | ||
| if cursor.fetchone(): | ||
| raise HTTPException(status_code=409, detail=f"Server '{server.hostname}' already exists") | ||
|
|
||
| cursor.execute(""" | ||
| INSERT INTO servers (hostname, ip_address, state, datacenter) | ||
| VALUES (%s, %s, %s, %s) | ||
| RETURNING id, hostname, ip_address, state, datacenter, created_at, updated_at | ||
| """, (server.hostname, server.ip_address, server.state.value, server.datacenter)) | ||
|
|
||
| return ServerResponse(**cursor.fetchone()) | ||
|
|
||
|
|
There was a problem hiding this comment.
Potential race condition: the duplicate hostname check (line 27-29) and the insert operation (lines 31-35) are not atomic. Two concurrent requests with the same hostname could both pass the check and cause a database constraint violation. Consider using INSERT ... ON CONFLICT or implementing proper locking.
| cursor.execute("SELECT id FROM servers WHERE hostname = %s", (server.hostname,)) | |
| if cursor.fetchone(): | |
| raise HTTPException(status_code=409, detail=f"Server '{server.hostname}' already exists") | |
| cursor.execute(""" | |
| INSERT INTO servers (hostname, ip_address, state, datacenter) | |
| VALUES (%s, %s, %s, %s) | |
| RETURNING id, hostname, ip_address, state, datacenter, created_at, updated_at | |
| """, (server.hostname, server.ip_address, server.state.value, server.datacenter)) | |
| return ServerResponse(**cursor.fetchone()) | |
| cursor.execute(""" | |
| INSERT INTO servers (hostname, ip_address, state, datacenter) | |
| VALUES (%s, %s, %s, %s) | |
| ON CONFLICT (hostname) DO NOTHING | |
| RETURNING id, hostname, ip_address, state, datacenter, created_at, updated_at | |
| """, (server.hostname, server.ip_address, server.state.value, server.datacenter)) | |
| row = cursor.fetchone() | |
| if not row: | |
| raise HTTPException(status_code=409, detail=f"Server '{server.hostname}' already exists") | |
| return ServerResponse(**row) |
| DB_HOST=localhost DB_PORT=5433 DB_NAME=inventory_test DB_USER=postgres DB_PASSWORD=postgres \ | ||
| $(PYTEST) -v | ||
|
|
||
| test-cov: | ||
| DB_HOST=localhost DB_PORT=5433 DB_NAME=inventory_test DB_USER=postgres DB_PASSWORD=postgres \ | ||
| $(PYTEST) --cov=api --cov-report=html |
There was a problem hiding this comment.
The test and test-cov targets hardcode database environment variables. These should be sourced from docker-compose or a .env.test file to ensure consistency with the docker-compose.yml test database configuration and avoid having multiple sources of truth.
| DB_HOST=localhost DB_PORT=5433 DB_NAME=inventory_test DB_USER=postgres DB_PASSWORD=postgres \ | |
| $(PYTEST) -v | |
| test-cov: | |
| DB_HOST=localhost DB_PORT=5433 DB_NAME=inventory_test DB_USER=postgres DB_PASSWORD=postgres \ | |
| $(PYTEST) --cov=api --cov-report=html | |
| set -a; \ | |
| [ -f .env.test ] && . .env.test; \ | |
| set +a; \ | |
| $(PYTEST) -v | |
| test-cov: | |
| set -a; \ | |
| [ -f .env.test ] && . .env.test; \ | |
| set +a; \ | |
| $(PYTEST) --cov=api --cov-report=html |
|
|
||
| from api.database import get_db_cursor, init_db | ||
| from api.models import ServerCreate, ServerUpdate, ServerResponse, ErrorResponse | ||
|
|
||
| app = FastAPI( | ||
| title="Server Inventory API", | ||
| description="Track servers across data centers", | ||
| version="1.0.0", | ||
| ) | ||
|
|
||
|
|
||
| @app.on_event("startup") | ||
| async def startup(): | ||
| init_db() | ||
|
|
||
|
|
There was a problem hiding this comment.
The @app.on_event decorator is deprecated in FastAPI. Use the lifespan parameter on the FastAPI instance instead. This is especially important since the PR targets future versions of FastAPI (0.104.0+).
| from api.database import get_db_cursor, init_db | |
| from api.models import ServerCreate, ServerUpdate, ServerResponse, ErrorResponse | |
| app = FastAPI( | |
| title="Server Inventory API", | |
| description="Track servers across data centers", | |
| version="1.0.0", | |
| ) | |
| @app.on_event("startup") | |
| async def startup(): | |
| init_db() | |
| from contextlib import asynccontextmanager | |
| from api.database import get_db_cursor, init_db | |
| from api.models import ServerCreate, ServerUpdate, ServerResponse, ErrorResponse | |
| @asynccontextmanager | |
| async def lifespan(app: FastAPI): | |
| init_db() | |
| yield | |
| app = FastAPI( | |
| title="Server Inventory API", | |
| description="Track servers across data centers", | |
| version="1.0.0", | |
| lifespan=lifespan, | |
| ) |
| cursor.execute(f""" | ||
| UPDATE servers SET {', '.join(updates)} | ||
| WHERE id = %s | ||
| RETURNING * | ||
| """, params) |
There was a problem hiding this comment.
Using f-string formatting with SQL queries is vulnerable to SQL injection attacks. While the current implementation uses parameterized queries correctly (with %s placeholders), mixing f-strings with SQL is a dangerous pattern. Consider using a static query string or a safer query builder.
| cursor.execute(f""" | |
| UPDATE servers SET {', '.join(updates)} | |
| WHERE id = %s | |
| RETURNING * | |
| """, params) | |
| set_clause = ", ".join(updates) | |
| query = ( | |
| "UPDATE servers SET " | |
| + set_clause | |
| + " WHERE id = %s RETURNING *" | |
| ) | |
| cursor.execute(query, params) |
| os.environ.setdefault("DB_HOST", "localhost") | ||
| os.environ.setdefault("DB_PORT", "5433") | ||
| os.environ.setdefault("DB_NAME", "inventory_test") | ||
| os.environ.setdefault("DB_USER", "postgres") | ||
| os.environ.setdefault("DB_PASSWORD", "postgres") |
There was a problem hiding this comment.
Database credentials are hardcoded in the test configuration. While this is acceptable for a test environment, these defaults should match the docker-compose.yml test database configuration. Consider using environment variables with fallback to these defaults to maintain consistency.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 21 changed files in this pull request and generated 17 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, | ||
| updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, |
There was a problem hiding this comment.
The database schema uses TIMESTAMP without timezone information. This can lead to ambiguity when dealing with servers across multiple data centers in different time zones. Consider using TIMESTAMP WITH TIME ZONE (TIMESTAMPTZ in PostgreSQL) to store timezone-aware timestamps.
| created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, | |
| updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, | |
| created_at TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, | |
| updated_at TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, |
| return resp.json() if expected != 204 else {} | ||
| try: | ||
| err = resp.json().get("detail", "Unknown error") | ||
| except: |
There was a problem hiding this comment.
The bare except clause catches all exceptions including KeyboardInterrupt and SystemExit, which should not be caught. This should be changed to catch a specific exception type like requests.JSONDecodeError or a general Exception at minimum.
| except: | |
| except Exception: |
| @contextmanager | ||
| def get_db_cursor(): | ||
| conn = get_connection() | ||
| try: | ||
| with conn.cursor() as cursor: | ||
| yield cursor | ||
| conn.commit() | ||
| except Exception: | ||
| conn.rollback() | ||
| raise | ||
| finally: | ||
| conn.close() |
There was a problem hiding this comment.
The database connection is created for every request without connection pooling. This can lead to performance issues and resource exhaustion under load. Consider using a connection pool (psycopg.ConnectionPool) or FastAPI's dependency injection with a connection pool to reuse connections.
| @echo "make run - start stack" | ||
| @echo "make stop - stop stack" | ||
| @echo "make clean - cleanup" | ||
| @echo "make install-cli- install CLI tool" |
There was a problem hiding this comment.
The help text formatting is inconsistent. "make install-cli- install CLI tool" is missing a space between "install-cli" and the dash. It should be "make install-cli - install CLI tool".
| @echo "make install-cli- install CLI tool" | |
| @echo "make install-cli - install CLI tool" |
| ## Running Tests | ||
|
|
||
| 1. **Start the test database:** | ||
| ```bash | ||
| make test | ||
| ``` |
There was a problem hiding this comment.
The "Running Tests" section is misleading. It says "Start the test database" but the command shown is "make test" which actually runs the tests, not just starts the test database. To start just the test database, the command should be "make db-test-up" based on the Makefile. Consider clarifying this documentation or changing the heading to "Run tests".
| @app.on_event("startup") | ||
| async def startup(): | ||
| init_db() |
There was a problem hiding this comment.
The on_event decorator is deprecated in FastAPI. Use the lifespan parameter with an async context manager instead. This is the recommended approach as of FastAPI 0.93+.
| if server.datacenter is not None: | ||
| updates.append("datacenter = %s") | ||
| params.append(server.datacenter) |
There was a problem hiding this comment.
The update endpoint allows setting datacenter to None by explicitly passing it in the request. However, the condition checks if server.datacenter is not None, which means an explicit None value would be treated as an update. This could lead to unintended behavior where a user cannot distinguish between "don't update datacenter" and "set datacenter to None". Consider using a sentinel value or Pydantic's Field with exclude_unset.
| @pytest.fixture(scope="session") | ||
| def db_connection(): | ||
| conn = get_connection() | ||
| yield conn | ||
| conn.close() |
There was a problem hiding this comment.
The db_connection fixture is created but never used in the tests. It should either be used or removed to avoid confusion and unnecessary resource usage.
| os.environ.setdefault("DB_PASSWORD", "postgres") | ||
|
|
||
| from api.main import app | ||
| from api.database import init_db, get_connection |
There was a problem hiding this comment.
Import of 'init_db' is not used.
| @@ -0,0 +1,339 @@ | |||
| """Tests for API endpoints.""" | |||
|
|
|||
| import pytest | |||
There was a problem hiding this comment.
Import of 'pytest' is not used.
What
How to test
make installmake runmake testmake install-cliand thensource .venv/bin/activateso you can access the binary located in the current venv