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.

Sign up for GitHub

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

Jump to bottom

Refactor sort/search/pagination for ClinicalData table endpoint #10407

Merged
merged 8 commits into from
May 7, 2024
Merged

Refactor sort/search/pagination for ClinicalData table endpoint #10407

merged 8 commits into from
May 7, 2024

Conversation

pvannierop
Copy link
Contributor

@pvannierop pvannierop commented Sep 28, 2023 •
edited
Loading

⚠️ This PR breaks the API. Should be merged together with the corresponding cbioportal-frontend PR.

Background

  • The current code used for paginated download of clinical data used by the Clinical Data tab on study view does not work correctly.
  • Numerical sort is not supported at present. All data is handled as string.
  • Data structure returned for clinical data is separated between sample and patient level clinical attributes. The frontend needs to integrate this at the sample level to generate the table.

Solution

  • The MyBatis SQL code was updated. This logic handles pagination, sorting and searching. Test were added to confirm correct function.
  • Conversion of Clinical Data to numeric type was included in SQL in order to perform correct sorting.
  • Logic was moved from Controller to Service layer.
  • Clinical Data is corrected in the SampleClinicalDataCollection class where the data is keyed by the uniqueSampleKey (base64 encoded concatenation of sample and study identifier).

Remarks

  • The pagination that is requested by the controller, is not handled in persistence layer (SQL), but in the application layer (StudyViewServiceImpl.java). The reason for this is that the query should also return the total amount of results w/o pagination. Handling the pagination in the application layer prevents duplicate database queries.
  • base64 encoder was moved to utils module for reuse in service and web modules.
  • OffsetCalculator was renamed to PaginationCalculator to provide other calculations.

@pvannierop pvannierop force-pushed the demo-fix-clinical-table-sorting branch from 0009e1e to 2a73c03 Compare September 28, 2023 09:15
@pvannierop pvannierop self-assigned this Sep 28, 2023
@pvannierop pvannierop force-pushed the demo-fix-clinical-table-sorting branch from 2a73c03 to 20d4347 Compare September 28, 2023 09:25
@sonarcloud
Copy link

sonarcloud bot commented Sep 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.20) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@dippindots dippindots added the api label Oct 17, 2023
dippindots
dippindots approved these changes Oct 17, 2023
View reviewed changes
Copy link
Member

@dippindots dippindots left a comment

Choose a reason for hiding this comment

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

LGTM, just added some comments about the new model created

model/src/main/java/org/cbioportal/model/SampleClinicalDataCollection.java Outdated Show resolved Hide resolved
service/src/main/java/org/cbioportal/service/ClinicalDataService.java Outdated Show resolved Hide resolved
...-mybatis/src/test/java/org/cbioportal/persistence/mybatis/util/PaginationCalculatorTest.java Outdated Show resolved Hide resolved
@dippindots
Copy link
Member

@pvannierop Could you link the related frontend pr and rebase this pr? Thanks!

@pvannierop pvannierop force-pushed the demo-fix-clinical-table-sorting branch from 20d4347 to 2a35baa Compare November 6, 2023 11:01
@pvannierop pvannierop requested a review from dippindots November 6, 2023 11:01
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.21) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@pvannierop
Copy link
Contributor Author

I have addressed your remarks. The branch has been rebased. I could not reference the frontend PR because @alisman still has to make this.

dippindots
dippindots approved these changes Nov 10, 2023
View reviewed changes
Copy link
Member

@dippindots dippindots left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@pvannierop Thanks for addressing my comments! It looks good to me. I guess we just need to wait for two more things:

  • wait for @alisman make the frontend pr
  • wait for RFC72 (spring-boot refactoring)
    Then we can merge this one.

model/src/main/java/org/cbioportal/model/ClinicalDataTableResult.java Outdated Show resolved Hide resolved
@pvannierop pvannierop mentioned this pull request Nov 29, 2023
Merged
@pvannierop pvannierop force-pushed the demo-fix-clinical-table-sorting branch from 2a35baa to dbe55af Compare December 21, 2023 13:22
@alisman alisman force-pushed the demo-fix-clinical-table-sorting branch from dbe55af to a19c568 Compare January 11, 2024 18:03
@alisman alisman force-pushed the demo-fix-clinical-table-sorting branch from a19c568 to bff1453 Compare January 30, 2024 15:15
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jan 30, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

6 New issues
0 Security Hotspots
89.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

JREastonMarks
JREastonMarks suggested changes Feb 27, 2024
View reviewed changes
Copy link
Contributor

@JREastonMarks JREastonMarks left a comment

Choose a reason for hiding this comment

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

A couple of issues to address mentioned here.
I would also look at SonarCube Analysis for other minor issues after you resolve the conflicts.

src/main/java/org/cbioportal/persistence/mybatis/util/PaginationCalculator.java Outdated Show resolved Hide resolved
src/main/java/org/cbioportal/service/impl/ClinicalDataServiceImpl.java Outdated Show resolved Hide resolved
src/main/java/org/cbioportal/service/impl/ClinicalDataServiceImpl.java Outdated Show resolved Hide resolved
src/main/java/org/cbioportal/web/StudyViewController.java Outdated Show resolved Hide resolved
src/main/java/org/cbioportal/persistence/ClinicalDataRepository.java Show resolved Hide resolved
@pvannierop pvannierop force-pushed the demo-fix-clinical-table-sorting branch from bff1453 to 9eeda41 Compare March 5, 2024 07:28
@pvannierop pvannierop requested a review from JREastonMarks March 5, 2024 07:28
@fuzhaoyuan fuzhaoyuan closed this Apr 2, 2024
@fuzhaoyuan fuzhaoyuan reopened this Apr 2, 2024
@fuzhaoyuan fuzhaoyuan self-assigned this Apr 10, 2024
@fuzhaoyuan fuzhaoyuan force-pushed the demo-fix-clinical-table-sorting branch from bc712a9 to 6ee609d Compare April 17, 2024 15:28
JREastonMarks
JREastonMarks approved these changes Apr 18, 2024
View reviewed changes
Copy link
Contributor

@JREastonMarks JREastonMarks left a comment

Choose a reason for hiding this comment

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

lgtm

@inodb inodb mentioned this pull request May 2, 2024
Closed
4 tasks
@inodb inodb requested a review from haynescd May 2, 2024 14:36
haynescd
haynescd approved these changes May 6, 2024
View reviewed changes
Copy link
Collaborator

@haynescd haynescd left a comment

Choose a reason for hiding this comment

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

lgtm

@dippindots dippindots force-pushed the demo-fix-clinical-table-sorting branch from 7c3e6a0 to 0726e4c Compare May 6, 2024 19:54
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 6, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
92.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@dippindots dippindots merged commit 166c955 into master May 7, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fuzhaoyuan fuzhaoyuan fuzhaoyuan left review comments

@haynescd haynescd haynescd approved these changes

@JREastonMarks JREastonMarks JREastonMarks approved these changes

@onursumer onursumer Awaiting requested review from onursumer

@dippindots dippindots Awaiting requested review from dippindots

Assignees

@pvannierop pvannierop

@fuzhaoyuan fuzhaoyuan

@TJMKuijpers TJMKuijpers

Labels
api
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pvannierop @dippindots @JREastonMarks @fuzhaoyuan @haynescd @TJMKuijpers