Skip to content

Conversation

@jcabrero
Copy link
Member

@jcabrero jcabrero commented Nov 4, 2025

Pull Request Overview

This PR implements a major refactoring of the authentication and user management system, transitioning from a database-driven user model to a credential validation middleware pattern. Key changes include renaming userid to user_id throughout the codebase, integrating nilauth-credit-middleware for credential validation, restructuring the database schema to remove user-specific token tracking, and introducing a new QueryLogContext dependency pattern for improved request logging.

  • Integrates nilauth-credit-middleware v0.1.2 for centralized credential validation
  • Refactors database schema: renames userid to user_id, removes user token counters, adds query log telemetry fields
  • Introduces QueryLogContext as a FastAPI dependency for better request lifecycle tracking
  • Updates rate limiting to use user_id instead of subscription_holder
  • Removes subscription owner checks from prompt delegation endpoint

@jcabrero jcabrero changed the base branch from main to chore/add_native_client_to_tests November 4, 2025 15:20
@jcabrero jcabrero requested a review from Copilot November 5, 2025 10:22
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a major refactoring of the authentication and user management system, transitioning from a database-driven user model to a credential validation middleware pattern. Key changes include renaming userid to user_id throughout the codebase, integrating nilauth-credit-middleware for credential validation, restructuring the database schema to remove user-specific token tracking, and introducing a new QueryLogContext dependency pattern for improved request logging.

  • Integrates nilauth-credit-middleware v0.1.2 for centralized credential validation
  • Refactors database schema: renames userid to user_id, removes user token counters, adds query log telemetry fields
  • Introduces QueryLogContext as a FastAPI dependency for better request lifecycle tracking
  • Updates rate limiting to use user_id instead of subscription_holder
  • Removes subscription owner checks from prompt delegation endpoint

Reviewed Changes

Copilot reviewed 29 out of 32 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
uv.lock Updates dependencies: nilauth-credit-middleware 0.1.1→0.1.2, secretvaults source changed
nilai-api/src/nilai_api/db/users.py Simplifies UserModel to only user_id and rate_limits; removes token tracking methods
nilai-api/src/nilai_api/db/logs.py Adds QueryLogContext for request lifecycle logging; adds telemetry columns to QueryLog
nilai-api/src/nilai_api/auth/strategies.py Integrates validate_credential function using nilauth-credit-middleware
nilai-api/src/nilai_api/routers/private.py Adds QueryLogContext dependency; improves error handling and logging
nilai-api/src/nilai_api/rate_limiting.py Renames subscription_holder to user_id; removes wait_for_bucket method
tests/* Updates all tests to use user_id instead of userid
nilai-api/alembic/versions/*.py Database migration scripts for schema changes
grafana/runtime-data/dashboards/*.json Updates Grafana queries to use user_id column
Comments suppressed due to low confidence (2)

tests/integration/nilai_api/test_users_db_integration.py:1

  • The test attempts to access user.name and user.apikey attributes, but these fields were removed from the UserModel. This test will fail with AttributeError. The assertions should be updated to reflect the new UserModel schema that only contains user_id and rate_limits.
"""

nilai-api/src/nilai_api/config/init.py:67

  • Print statement may execute during import.
print(CONFIG.prettify())

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

]

logging.info(CONFIG.prettify())
print(CONFIG.prettify())
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

Adding a print statement for configuration output is problematic in production code. This will clutter logs and should be removed or made conditional based on environment/debug settings.

Suggested change
print(CONFIG.prettify())

Copilot uses AI. Check for mistakes.
from sqlalchemy.dialects import postgresql

# revision identifiers, used by Alembic.
revision: str = "0ba073468afc"
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

The global variable 'revision' is not used.

Copilot uses AI. Check for mistakes.
revision: str = "0ba073468afc"
down_revision: Union[str, None] = "9ddf28cf6b6f"
branch_labels: Union[str, Sequence[str], None] = None
depends_on: Union[str, Sequence[str], None] = None
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

The global variable 'depends_on' is not used.

Suggested change
depends_on: Union[str, Sequence[str], None] = None

Copilot uses AI. Check for mistakes.


# revision identifiers, used by Alembic.
revision: str = "43b23c73035b"
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

The global variable 'revision' is not used.

Copilot uses AI. Check for mistakes.

# revision identifiers, used by Alembic.
revision: str = "43b23c73035b"
down_revision: Union[str, None] = "0ba073468afc"
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

The global variable 'down_revision' is not used.

Copilot uses AI. Check for mistakes.
# revision identifiers, used by Alembic.
revision: str = "43b23c73035b"
down_revision: Union[str, None] = "0ba073468afc"
branch_labels: Union[str, Sequence[str], None] = None
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

The global variable 'branch_labels' is not used.

Suggested change
branch_labels: Union[str, Sequence[str], None] = None

Copilot uses AI. Check for mistakes.
revision: str = "43b23c73035b"
down_revision: Union[str, None] = "0ba073468afc"
branch_labels: Union[str, Sequence[str], None] = None
depends_on: Union[str, Sequence[str], None] = None
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

The global variable 'depends_on' is not used.

Suggested change
depends_on: Union[str, Sequence[str], None] = None

Copilot uses AI. Check for mistakes.
@jcabrero jcabrero force-pushed the chore/improve_db_structure branch from 8b6ce65 to dd374dd Compare November 5, 2025 10:41
@jcabrero jcabrero force-pushed the chore/improve_db_structure branch from bfa83b9 to fabf9fd Compare November 13, 2025 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants