Feature/azure sql integration : Add Azure SQL integration support#2
Feature/azure sql integration : Add Azure SQL integration support#2
Conversation
Add Azure SQL integration to save occupancy data
Add pyodbc dependency for Azure SQL integration
Add Azure SQL support and ODBC driver installation - Installed Microsoft ODBC Driver 18 - Added unixodbc dependencies - Combined RUN commands for proper Docker build layering - Prepared backend for Azure SQL integration
There was a problem hiding this comment.
Pull request overview
Adds Azure SQL persistence support to the Occupancy API so occupancy results can be stored in an Azure SQL table in addition to the existing local/Blob storage paths.
Changes:
- Add
pyodbcdependency for ODBC connectivity. - Add Docker image steps to install Microsoft ODBC Driver 18 (and unixODBC deps).
- Add SQL connection + insert logic in
main.pyand call it during/api/thermalingestion.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| requirements.txt | Adds pyodbc dependency needed for SQL connectivity. |
| main.py | Introduces SQL connection helper + occupancy_data insert on each ingest. |
| Dockerfile | Installs unixODBC + Microsoft ODBC Driver 18 to support pyodbc in-container. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _get_sql_connection(): | ||
| """Create Azure SQL connection. Returns None if not configured.""" | ||
| if not SQL_CONNECTION_STRING: | ||
| return None | ||
| try: | ||
| return pyodbc.connect(SQL_CONNECTION_STRING, timeout=10) | ||
| except Exception as e: |
There was a problem hiding this comment.
A new ODBC connection is created on every call, which can become a significant overhead under load. Consider reusing a cached connection (similar to the blob client pattern), enabling/confirming pyodbc pooling explicitly, and adding a simple backoff/"disable after failure" flag to avoid repeated connection attempts on persistent misconfiguration/outage.
| cursor = conn.cursor() | ||
| cursor.execute( | ||
| """ | ||
| INSERT INTO occupancy_data | ||
| ([timestamp], sensor_id, occupancy, room_temperature, | ||
| people_clusters, fever_count, any_fever) | ||
| VALUES (?, ?, ?, ?, ?, ?, ?) | ||
| """, | ||
| entry["timestamp"], | ||
| entry["sensor_id"], | ||
| entry["occupancy"], | ||
| entry["room_temperature"], | ||
| entry["people_clusters"], | ||
| entry["fever_count"], | ||
| 1 if entry["any_fever"] else 0, | ||
| ) | ||
| conn.commit() | ||
| cursor.close() | ||
| conn.close() | ||
| except Exception as e: | ||
| print(f"Error saving occupancy data to Azure SQL: {e}") | ||
| try: | ||
| conn.close() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
If an exception occurs after creating the cursor, the cursor is never explicitly closed (only the connection is closed in the except). Use a finally block (or context managers) to guarantee both cursor and connection are closed, and consider calling rollback() on failures before closing to avoid leaving open transactions in pooled connections.
| save_thermal_data(compact_data, latest_thermal_data, sensor_id) | ||
| save_occupancy_data(occupancy_result) | ||
| save_occupancy_data_sql(occupancy_result, timestamp_iso=now_iso) |
There was a problem hiding this comment.
PR description mentions "switching" between local JSONL storage and Azure SQL, but the API still always writes local occupancy history when SAVE_THERMAL_DATA is true, and all history/stats endpoints still read only from local files. If Azure SQL is intended to be the backend, either add SQL-backed read paths (or a clear fallback), or update the description/env flags to reflect that SQL is an additional sink rather than a full backend.
Dockerfile
Outdated
| RUN apt-get update && apt-get install -y curl gnupg unixodbc unixodbc-dev && \ | ||
| curl https://packages.microsoft.com/keys/microsoft.asc | apt-key add - && \ | ||
| curl https://packages.microsoft.com/config/debian/12/prod.list > /etc/apt/sources.list.d/mssql-release.list && \ | ||
| apt-get update && ACCEPT_EULA=Y apt-get install -y msodbcsql18 && \ |
There was a problem hiding this comment.
Using apt-key add - is deprecated on modern Debian and may break in future base images. Import the Microsoft key into /etc/apt/keyrings/ and reference it via signed-by= in the repo list instead. Also consider --no-install-recommends and cleaning /var/lib/apt/lists/* after installs to keep the image smaller and reduce attack surface.
| RUN apt-get update && apt-get install -y curl gnupg unixodbc unixodbc-dev && \ | |
| curl https://packages.microsoft.com/keys/microsoft.asc | apt-key add - && \ | |
| curl https://packages.microsoft.com/config/debian/12/prod.list > /etc/apt/sources.list.d/mssql-release.list && \ | |
| apt-get update && ACCEPT_EULA=Y apt-get install -y msodbcsql18 && \ | |
| RUN apt-get update && apt-get install -y --no-install-recommends curl gnupg ca-certificates unixodbc unixodbc-dev && \ | |
| mkdir -p /etc/apt/keyrings && \ | |
| curl https://packages.microsoft.com/keys/microsoft.asc | gpg --dearmor -o /etc/apt/keyrings/microsoft.gpg && \ | |
| chmod 644 /etc/apt/keyrings/microsoft.gpg && \ | |
| curl https://packages.microsoft.com/config/debian/12/prod.list | sed 's#^deb #deb [signed-by=/etc/apt/keyrings/microsoft.gpg] #' > /etc/apt/sources.list.d/mssql-release.list && \ | |
| apt-get update && ACCEPT_EULA=Y apt-get install -y --no-install-recommends msodbcsql18 && \ | |
| rm -rf /var/lib/apt/lists/* && \ |
main.py
Outdated
| import json | ||
| import os | ||
| import pyodbc |
There was a problem hiding this comment.
Importing pyodbc at module import time can prevent the API from starting in deployments where the ODBC runtime libraries aren’t present (common outside the Docker image), even if SQL saving is disabled. To keep SQL support optional, consider moving the import inside _get_sql_connection() and handling ImportError by disabling SQL saving with a clear (sanitized) log message.
Dockerfile
Outdated
| @@ -5,7 +5,11 @@ WORKDIR /app | |||
|
|
|||
| # Install dependencies (no build tools needed for current deps) | |||
There was a problem hiding this comment.
The comment says "no build tools needed for current deps", but this layer now installs build/runtime dependencies (unixODBC, msodbcsql18). Update the comment to reflect the new rationale (ODBC driver + pyodbc support) to avoid confusion for future maintenance.
| # Install dependencies (no build tools needed for current deps) | |
| # Install system deps for ODBC/SQL Server (unixODBC, msodbcsql18) and Python requirements |
| try: | ||
| return pyodbc.connect(SQL_CONNECTION_STRING, timeout=10) | ||
| except Exception as e: | ||
| print(f"Azure SQL connection failed: {e}") | ||
| return None |
There was a problem hiding this comment.
Avoid printing raw SQL connection exceptions here. pyodbc error strings can include server/user details (and in some cases parts of the connection string), which can leak sensitive info into logs. Prefer structured logging and a sanitized message (e.g., log an error code/class, or gate detailed errors behind a debug flag).
|
@copilot open a new pull request to apply changes based on the comments in this thread |
…lazy import, and error handling Co-authored-by: mandeeps <3266584+mandeeps@users.noreply.github.com>
Fix Azure SQL integration: connection caching, lazy import, safe error handling, and Dockerfile hardening
This pull request adds Azure SQL support to the API.
Changes:
This allows switching between local JSONL storage and Azure SQL.