feat: endpoint /api/v1/strucvars/clinvar-annos/query with OpenAPI (#578) #591
Conversation
WalkthroughThis pull request includes several modifications across multiple files related to the handling of variant annotations and ClinVar queries. Key changes involve the restructuring of endpoints, specifically renaming the ClinVar SV query endpoint and introducing new request and response structures. The removal of lint suppression attributes in some functions is also noted, which may affect compiler warnings. Additionally, enhancements to OpenAPI documentation and the addition of new modules related to ClinVar data are implemented, improving the API's usability and clarity. Changes
Possibly related issues
Possibly related PRs
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: 3
🧹 Outside diff range and nitpick comments (3)
src/server/run/clinvar_sv.rs (1)
355-355: Fix typo in error messageThere's a typo in the error message: "Implementaion" should be "Implementation".
Apply the following diff:
- .map_err(|e| CustomError::new(anyhow::anyhow!("Implementaion error: {:?}", e)))? + .map_err(|e| CustomError::new(anyhow::anyhow!("Implementation error: {:?}", e)))?src/server/run/mod.rs (2)
41-41: Avoid using wildcard importsUsing wildcard imports like
server::run::clinvar_data::*can lead to namespace pollution and might import unnecessary items. It's better to import only the needed components explicitly to improve code clarity and maintainability.Consider specifying the items to import:
- server::run::clinvar_data::*, + server::run::clinvar_data::{ClinvarAllele, ClinvarClinicalAssertion, ClinvarTraitSet, ClinvarVariationArchive};Replace the items with the specific components you need from
clinvar_data.
125-261: Consider modularizing OpenAPI schema definitionsThe addition of numerous ClinVar-related schemas makes the
ApiDocstruct considerably large and harder to maintain. Organizing these schemas into separate modules or grouping them logically can improve code readability and maintainability.Consider refactoring by:
- Creating sub-modules for related schemas (e.g.,
clinvar_data::allele,clinvar_data::variation).- Using re-exports to simplify the
components(schemas(...))section.This approach will help future developers navigate and update the code more efficiently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/server/run/annos_variant.rs(0 hunks)src/server/run/clinvar_sv.rs(5 hunks)src/server/run/genes_clinvar.rs(0 hunks)src/server/run/genes_info.rs(0 hunks)src/server/run/mod.rs(5 hunks)
💤 Files with no reviewable changes (3)
- src/server/run/annos_variant.rs
- src/server/run/genes_clinvar.rs
- src/server/run/genes_info.rs
🧰 Additional context used
🪛 GitHub Check: clippy
src/server/run/clinvar_sv.rs
[warning] 350-350: use of a fallible conversion when an infallible one could be used
warning: use of a fallible conversion when an infallible one could be used
--> src/server/run/clinvar_sv.rs:350:13
|
350 | TryInto::::try_into(query.into_inner()).map_err(|e| {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: Into::into
|
= note: converting StrucvarsClinvarQuery to Request cannot fail
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_fallible_conversions
= note: #[warn(clippy::unnecessary_fallible_conversions)] on by default
[warning] 229-229: an implementation of From is preferred since it gives you Into<_> for free where the reverse isn't true
warning: an implementation of From is preferred since it gives you Into<_> for free where the reverse isn't true
--> src/server/run/clinvar_sv.rs:229:1
|
229 | impl Into for StrucvarsClinvarQuery {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#from_over_into
help: replace the Into implementation with From<server::run::clinvar_sv::StrucvarsClinvarQuery>
|
229 ~ impl From for Request {
230 ~ fn from(val: StrucvarsClinvarQuery) -> Self {
231 | Request {
232 ~ genome_release: val.genome_release.to_string(),
233 ~ chromosome: val.chromosome,
234 ~ start: val.start,
235 ~ stop: val.stop,
236 ~ variation_types: val
237 | .variation_types
238 | .map(|v| v.into_iter().map(Into::into).collect()),
239 ~ min_overlap: val.min_overlap,
240 ~ page_no: val.page_no,
241 ~ page_size: val.page_size,
|
| impl Into<Request> for StrucvarsClinvarQuery { | ||
| fn into(self) -> Request { | ||
| Request { | ||
| genome_release: self.genome_release.to_string(), | ||
| chromosome: self.chromosome, | ||
| start: self.start, | ||
| stop: self.stop, | ||
| variation_types: self | ||
| .variation_types | ||
| .map(|v| v.into_iter().map(Into::into).collect()), | ||
| min_overlap: self.min_overlap, | ||
| page_no: self.page_no, | ||
| page_size: self.page_size, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer implementing From instead of Into for conversions
Implementing From<StrucvarsClinvarQuery> for Request is preferred since it automatically provides the Into implementation, enhancing code clarity and interoperability.
Apply the following changes:
-impl Into<Request> for StrucvarsClinvarQuery {
- fn into(self) -> Request {
+impl From<StrucvarsClinvarQuery> for Request {
+ fn from(val: StrucvarsClinvarQuery) -> Self {
Request {
- genome_release: self.genome_release.to_string(),
- chromosome: self.chromosome,
- start: self.start,
- stop: self.stop,
- variation_types: self
+ genome_release: val.genome_release.to_string(),
+ chromosome: val.chromosome,
+ start: val.start,
+ stop: val.stop,
+ variation_types: val
.variation_types
.map(|v| v.into_iter().map(Into::into).collect()),
- min_overlap: self.min_overlap,
- page_no: self.page_no,
- page_size: self.page_size,
+ min_overlap: val.min_overlap,
+ page_no: val.page_no,
+ page_size: val.page_size,
}
}
}📝 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.
| impl Into<Request> for StrucvarsClinvarQuery { | |
| fn into(self) -> Request { | |
| Request { | |
| genome_release: self.genome_release.to_string(), | |
| chromosome: self.chromosome, | |
| start: self.start, | |
| stop: self.stop, | |
| variation_types: self | |
| .variation_types | |
| .map(|v| v.into_iter().map(Into::into).collect()), | |
| min_overlap: self.min_overlap, | |
| page_no: self.page_no, | |
| page_size: self.page_size, | |
| } | |
| } | |
| } | |
| impl From<StrucvarsClinvarQuery> for Request { | |
| fn from(val: StrucvarsClinvarQuery) -> Self { | |
| Request { | |
| genome_release: val.genome_release.to_string(), | |
| chromosome: val.chromosome, | |
| start: val.start, | |
| stop: val.stop, | |
| variation_types: val | |
| .variation_types | |
| .map(|v| v.into_iter().map(Into::into).collect()), | |
| min_overlap: val.min_overlap, | |
| page_no: val.page_no, | |
| page_size: val.page_size, | |
| } | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: clippy
[warning] 229-229: an implementation of From is preferred since it gives you Into<_> for free where the reverse isn't true
warning: an implementation of From is preferred since it gives you Into<_> for free where the reverse isn't true
--> src/server/run/clinvar_sv.rs:229:1
|
229 | impl Into for StrucvarsClinvarQuery {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#from_over_into
help: replace the Into implementation with From<server::run::clinvar_sv::StrucvarsClinvarQuery>
|
229 ~ impl From for Request {
230 ~ fn from(val: StrucvarsClinvarQuery) -> Self {
231 | Request {
232 ~ genome_release: val.genome_release.to_string(),
233 ~ chromosome: val.chromosome,
234 ~ start: val.start,
235 ~ stop: val.stop,
236 ~ variation_types: val
237 | .variation_types
238 | .map(|v| v.into_iter().map(Into::into).collect()),
239 ~ min_overlap: val.min_overlap,
240 ~ page_no: val.page_no,
241 ~ page_size: val.page_size,
|
| TryInto::<Request>::try_into(query.into_inner()).map_err(|e| { | ||
| CustomError::new(anyhow::anyhow!("Query conversion error: {:?}", e)) | ||
| })?, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Into instead of TryInto for infallible conversions
Since converting StrucvarsClinvarQuery to Request cannot fail, you can simplify the code by using Into::into instead of TryInto::try_into, removing unnecessary error handling.
Apply the following changes:
- TryInto::<Request>::try_into(query.into_inner()).map_err(|e| {
- CustomError::new(anyhow::anyhow!("Query conversion error: {:?}", e))
- })?,
+ let request: Request = query.into_inner().into();Update the function call accordingly:
- handle_impl(
- data,
- path,
- request,
- )
+ handle_impl(data, path, request)And remove the now-unnecessary error mapping.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: clippy
[warning] 350-350: use of a fallible conversion when an infallible one could be used
warning: use of a fallible conversion when an infallible one could be used
--> src/server/run/clinvar_sv.rs:350:13
|
350 | TryInto::::try_into(query.into_inner()).map_err(|e| {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: Into::into
|
= note: converting StrucvarsClinvarQuery to Request cannot fail
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_fallible_conversions
= note: #[warn(clippy::unnecessary_fallible_conversions)] on by default
| @@ -140,6 +280,7 @@ pub async fn main(args: &Args, dbs: Data<WebServerData>) -> std::io::Result<()> | |||
| .service(annos_range::handle) | |||
| .service(annos_db_info::handle) | |||
| .service(clinvar_sv::handle) | |||
| .service(clinvar_sv::handle_with_openapi) | |||
There was a problem hiding this comment.
💡 Codebase verification
Remove the deprecated /clinvar-sv/query endpoint registration
The code shows that handle is marked with the deprecated path /clinvar-sv/query while handle_with_openapi serves the new path /api/v1/strucvars/clinvar/query. The dual registration is intentional for backward compatibility, but the old endpoint should be removed as it's explicitly marked as deprecated in the module documentation.
- Remove
.service(clinvar_sv::handle)fromsrc/server/run/mod.rs
🔗 Analysis chain
Verify the necessity of dual service registrations for clinvar_sv
Both clinvar_sv::handle and clinvar_sv::handle_with_openapi are registered as services. If handle_with_openapi is intended to replace handle, consider removing the original handle registration to avoid redundancy and potential conflicts.
Run the following script to check where clinvar_sv::handle is being used:
This script searches for any references to clinvar_sv::handle in the codebase outside of src/server/run/mod.rs. If there are no other references, it might be safe to remove the registration of clinvar_sv::handle.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of `clinvar_sv::handle` in the codebase.
# Find all occurrences of `clinvar_sv::handle` outside of `mod.rs`.
rg -g '!src/server/run/mod.rs' 'clinvar_sv::handle'
Length of output: 51
Script:
#!/bin/bash
# Let's check the content of the mod.rs file to see both service registrations
rg -A 5 -B 5 'clinvar_sv::' src/server/run/mod.rs
Length of output: 1759
Script:
#!/bin/bash
# Let's also check the clinvar_sv module to understand the implementation
fd clinvar_sv.rs --exec cat {}
Length of output: 12771
holtgrewe
deleted the
578-add-endpoint-apiv1strucvarsclinvar-annosquery-with-openapi-schema
branch
Summary by CodeRabbit
Release Notes
New Features
/api/v1/strucvars/clinvar/query, with updated request and response structures./api/v1/genes/infoendpoint, enhancing API documentation.Bug Fixes
Documentation
Chores