-
Notifications
You must be signed in to change notification settings - Fork 102
TASK-7756 - Implement Catalog Operation Services #2605
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
base: develop
Are you sure you want to change the base?
Conversation
|
Task linked: TASK-7756 Implement Catalog Operation Services |
…s/study/CatalogStudyConfiguration.java Co-authored-by: Copilot <[email protected]> Signed-off-by: Pedro Furió <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements catalog operation services by adding support for new catalog and quality control configurations and updating clinical analysis CVDB index handling. Key changes include:
- Introducing CatalogStudyConfiguration in StudyConfiguration to provide catalog‐related defaults.
- Propagating QualityControlStatus throughout Sample, Individual, and Family models.
- Migrating CVDB index fields and updating related DB query parameters and migration tasks.
Reviewed Changes
Copilot reviewed 25 out of 105 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| opencga-core/src/main/java/org/opencb/opencga/core/models/study/configuration/StudyConfiguration.java | Added CatalogStudyConfiguration field and updated constructors/toString. |
| opencga-core/src/main/java/org/opencb/opencga/core/models/study/CatalogStudyConfiguration.java & CatalogServiceConfiguration.java | Introduced configuration defaults for catalog operations. |
| opencga-core/src/main/java/org/opencb/opencga/core/models/common/QualityControlStatus.java | Added new quality control status with updated valid statuses. |
| opencga-core/src/main/java/org/opencb/opencga/core/models/clinical/CvdbIndexStatus.java & CvdbIndex.java | Revised CVDB index types and status handling. |
| opencga-catalog/... | Updated clinical analysis managers, MongoDB adaptors, and DB API keys to accommodate new fields. |
| opencga-app/src/test/java/...MigrationTask7756Test.java & ClinicalMigrationTask7756.java | Added migration support and tests for new catalog and CVDB index configurations. |
| opencga-app/src/main/java/org/opencb/opencga/app/cli/admin/executors/CatalogCommandExecutor.java | Adjusted method visibility for configuration validation. |
Comments suppressed due to low confidence (2)
opencga-app/src/main/java/org/opencb/opencga/app/cli/admin/executors/CatalogCommandExecutor.java:244
- Review whether changing the method visibility from private to protected is intentional and does not unintentionally expose internal configuration logic.
protected void validateConfiguration(AdminCliOptionsParser.CatalogDatabaseCommandOptions catalogOptions) {
opencga-catalog/src/main/java/org/opencb/opencga/catalog/db/mongodb/ClinicalAnalysisMongoDBAdaptor.java:466
- Ensure that the change from using the key INTERNAL_CVDB_INDEX to INTERNAL_CVDB_INDEX_STATUS is backward compatible and that all downstream consumers are updated accordingly.
QueryParams.CONSENT.key(), QueryParams.STATUS.key(), QueryParams.INTERPRETATION.key(), REPORT.key(),
opencga-catalog/src/main/java/org/opencb/opencga/catalog/managers/ClinicalAnalysisManager.java
Show resolved
Hide resolved
| } | ||
| break; | ||
| case CvdbIndexStatus.ERROR: | ||
| if (!Arrays.asList(CvdbIndexStatus.PENDING_REMOVE, CvdbIndexStatus.PENDING_OVERWRITE, CvdbIndexStatus.PENDING_INDEX) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about adding a method "isPending" to the class "CvdbIndexStatus" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And what's the purpose for that? I find it better this way
| } | ||
| break; | ||
| case CvdbIndexStatus.READY: | ||
| if (!Arrays.asList(CvdbIndexStatus.ERROR, CvdbIndexStatus.PENDING_REMOVE, CvdbIndexStatus.PENDING_OVERWRITE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't change to pending_remove or pending_overwrite, but it's ok to change to pending_index? What's the logic of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The state diagram of CvdbIndexStatus is somehow convoluted..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's completely the opposite. From READY it's OK to move to ERROR, PENDING_REMOVE or PENDING_OVERWRITE. Nothing else is allowed
|
|
||
| public class CatalogServiceConfiguration { | ||
|
|
||
| private boolean active; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, remember to add @DataField() annotations to any new catalog object with meaningfull descriptions
No description provided.