Skip to content
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

chore: move ApiKey type to backends, remove AccountTier sqlx::Type #1923

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jonaro00
Copy link
Member

  • debloats common
  • prevents mistake of binding an AccountTier and db not finding the type

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR moves the ApiKey type from common to backends module and removes AccountTier's sqlx::Type derivation to reduce common module bloat and prevent database type binding issues.

  • Relocated ApiKey implementation to backends/src/key.rs with proper validation for 16-char alphanumeric keys and secure memory handling via zeroize
  • Removed #[cfg_attr(feature = "persist", derive(sqlx::Type))] from common/src/models/user.rs to prevent direct AccountTier database bindings
  • Updated import paths across codebase to use shuttle_backends::key::ApiKey instead of from common module

11 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile

backends/Cargo.toml Show resolved Hide resolved
backends/src/key.rs Outdated Show resolved Hide resolved
common/Cargo.toml Show resolved Hide resolved
Copy link
Contributor

@oddgrd oddgrd left a comment

Choose a reason for hiding this comment

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

See auth CI failure

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

This PR continues refactoring the ApiKey and AccountTier handling, focusing on database interaction changes in the auth module.

  • Modified auth/src/lib.rs to use key.as_ref() for SQL query binding, with potential panic risk from unwrap() on ApiKey::parse()
  • Updated auth/src/user.rs to handle ApiKey parsing from database rows with explicit string conversion
  • Removed direct sqlx::Type implementation for AccountTier in favor of string-based database interactions

2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

auth/src/user.rs Outdated Show resolved Hide resolved
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

This update focuses on error handling improvements in the auth module, particularly around ApiKey parsing and database interactions.

  • Added improved error handling in auth/src/user.rs for ApiKey parsing from database rows
  • Implemented proper string conversion for database interactions in auth/src/lib.rs
  • Updated error propagation for invalid API key formats in backends/src/key.rs

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

Based on the latest changes and previous reviews, here's my summary of the recent updates:

This update focuses on database interaction and error handling improvements for the ApiKey type in the auth module.

  • Enhanced error handling in auth/src/user.rs by wrapping ApiKey parse errors in sqlx::Error::ColumnDecode for better database error context
  • Implemented sqlx::Type trait for ApiKey in backends/src/key.rs with transparent serialization
  • Added proper string-based database interactions for AccountTier in auth/src/lib.rs using to_string() conversions

The changes look solid with improved error handling and database interaction patterns. The code maintains good test coverage and follows best practices for database operations.

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@jonaro00 jonaro00 requested a review from oddgrd December 2, 2024 23:53
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