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

Merged
merged 4 commits into from
Jan 9, 2025

Conversation

jonaro00
Copy link
Member

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

Copy link
Contributor

@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

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
Contributor

@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

Copy link
Contributor

@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
Contributor

@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
Copy link
Contributor

@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 recent changes and previous reviews, here's my summary of the latest updates:

This update focuses on finalizing the ApiKey type relocation and database interaction improvements.

  • Added sqlx::Type derive with transparent attribute for ApiKey in backends/src/key.rs to ensure proper database serialization
  • Implemented zeroize trait for ApiKey to securely handle sensitive data in memory
  • Added property-based testing with proptest for ApiKey validation in backends/src/key.rs

The changes look complete with proper type implementations and security considerations. The code maintains good test coverage and follows best practices for handling sensitive data.

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

@jonaro00 jonaro00 force-pushed the clean-common-apikey branch from e283622 to 6897c7e Compare January 9, 2025 13:51
Copy link
Contributor

@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

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

This update completes the ApiKey type relocation with additional security and testing improvements.

  • Added #[serde(transparent)] attribute to ApiKey in backends/src/key.rs for consistent serialization behavior
  • Implemented AsRef<str> trait for ApiKey to provide safe access to underlying string value
  • Added test cases in backends/src/key.rs to verify API key length and character validation

The changes focus on ensuring proper encapsulation and validation of the ApiKey type while maintaining consistent behavior across the codebase.

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

@jonaro00 jonaro00 merged commit ef1ee64 into main Jan 9, 2025
31 of 33 checks passed
@jonaro00 jonaro00 deleted the clean-common-apikey branch January 9, 2025 14:41
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.

None yet

2 participants