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

Fix # Issue with gene-panel-data/fetch API When molecularProfileIds is Empty [Issue 11252] #11253

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jonkiky
Copy link

@jonkiky jonkiky commented Dec 4, 2024

Fix # Issue with gene-panel-data/fetch API When molecularProfileIds is Empty
Related issue ticket: #11252

There is an issue when using the API gene-panel-data/fetch with an empty molecularProfileIds array as the request payload, it returns error.

Describe changes proposed in this pull request:

  • The fix allows to pass in empty molecularProfileIds and response empty list .

@Aiosa
Copy link

Aiosa commented Dec 10, 2024

I did similar thing to fix the checker:

BBMRI-ERIC/cbioportal@8636b4a#diff-1054b9ebb643fc3312ccec2bec08d82d9e28f9249024817405fd16ded9023a5fR93

However, I had to also extend endpoint to allow query to request 0 identifiers

BBMRI-ERIC/cbioportal@8636b4a#diff-1054b9ebb643fc3312ccec2bec08d82d9e28f9249024817405fd16ded9023a5fR93

Note that github seems to be having issue with direct links to rows, so you might need to search for relevant lines in the GenePanelDataMultipleStudyFilter.java and GenePanelDataController.java

@alisman alisman force-pushed the yizhen-gene-panel-data-empty-molecularProfile branch from 822b89f to 3ad70d1 Compare January 13, 2025 21:14
@alisman
Copy link
Contributor

alisman commented Jan 13, 2025

@jonkiky which PR should i merge? this one or @Aiosa ?

@Aiosa
Copy link

Aiosa commented Jan 14, 2025

Well, this PR does not account for modifications in the API (annotation to allow empty gene panel query).

if(CollectionUtils.isEmpty(interceptedGenePanelDataMultipleStudyFilter.getMolecularProfileIds())) {
List<MolecularProfileCaseIdentifier> molecularProfileSampleIdentifiers = interceptedGenePanelDataMultipleStudyFilter.getSampleMolecularIdentifiers()
List<GenePanelData> genePanelDataList = new ArrayList<>();
if(interceptedGenePanelDataMultipleStudyFilter.getMolecularProfileIds()!= null && CollectionUtils.isEmpty(interceptedGenePanelDataMultipleStudyFilter.getMolecularProfileIds())) {
Copy link

Choose a reason for hiding this comment

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

it would be nicer to call the endpoint once, store it to a variable and test against it - a function call might be unnecessarily expensive.

List<MolecularProfileCaseIdentifier> molecularProfileSampleIdentifiers = interceptedGenePanelDataMultipleStudyFilter.getSampleMolecularIdentifiers()
List<GenePanelData> genePanelDataList = new ArrayList<>();
if(interceptedGenePanelDataMultipleStudyFilter.getMolecularProfileIds()!= null && CollectionUtils.isEmpty(interceptedGenePanelDataMultipleStudyFilter.getMolecularProfileIds())) {
if(interceptedGenePanelDataMultipleStudyFilter.getSampleMolecularIdentifiers()!= null){
Copy link

Choose a reason for hiding this comment

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

also here

@Aiosa
Copy link

Aiosa commented Jan 16, 2025

@jonkiky also I am not sure how this fix worked for you, but the API has to also enable zero-query panels, e.g.

public class GenePanelDataMultipleStudyFilter implements Serializable {

    -  @Size(min = 1, max = PagingConstants.MAX_PAGE_SIZE)
    +  @Size(min = 0, max = PagingConstants.MAX_PAGE_SIZE)
    private List<SampleMolecularIdentifier> sampleMolecularIdentifiers;
    -  @Size(min = 1, max = PagingConstants.MAX_PAGE_SIZE)
    +  @Size(min = 0, max = PagingConstants.MAX_PAGE_SIZE)
    private List<String> molecularProfileIds;

So if you fix these, it should be ready to go. Otherwise we will create new PR and submit the fix elsehwere.

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.

3 participants