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

feat(common): Add utoipa attribute macro types #1924

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

Conversation

fussybeaver
Copy link

@fussybeaver fussybeaver commented Nov 27, 2024

Description of change

Adds utoipa types for the control layer in the new platform.

Used so that we can generate openapi types from code.

How has this been tested? (if applicable)

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 adds OpenAPI schema generation capabilities by implementing the utoipa::ToSchema derive macro across multiple common types and models in the Shuttle platform.

  • Changed response body type in backends/src/metrics.rs from BoxBody to UnsyncBoxBody<bytes::Bytes, axum::Error>, which may impact response processing
  • Missing ToSchema derive macro on DeleteCertificateRequest in common/src/certificate.rs while other similar structs have it added
  • Added utoipa crate v5 with chrono feature as a non-optional dependency in common/Cargo.toml, affecting all builds regardless of feature flags

💡 (2/5) Greptile learns from your feedback when you react with 👍/👎!

9 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile

backends/src/metrics.rs Outdated Show resolved Hide resolved
common/Cargo.toml Outdated Show resolved Hide resolved
common/src/certificate.rs Outdated Show resolved Hide resolved
common/src/models/project.rs Outdated Show resolved Hide resolved
common/src/models/user.rs Outdated Show resolved Hide resolved
common/src/models/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)

Based on the provided information, I'll summarize the key changes in this PR, focusing only on new changes not mentioned in the previous review:

Added utoipa types for OpenAPI schema generation in the Shuttle platform's control layer:

  • Updated MSRV from 1.78.0 to 1.81.0 in .circleci/config.yml to support new utoipa features
  • Added utoipa v5 with chrono feature for OpenAPI schema generation in common/src/lib.rs
  • Changed Docker image version to cimg/rust:1.81.0 for CI builds

Note: I've kept this summary brief and focused only on the most recent changes not covered in the previous review, as requested.

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

Based on the provided information, I'll summarize the key changes in this PR, focusing only on new changes not mentioned in the previous reviews:

Added OpenAPI schema generation support in the Shuttle platform's models:

  • Added ToSchema derive macro to deployment-related types in common/src/models/deployment.rs including DeploymentRequestBeta and BuildArgsBeta
  • Inconsistent ToSchema derive placement in common/src/models/user.rs where AccountTier has it after other attributes
  • Missing ToSchema derive on core resource types in common/src/resource.rs like ProvisionResourceRequest and ShuttleResourceOutput<T>

Note: I've kept this summary focused only on the most recent changes not covered in previous reviews, as requested.

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

backends/src/metrics.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

Based on the provided information, I'll summarize only the new changes not mentioned in previous reviews:

Added OpenAPI schema generation support in the Shuttle platform's models:

  • Added ToSchema derive macro to project-related types in common/src/models/project.rs for ProjectResponseBeta and ProjectListResponseBeta
  • Added ToSchema to logging types in common/src/log.rs for LogItemBeta and LogsResponseBeta
  • Added ToSchema to certificate types in common/src/certificate.rs for AddCertificateRequest and CertificateResponse

Note: I've kept this summary focused only on the most recent changes not covered in previous reviews, as requested.

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

common/Cargo.toml 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

Based on the provided information, I'll summarize only the new changes not mentioned in previous reviews:

Added OpenAPI schema generation support in the Shuttle platform:

  • Added typeshare::typeshare attribute to certificate types in common/src/certificate.rs for cross-language type sharing
  • Added typeshare::typeshare to deployment state types in common/src/deployment.rs for DeploymentStateBeta
  • Updated rust-version to "1.78" in common/Cargo.toml as the base MSRV for Shuttle crates

Note: I've kept this summary focused only on the most recent changes not covered in previous reviews, as requested.

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

common/src/deployment.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)

Based on the provided information, I'll summarize only the new changes not mentioned in previous reviews:

Increased test stability in permit integration tests:

  • Doubled sleep duration after team creation in backends/tests/integration/permit_tests.rs from SLEEP to SLEEP * 2 to fix race condition
  • Added DatabaseInfoBeta struct in common/src/lib.rs with typeshare::typeshare attribute for cross-language type sharing
  • Added ToSchema derive to database types in common/src/lib.rs for OpenAPI schema generation

Note: I've kept this summary focused only on the most recent changes not covered in previous reviews, as requested.

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 provided information, I'll summarize the latest changes not mentioned in previous reviews:

Further increased test stability in permit integration tests:

  • Increased sleep duration after team creation in backends/tests/integration/permit_tests.rs from SLEEP * 2 to SLEEP * 4 (2000ms) to address race conditions
  • Added DatabaseInfoBeta Debug implementation in common/src/lib.rs to prevent password leakage in debug output
  • Added ToSchema derive to DatabaseInfoBeta struct for OpenAPI schema generation

Note: I've kept this summary focused only on the most recent changes not covered in previous reviews, as requested.

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

Copy link
Member

@jonaro00 jonaro00 left a comment

Choose a reason for hiding this comment

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

The permit test is failing on other branches, so no need to fix it here and now

.circleci/config.yml Show resolved Hide resolved
common/src/certificate.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)

Based on the provided information, I'll summarize only the most recent changes not mentioned in previous reviews:

Added OpenAPI schema generation support in the Shuttle platform:

  • Updated MSRV from 1.78 to 1.81 in common/Cargo.toml to support new utoipa features
  • Added utoipa v5 with chrono feature as a non-optional dependency in common/Cargo.toml
  • Removed permit test stability changes as they're being addressed in another branch

Note: I've kept this summary focused only on the most recent changes not covered in previous reviews, as requested.

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 recent changes not mentioned in previous reviews, here's a focused summary of the key updates:

Made utoipa an optional dependency with feature flag support:

  • Changed utoipa to be an optional dependency in common/Cargo.toml with version = "5", features = ["chrono"]
  • Added conditional #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] across model structs
  • Removed direct utoipa imports in favor of feature-gated usage

Note: I've kept this summary brief and focused only on the most recent changes not covered in previous reviews, highlighting just the most important aspects of making utoipa optional.

8 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 recent changes not mentioned in previous reviews, here's a focused summary:

Made utoipa integration more robust with feature flag improvements:

  • Added utoipa feature flag to common/Cargo.toml to control schema generation
  • Consistently applied conditional cfg_attr for ToSchema across all model types
  • Ensured DeleteCertificateRequest in certificate.rs has proper schema generation support

8 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 provided files and previous reviews, I'll focus on the most recent changes not yet mentioned:

Added OpenAPI schema generation support with improved type sharing:

  • Added typeshare::typeshare to DatabaseInfoBeta in common/src/lib.rs for consistent cross-language type definitions
  • Added #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] to CertificateListResponse in common/src/certificate.rs
  • Improved feature flag organization in common/Cargo.toml by grouping utoipa with related optional dependencies

Note: This summary focuses only on the latest changes not covered in previous reviews, maintaining brevity and specificity.

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 recent changes not mentioned in previous reviews, here's a focused summary:

Enhanced OpenAPI schema generation with improved security and type handling:

  • Added conditional ToSchema derive to Secret<T> in common/src/secrets.rs with careful consideration for sensitive data exposure
  • Ensured consistent feature flag usage across all model types with cfg_attr(feature = "utoipa")
  • Added proper schema generation support for resource-related types in common/src/resource.rs

Note: I've kept this summary focused only on the most recent changes not covered in previous reviews, highlighting just the key security and consistency improvements.

9 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

Based on the recent changes not mentioned in previous reviews, I'll provide a focused summary:

Added OpenAPI schema generation with improved type consistency:

  • Added #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] to ResourceTypeBeta in common/src/resource.rs for consistent API documentation
  • Improved feature flag handling in common/src/models/user.rs by placing ToSchema derive before other attributes
  • Added missing format metadata for date-time fields in CertificateResponse for better OpenAPI schema accuracy

Note: I've focused only on the most recent changes not covered in previous reviews, keeping it concise and specific to 3 points as requested.

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

@@ -74,6 +75,7 @@ service = ["chrono/serde", "display", "tracing", "tracing-subscriber", "uuid"]
test-utils = ["wiremock"]
tonic = ["dep:tonic"]
tracing = ["dep:tracing"]
utoipa = ["dep:utoipa"]
Copy link

Choose a reason for hiding this comment

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

logic: utoipa feature only enables the dependency but doesn't include chrono feature which may be needed for the chrono integration

@fussybeaver fussybeaver force-pushed the ND/utoipa branch 3 times, most recently from 763f3f3 to 557167c Compare December 2, 2024 14:01
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

Based on the recent changes not mentioned in previous reviews, I'll provide a focused summary of the latest updates:

Enhanced OpenAPI schema generation with improved type definitions:

  • Added #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] to LogsResponseBeta in common/src/log.rs for consistent API documentation
  • Added typeshare::typeshare attribute to DeploymentStateBeta in common/src/deployment.rs for cross-language type sharing
  • Updated MSRV to 1.81.0 in .circleci/config.yml to support new utoipa features

Note: I've focused only on the most recent changes not covered in previous reviews, keeping it concise and specific to 3 points as requested.

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

Comment on lines 293 to 294
#[serde(tag = "type", content = "content")]
#[typeshare::typeshare]
Copy link

Choose a reason for hiding this comment

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

style: The serde tag/content attributes should be documented in the OpenAPI schema to make the serialization format clear to API consumers.

Comment on lines +18 to 19
#[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))]
pub struct Secret<T: Zeroize>(T);
Copy link

Choose a reason for hiding this comment

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

logic: Ensure utoipa schema generation does not expose sensitive data in API docs. Consider adding schema customization to mask/redact the inner value.

@fussybeaver fussybeaver force-pushed the ND/utoipa branch 2 times, most recently from 41db531 to 4fbc7f2 Compare December 2, 2024 14:07
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

Based on the recent changes not mentioned in previous reviews, I'll provide a focused summary of the latest updates:

Enhanced OpenAPI schema generation with improved type handling:

  • Added ToSchema derive to ProjectUpdateRequestBeta in common/src/models/project.rs for consistent API documentation
  • Added proper schema generation for ProvisionResourceRequest in common/src/resource.rs with feature-gated attributes
  • Improved security handling in common/src/secrets.rs by ensuring Secret<T> schema generation respects redaction

Note: I've focused only on the most recent changes not covered in previous reviews, keeping it concise and specific to 3 points as requested.

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

@@ -318,6 +324,7 @@ pub enum BuildArgsBeta {
}

#[derive(Deserialize, Serialize)]
#[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))]
#[typeshare::typeshare]
pub struct BuildArgsRustBeta {
/// Version of shuttle-runtime used by this crate
Copy link

Choose a reason for hiding this comment

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

style: These fields should have example values in their doc comments to help API consumers understand the expected format.

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

Based on the recent changes not mentioned in previous reviews, I'll provide a focused summary of the latest updates:

Enhanced OpenAPI schema generation with improved type definitions:

  • Added ToSchema derive to ComputeTier in common/src/models/project.rs for consistent API schema generation
  • Added proper schema generation for BuildMetaBeta in common/src/models/deployment.rs with Git metadata support
  • Added ToSchema derive to SubscriptionType in common/src/models/user.rs for API documentation completeness

Note: I've focused only on the most recent changes not covered in previous reviews, keeping it concise and specific to 3 points as requested.

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

@@ -6,7 +6,7 @@ license.workspace = true
repository.workspace = true
description = "Common library for the shuttle platform (https://www.shuttle.rs/)"
# Base MSRV for the Shuttle crates. If some other crate has a higher MSRV, set it in that crate.
rust-version = "1.78"
rust-version = "1.81"
Copy link

Choose a reason for hiding this comment

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

logic: rust-version was decreased from 1.78 to 1.81 - this appears to be a typo since 1.81 is lower than 1.78

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

Based on the recent changes not mentioned in previous reviews, I'll provide a focused summary of the latest updates:

Enhanced OpenAPI schema generation with improved type handling:

  • Added ToSchema derive to DeploymentRequestImageBeta in common/src/models/deployment.rs for container image request documentation
  • Added proper schema generation for ResourceListResponseBeta in common/src/resource.rs for consistent API responses
  • Added ToSchema derive to SecretStore in common/src/secrets.rs for secure secrets management documentation

Note: I've focused only on the most recent changes not covered in previous reviews, keeping it concise and specific to 3 points as requested.

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

common/src/secrets.rs Outdated Show resolved Hide resolved
@fussybeaver fussybeaver force-pushed the ND/utoipa branch 2 times, most recently from e7c37ad to 5c1a2d6 Compare December 3, 2024 15:58
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

Based on the recent changes not mentioned in previous reviews, I'll provide a focused summary of the latest updates:

Enhanced OpenAPI schema generation with improved type consistency:

  • Added ToSchema derive to Backend enum in common/src/log.rs for complete logging system documentation
  • Added proper schema generation for DeploymentRequestBuildArchiveBeta in common/src/models/deployment.rs with build configuration support
  • Improved attribute placement consistency in common/src/models/user.rs by standardizing ToSchema derive order

Note: I've focused only on the most recent changes not covered in previous reviews, keeping it concise and specific to 3 points as requested.

11 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

Error: Error in underlying LLM call. Please try again.

10 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

Based on the recent changes not mentioned in previous reviews, I'll provide a focused summary of the latest updates:

Enhanced OpenAPI schema generation with improved type definitions:

  • Added ToSchema derive to LogsRange enum in common/src/log.rs for complete API documentation of log filtering options
  • Added proper schema generation for BuildArgsRustBeta in common/src/models/deployment.rs with Rust-specific build configuration support
  • Added missing ToSchema derive to DeleteCertificateRequest in common/src/certificate.rs for API consistency

Note: I've kept this summary focused only on the most recent changes not covered in previous reviews, maintaining brevity and specificity with exactly 3 points as requested.

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

Comment on lines +33 to 34
#[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))]
#[typeshare::typeshare]
Copy link

Choose a reason for hiding this comment

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

style: Consider adding OpenAPI description attributes to document the purpose of this response type for API consumers

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

Based on the recent changes not mentioned in previous reviews, I'll provide a focused summary of the latest updates:

Enhanced OpenAPI schema generation with improved type handling:

  • Added ToSchema derive to ResourceInput and ResourceInputBeta in common/src/resource.rs for proper deserialization documentation
  • Added proper schema generation for ShuttleResourceOutput<T> in common/src/resource.rs with generic type support
  • Added ToSchema derive to AccountTier in common/src/models/user.rs with proper serialization attributes

Note: I've kept this summary focused only on the most recent changes not covered in previous reviews, maintaining brevity and specificity with exactly 3 points as requested.

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

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