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

IDC-1532 multiple series search google #2026

Merged
merged 7 commits into from
Sep 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions platform/core/src/studies/retrieveStudiesMetadata.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,28 @@ import { retrieveStudyMetadata } from './retrieveStudyMetadata';
* @param {Array} studyInstanceUIDs The UIDs of the Studies to be retrieved
* @param {Object} [filters] - Object containing filters to be applied on retrieve metadata process
* @param {string} [filter.seriesInstanceUID] - series instance uid to filter results against
* @param {boolean} [separateSeriesInstanceUIDFilters = false] - If true, split filtered metadata calls into multiple calls,
* as some DICOMWeb implementations only support single filters.
* @returns {Promise} that will be resolved with the metadata or rejected with the error
*/
export default function retrieveStudiesMetadata(
server,
studyInstanceUIDs,
filters
filters,
separateSeriesInstanceUIDFilters = false
) {
// Create an empty array to store the Promises for each metaData retrieval call
const promises = [];

// Loop through the array of studyInstanceUIDs
studyInstanceUIDs.forEach(function(StudyInstanceUID) {
// Send the call and resolve or reject the related promise based on its outcome
const promise = retrieveStudyMetadata(server, StudyInstanceUID, filters);
const promise = retrieveStudyMetadata(
server,
StudyInstanceUID,
filters,
separateSeriesInstanceUIDFilters
);

// Add the current promise to the array of promises
promises.push(promise);
Expand Down
80 changes: 74 additions & 6 deletions platform/core/src/studies/retrieveStudyMetadata.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,16 @@ const StudyMetaDataPromises = new Map();
* @param {string} StudyInstanceUID The UID of the Study to be retrieved
* @param {Object} [filters] - Object containing filters to be applied on retrieve metadata process
* @param {string} [filter.seriesInstanceUID] - series instance uid to filter results against
* @param {boolean} [separateSeriesInstanceUIDFilters = false] - If true, split filtered metadata calls into multiple calls,
* as some DICOMWeb implementations only support single filters.
* @returns {Promise} that will be resolved with the metadata or rejected with the error
*/
export function retrieveStudyMetadata(server, StudyInstanceUID, filters) {
export function retrieveStudyMetadata(
server,
StudyInstanceUID,
filters,
separateSeriesInstanceUIDFilters = false
) {
// @TODO: Whenever a study metadata request has failed, its related promise will be rejected once and for all
// and further requests for that metadata will always fail. On failure, we probably need to remove the
// corresponding promise from the "StudyMetaDataPromises" map...
Expand All @@ -33,18 +40,79 @@ export function retrieveStudyMetadata(server, StudyInstanceUID, filters) {
}

// Create a promise to handle the data retrieval
const promise = new Promise((resolve, reject) => {
RetrieveMetadata(server, StudyInstanceUID, filters).then(function(data) {
resolve(data);
}, reject);
});
let promise;

if (
filters &&
filters.seriesInstanceUID &&
separateSeriesInstanceUIDFilters
) {
promise = __separateSeriesRequestToAggregatePromiseateSeriesRequestToAggregatePromise(
server,
StudyInstanceUID,
filters
);
} else {
promise = RetrieveMetadata(server, StudyInstanceUID, filters);

/*
promise = new Promise((resolve, reject) => {
Copy link
Member

Choose a reason for hiding this comment

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

what is the point of this new promise? isn't it the same as promise = RetrieveMetadata(server, StudyInstanceUID, filters)?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just what was in the previous code. Hmm you are right, weird. Removed and tested, all works fine.

RetrieveMetadata(server, StudyInstanceUID, filters).then(function(data) {
resolve(data);
}, reject);
});
*/
}

// Store the promise in cache
StudyMetaDataPromises.set(StudyInstanceUID, promise);

return promise;
}

/**
* Splits up seriesInstanceUID filters to multiple calls for platforms
JamesAPetts marked this conversation as resolved.
Show resolved Hide resolved
* @param {Object} server Object with server configuration parameters
* @param {string} StudyInstanceUID The UID of the Study to be retrieved
* @param {Object} filters - Object containing filters to be applied on retrieve metadata process
*/
function __separateSeriesRequestToAggregatePromiseateSeriesRequestToAggregatePromise(
server,
StudyInstanceUID,
filters
) {
const { seriesInstanceUID } = filters;
const seriesInstanceUIDs = seriesInstanceUID.split(',');

return new Promise((resolve, reject) => {
const promises = [];

seriesInstanceUIDs.forEach(uid => {
const seriesSpecificFilters = Object.assign({}, filters, {
seriesInstanceUID: uid,
});

promises.push(
RetrieveMetadata(server, StudyInstanceUID, seriesSpecificFilters)
);
});

Promise.all(promises).then(results => {
const data = results[0];

let series = [];

results.forEach(result => {
series = [...series, ...result.series];
});

data.series = series;

resolve(data);
}, reject);
});
}

/**
* Delete the cached study metadata retrieval promise to ensure that the browser will
* re-retrieve the study metadata when it is next requested
Expand Down
2 changes: 2 additions & 0 deletions platform/viewer/public/config/idc.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ window.config = function(props) {
enableGoogleCloudAdapter: true,
enableGoogleCloudAdapterUI: false,
showStudyList: true,
filterQueryParam: true,
httpErrorHandler: error => {
// This is 429 when rejected from the public idc sandbox too often.
console.warn(error.status);

// Could use services manager here to bring up a dialog/modal if needed.
console.warn('test, navigate to https://ohif.org/');
window.location = 'https://ohif.org/';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ const _isQueryParamApplied = (study, filters = {}, isFilterStrategy) => {
const { seriesInstanceUID } = filters;
let applied = true;
// skip in case no filter or no toast manager

if (!seriesInstanceUID) {
return applied;
}
Expand Down Expand Up @@ -340,6 +341,10 @@ function ViewerRetrieveStudyData({
}
}

if (appConfig.enableGoogleCloudAdapter) {
retrieveParams.push(true); // Seperate SeriesInstanceUID filter calls.
Copy link
Member

Choose a reason for hiding this comment

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

This is really weird. Let's hope we never reorganize the arguments to the function.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, I'd rather have a deconstructed object inside retrieveStudiesMetadata, but it'd change what is actually top level API in @ohif/core.

}

cancelableStudiesPromises[studyInstanceUIDs] = makeCancelable(
retrieveStudiesMetadata(...retrieveParams)
)
Expand Down