refactor: make versions infos endoint consistent with mehari #579
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve significant updates to the project's CI workflow, project description, and data handling mechanisms. A new job named Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant Schema Job
participant Protobuf Compiler
participant OpenAPI Schema
GitHub Actions->>Schema Job: Trigger on Formatting completion
Schema Job->>Protobuf Compiler: Install dependencies
Schema Job->>Protobuf Compiler: Generate OpenAPI schema
Schema Job->>OpenAPI Schema: Copy and modify schema
Schema Job->>OpenAPI Schema: Compare with existing schema
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (9)
src/server/run/error.rs (1)
6-8: Add documentation for serialization format.Since this error type is now serializable and part of the API schema, it would be helpful to document the expected JSON format with an example.
/// Custom error type for the Actix server. +/// +/// # Serialization Format +/// +/// When serialized to JSON, the error will have the following format: +/// +/// ```json +/// { +/// "err": "error message" +/// } +/// ``` #[derive(Debug, Clone, serde::Serialize, serde::Deserialize, utoipa::ToSchema)] pub struct CustomError { err: String,.github/workflows/rust.yml (1)
133-142: Enhance schema comparison robustness.The current schema comparison process could be improved in several ways:
- Consider using a more robust YAML parsing tool instead of perl for version stripping
- Add error handling for the diff command
- Save artifacts when comparison fails for easier debugging
- name: Write schema run: cargo run -- server schema --output-file /tmp/openapi.schema.yaml-gen - name: Copy repo schema and strip versions run: | cp openapi.schema.yaml /tmp/openapi.schema.yaml-repo - perl -p -i -e 's/^ version: .*/ version: 0.0.0/' /tmp/*.yaml-* + # Use yq for more robust YAML manipulation + for file in /tmp/*.yaml-*; do + yq e '.version = "0.0.0"' -i "$file" + done - name: Compare YAML in git to the one just generated - run: diff /tmp/openapi.schema.yaml-repo /tmp/openapi.schema.yaml-gen + run: | + if ! diff -u /tmp/openapi.schema.yaml-repo /tmp/openapi.schema.yaml-gen; then + echo "Schema files differ! Check the diff output above." + exit 1 + fi + - name: Upload schema files on failure + if: failure() + uses: actions/upload-artifact@v3 + with: + name: schema-files + path: | + /tmp/openapi.schema.yaml-repo + /tmp/openapi.schema.yaml-genopenapi.schema.yaml (4)
1-10: Consider enhancing API documentation metadata.The basic metadata is good, but consider adding:
termsOfServiceURL- Extended documentation URLs in the
externalDocsfield- API status/maturity level in the description
🧰 Tools
🪛 checkov
[HIGH] 1-173: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[HIGH] 1-173: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
149-151: Add format specification for date field.The date field should specify its format for consistent parsing.
Apply this change:
date: type: string + format: date-time description: Date of the data.
152-154: Add pattern validation for version field.The version field should enforce a consistent version format.
Apply this change:
version: type: string + pattern: '^[0-9]+\.[0-9]+\.[0-9]+$' description: Version of the data.
1-171: Consider implementing API versioning strategy.While the endpoint is under
/api/v1/, the OpenAPI schema should document:
- API versioning policy
- Deprecation policy
- Breaking changes policy
This will help maintain backward compatibility as the API evolves.
🧰 Tools
🪛 checkov
[HIGH] 1-173: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[HIGH] 1-173: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
src/server/run/versions.rs (1)
197-197: Consider revising the operation_id.The
operation_id"versionsInfo" doesn't follow common API naming conventions. Consider using "getVersionsInfo" or "listVersionsInfo" to better reflect the operation type.src/genes/cli/import.rs (2)
Line range hint
77-449: Consider buffering improvements for file reading operations.While the implementation is solid, consider using larger buffer sizes for file reading operations to potentially improve performance when dealing with large files.
Apply this pattern to the file reading operations:
- let reader = std::fs::File::open(path).map(std::io::BufReader::new)?; + let reader = std::fs::File::open(path).map(|file| std::io::BufReader::with_capacity(32 * 1024, file))?;
Line range hint
1161-1279: Enhance test coverage with edge cases.The smoke tests are good but consider adding:
- Edge case tests with malformed input files
- Tests for error handling scenarios
- Tests with different file sizes to verify performance
Would you like me to help generate additional test cases to improve coverage?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
.github/workflows/rust.yml(1 hunks)Cargo.toml(1 hunks)build.rs(0 hunks)openapi.schema.yaml(1 hunks)protos/annonars/common/versions.proto(0 hunks)src/common/cli.rs(1 hunks)src/genes/cli/data.rs(1 hunks)src/genes/cli/import.rs(1 hunks)src/gnomad_nuclear/cli/import.rs(1 hunks)src/pbs/common.rs(0 hunks)src/pbs/mod.rs(0 hunks)src/server/run/error.rs(2 hunks)src/server/run/mod.rs(5 hunks)src/server/run/versions.rs(6 hunks)
💤 Files with no reviewable changes (4)
- build.rs
- protos/annonars/common/versions.proto
- src/pbs/common.rs
- src/pbs/mod.rs
🧰 Additional context used
🪛 actionlint
.github/workflows/rust.yml
123-123: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 checkov
openapi.schema.yaml
[HIGH] 1-173: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[HIGH] 1-173: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
🔇 Additional comments (20)
src/server/run/error.rs (2)
6-6: LGTM! Trait derivations support API documentation.
The added derive macros (Clone, Serialize, Deserialize, ToSchema) align well with the PR objective of making the endpoint consistent and enable proper OpenAPI schema generation.
8-8: Consider preserving error context.
Converting anyhow::Error to String loses the error context and stack traces, which might make debugging more difficult in production. Consider either:
- Storing both the string representation and the original error
- Adding debug logging before conversion
pub struct CustomError {
err: String,
+ #[serde(skip)]
+ context: Option<anyhow::Error>,
}
impl CustomError {
pub fn new(err: anyhow::Error) -> Self {
+ tracing::debug!(?err, "Creating CustomError");
CustomError {
err: err.to_string(),
+ context: Some(err),
}
}
}Also applies to: 20-22
Cargo.toml (1)
Line range hint 1-93: Verify dependency requirements after protobuf removal.
Given the removal of version-related protobuf definitions mentioned in the AI summary, please verify if all protobuf-related dependencies (pbjson, pbjson-types, prost, pbjson-build, prost-build) are still required or if any new dependencies are needed for the new versioning approach.
✅ Verification successful
The results show extensive usage of protobuf across the codebase with numerous .proto files and many source files importing protobuf-related crates. Let me verify the build system's usage of these dependencies.
Protobuf dependencies are still required - do not remove
The verification shows that protobuf is extensively used throughout the codebase:
- 26
.protofiles are actively maintained in theprotos/directory - The
build.rsscript is configured to compile these proto files and generate Rust code - Multiple source files import and use protobuf-related crates
- The protobuf dependencies (
pbjson,pbjson-types,prost,pbjson-build,prost-build) are essential for the project's functionality
The removal of version-related protobuf definitions does not affect the need for these dependencies as they are used for other core features in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if protobuf-related dependencies are still used in the codebase
# after removing versions.proto
# Test 1: Search for protobuf imports and usage
echo "Checking for protobuf usage in the codebase..."
rg -l "use (prost|pbjson)"
# Test 2: Search for .proto files to verify if any remain
echo "Checking for remaining .proto files..."
fd -e proto
Length of output: 2158
Script:
#!/bin/bash
# Check build.rs content to understand protobuf compilation setup
echo "Checking build.rs content..."
cat build.rs
# Check for protobuf message usage in source files
echo -e "\nChecking for protobuf message type usage..."
rg "::proto::" -A 2
Length of output: 2704
.github/workflows/rust.yml (2)
109-111: LGTM: Job configuration is well structured.
The Schema job is properly configured with appropriate dependencies and runner specification.
133-134: Verify versionsInfo endpoint schema generation.
Since this PR aims to make the versions info endpoint consistent with mehari, ensure that the schema generation captures these changes correctly.
✅ Verification successful
Schema generation correctly reflects versionsInfo endpoint structure
The schema generation for the versionsInfo endpoint is properly defined with:
- Complete OpenAPI documentation via utoipa attributes
- Well-structured response types (VersionsInfoResponse) with proper serialization
- Comprehensive version information model including genes and sequence variants
- Proper error handling with 200 and 500 response codes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the generated schema includes the versionsInfo endpoint
cargo run -- server schema --output-file /tmp/check.yaml
yq e '.paths."/api/v1/versionsInfo"' /tmp/check.yaml
Length of output: 256
Script:
#!/bin/bash
# Let's check the API routes and schema definitions in the codebase
rg -A 5 "versionsInfo" --type rust
Length of output: 815
Script:
#!/bin/bash
# Let's check the schema definition for VersionsResponse and VersionsInfoQuery
rg -A 10 "struct VersionsResponse|struct VersionsInfoQuery" --type rust
Length of output: 687
Script:
#!/bin/bash
# Let's check the entire versions.rs file to understand the complete schema
cat src/server/run/versions.rs
Length of output: 8618
openapi.schema.yaml (1)
88-91: Enhance VersionsInfoQuery schema.
The VersionsInfoQuery schema is empty but defined. Either:
- Remove it if not needed
- Add expected query parameters
Let's verify if this schema is used:
✅ Verification successful
The search results show that VersionsInfoQuery is actively used in the codebase, specifically in src/server/run/versions.rs. It's defined as an empty struct pub struct VersionsInfoQuery {} and used as a query parameter in an API endpoint. This indicates that it's intentionally empty and doesn't require any query parameters.
Empty VersionsInfoQuery schema is correct
The empty schema is intentional as evidenced by the Rust implementation. The endpoint accepts an empty query struct, meaning it doesn't expect any query parameters. No changes needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for VersionsInfoQuery usage
rg "VersionsInfoQuery" -A 5
Length of output: 2258
src/common/cli.rs (1)
37-37: LGTM! The added derive macros enhance API compatibility.
The addition of serde::Deserialize and utoipa::ToSchema traits to GenomeRelease is appropriate for:
- Enabling deserialization of genome release values from API requests
- Generating OpenAPI schema documentation for the versions info endpoint
This change aligns well with the PR objective of making the versions info endpoint consistent with Mehari.
src/server/run/versions.rs (4)
26-30: LGTM: Clean type conversion implementations.
The From trait implementations for converting between schema types and API types are well-structured and handle all fields correctly, including optional fields.
Also applies to: 89-117
127-191: LGTM: Well-structured and documented data types.
The version-related structs are well-organized with:
- Consistent naming convention using "Versions" prefix
- Comprehensive documentation for all fields
- Appropriate trait derivations for serialization/deserialization
204-236: LGTM: Well-implemented endpoint handler.
The handler implementation is clean and robust:
- Properly builds the response structure
- Correctly handles optional version specs
- Uses appropriate error handling with CustomError
122-125: Verify the empty query struct.
The VersionsInfoQuery struct is empty. Is this intentional? If no query parameters are needed, consider documenting this design decision.
✅ Verification successful
Empty query struct is intentional and well-documented
The empty VersionsInfoQuery struct is intentionally designed this way as it represents an endpoint that doesn't require any query parameters. This is documented in the OpenAPI schema (openapi.schema.yaml) which describes it as "Query parameters for handle()" with type "object" but no fields, indicating a deliberate design choice for an endpoint that returns version information without any filtering or parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any usages of VersionsInfoQuery fields in the codebase
rg "VersionsInfoQuery\s*{\s*\w+\s*:"
Length of output: 173
Script:
#!/bin/bash
# Check for usages of VersionsInfoQuery in the codebase
rg "VersionsInfoQuery" -A 3
# Check if there are any routes or handlers using this query
ast-grep --pattern 'fn $_($_: VersionsInfoQuery) $$$'
Length of output: 1711
src/genes/cli/import.rs (2)
Line range hint 1-75: LGTM: Well-structured argument handling and imports.
The Args struct is well-documented and properly implements command-line argument parsing with all required paths. The imports are organized logically and provide comprehensive functionality for the module.
1159-1159: 🛠️ Refactor suggestion
Add validation before database write.
Consider adding validation checks before writing to the database to ensure data integrity. This could include checking for required fields or data format validation.
src/gnomad_nuclear/cli/import.rs (2)
Line range hint 1-1: Module documentation added successfully
The module-level comment provides a clear description of the module's purpose, improving code readability and maintainability.
421-421: Consistent key formatting in RocksDB metadata
The use of header_field.replace('_', '-') in the key formatting ensures consistency by replacing underscores with hyphens. This approach aligns with the rest of the metadata keys and helps maintain a uniform naming convention.
src/server/run/mod.rs (5)
31-31: Import pbs::genes for gene data handling
The addition of pbs::genes is appropriate since it provides the necessary modules for gene data extraction, such as genes::base::Record.
40-42: Update imports for version-related types
Importing version-related types from server::run::versions aligns with the refactoring goal to make version info endpoints consistent with Mehari. This change consolidates version handling within the versions module.
52-57: Enhance OpenAPI documentation with new version schemas
Including the new version-related types (VersionsInfoQuery, VersionsInfoResponse, etc.) in the OpenAPI components ensures the API documentation is up-to-date with the latest schema changes.
267-267: Add version_spec to WithVersionSpec struct
Introducing the version_spec field enhances the WithVersionSpec<T> struct by associating version specification data with the contained item.
279-282: Properly load the version specification from file
The implementation correctly reads the version_spec from the provided path using from_path and handles the optional presence with transpose(), ensuring any I/O errors are propagated.
| - name: Install stable toolchain | ||
| uses: actions-rs/toolchain@v1 | ||
| with: | ||
| toolchain: stable | ||
| override: true | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update outdated GitHub Action.
The actions-rs/toolchain@v1 action is outdated. Consider updating to a newer version or switching to dtolnay/rust-toolchain which is actively maintained.
- - name: Install stable toolchain
- uses: actions-rs/toolchain@v1
- with:
- toolchain: stable
- override: true
+ - name: Install stable toolchain
+ uses: dtolnay/rust-toolchain@stable📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Install stable toolchain | |
| uses: actions-rs/toolchain@v1 | |
| with: | |
| toolchain: stable | |
| override: true | |
| - name: Install stable toolchain | |
| uses: dtolnay/rust-toolchain@stable |
🧰 Tools
🪛 actionlint
123-123: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| get: | ||
| tags: | ||
| - versions | ||
| summary: Query for annotations for one variant. |
There was a problem hiding this comment.
Update endpoint summary for accuracy.
The current summary "Query for annotations for one variant" seems incorrect as this endpoint returns version information.
Apply this change:
- summary: Query for annotations for one variant.
+ summary: Retrieve version information for all annotation databases.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| summary: Query for annotations for one variant. | |
| summary: Retrieve version information for all annotation databases. |
openapi.schema.yaml
Outdated
| /api/v1/versionsInfo: | ||
| get: | ||
| tags: | ||
| - versions | ||
| summary: Query for annotations for one variant. | ||
| operationId: versionsInfo | ||
| parameters: [] | ||
| responses: | ||
| '200': | ||
| description: Version information. | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/VersionsResponse' | ||
| '500': | ||
| description: Internal server error. | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/CustomError' |
There was a problem hiding this comment.
Add security definitions to protect the endpoint.
The endpoint currently lacks security definitions. Consider adding:
- Authentication requirements
- Rate limiting
- CORS policies
Apply this addition to the OpenAPI schema:
paths:
/api/v1/versionsInfo:
get:
+ security:
+ - bearerAuth: []
tags:
- versionsAnd add the security schemes:
components:
+ securitySchemes:
+ bearerAuth:
+ type: http
+ scheme: bearerCommittable suggestion skipped: line range outside the PR's diff.
src/server/run/versions.rs
Outdated
| get, | ||
| operation_id = "versionInfos", | ||
| params(VersionInfoQuery), | ||
| operation_id = "versionsInfo", | ||
| params(VersionsInfoQuery), | ||
| responses( | ||
| (status = 200, description = "Version information.", body = VersionInfoResponse), | ||
| (status = 200, description = "Version information.", body = VersionsResponse), | ||
| (status = 500, description = "Internal server error.", body = CustomError) |
There was a problem hiding this comment.
Fix OpenAPI documentation inconsistency.
The response type in the OpenAPI documentation (VersionsResponse) doesn't match the actual response type (VersionsInfoResponse).
Apply this fix:
- (status = 200, description = "Version information.", body = VersionsResponse),
+ (status = 200, description = "Version information.", body = VersionsInfoResponse),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| get, | |
| operation_id = "versionInfos", | |
| params(VersionInfoQuery), | |
| operation_id = "versionsInfo", | |
| params(VersionsInfoQuery), | |
| responses( | |
| (status = 200, description = "Version information.", body = VersionInfoResponse), | |
| (status = 200, description = "Version information.", body = VersionsResponse), | |
| (status = 500, description = "Internal server error.", body = CustomError) | |
| get, | |
| operation_id = "versionsInfo", | |
| params(VersionsInfoQuery), | |
| responses( | |
| (status = 200, description = "Version information.", body = VersionsInfoResponse), | |
| (status = 500, description = "Internal server error.", body = CustomError) |
| if let Some(log::Level::Trace | log::Level::Debug) = args_common.verbose.log_level() { | ||
| std::env::set_var("RUST_LOG", "debug"); | ||
| env_logger::init_from_env(env_logger::Env::new().default_filter_or("info")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust logging level to match verbosity settings
Currently, when the verbosity is set to trace, the RUST_LOG environment variable is set to "debug", which might prevent trace-level logs from appearing. Consider setting RUST_LOG to reflect the actual log level specified by the user.
Apply this change to align the logging level with the user's verbosity setting:
- if let Some(log::Level::Trace | log::Level::Debug) = args_common.verbose.log_level() {
- std::env::set_var("RUST_LOG", "debug");
+ if let Some(level @ (log::Level::Trace | log::Level::Debug)) = args_common.verbose.log_level() {
+ std::env::set_var("RUST_LOG", level.as_str().to_lowercase());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(log::Level::Trace | log::Level::Debug) = args_common.verbose.log_level() { | |
| std::env::set_var("RUST_LOG", "debug"); | |
| env_logger::init_from_env(env_logger::Env::new().default_filter_or("info")); | |
| if let Some(level @ (log::Level::Trace | log::Level::Debug)) = args_common.verbose.log_level() { | |
| std::env::set_var("RUST_LOG", level.as_str().to_lowercase()); | |
| env_logger::init_from_env(env_logger::Env::new().default_filter_or("info")); |
| .inspect_err(|e| { | ||
| println!("{} -- {:?}", &s, e); | ||
| }) | ||
| .unwrap() | ||
| }) |
There was a problem hiding this comment.
Use of .inspect_err() requires Rust 1.62 or newer
The method .inspect_err() on Result was stabilized in Rust 1.62. If your project targets an older Rust version, this code will not compile. To maintain compatibility with earlier Rust versions, consider using .map_err() instead.
Apply this diff to use .map_err():
serde_json::from_str::<panelapp::Record>(s)
- .inspect_err(|e| {
+ .map_err(|e| {
println!("{} -- {:?}", &s, e);
+ e
})
.unwrap()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .inspect_err(|e| { | |
| println!("{} -- {:?}", &s, e); | |
| }) | |
| .unwrap() | |
| }) | |
| .map_err(|e| { | |
| println!("{} -- {:?}", &s, e); | |
| e | |
| }) | |
| .unwrap() | |
| }) |
holtgrewe
force-pushed
the
refactor-versions-infos
branch
2 times, most recently
from
3a902d0 to
1b59128
Compare
holtgrewe
force-pushed
the
refactor-versions-infos
branch
from
1b59128 to
024bb10
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #579 +/- ##
==========================================
- Coverage 70.78% 70.78% -0.01%
==========================================
Files 80 80
Lines 13151 13152 +1
==========================================
Hits 9309 9309
- Misses 3842 3843 +1 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
Release Notes
New Features
/api/v1/versionsInfofor querying version annotations.Improvements
utoipain the CI configuration.Bug Fixes
CustomErrorstruct for better clarity.Refactor