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

#1031 Submission GQL Small Data Fetch #1060

Merged
merged 10 commits into from
Sep 25, 2023
Merged

#1031 Submission GQL Small Data Fetch #1060

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7a7ea64
Base Files
demariadaniel Sep 18, 2023
037bd55
Type Defs, Remove Repeated Chars
demariadaniel Sep 18, 2023
3e20883
Merge branch 'develop' of https://github.com/icgc-argo/argo-clinical …
demariadaniel Sep 20, 2023
bbed438
First Round Working ClinicalSubmissionTypesList
demariadaniel Sep 20, 2023
0e6ce8a
Updated SchemaVersion
demariadaniel Sep 20, 2023
07024f5
Working Clinical Submission Disabled
demariadaniel Sep 20, 2023
1dd4f7c
Clean Up Resolvers
demariadaniel Sep 20, 2023
67ceec9
Make getSchemas more DRY
demariadaniel Sep 21, 2023
a603f09
Remove extra .then
demariadaniel Sep 21, 2023
7b08b56
Updated Type Assertion
demariadaniel Sep 21, 2023
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
7 changes: 1 addition & 6 deletions src/clinical/service-worker-thread/tasks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,10 @@

import { DeepReadonly } from 'deep-freeze';
import _, { isEmpty } from 'lodash';
import {
ClinicalEntitySchemaNames,
aliasEntityNames,
queryEntityNames,
} from '../../common-model/entities';
import { ClinicalEntitySchemaNames, aliasEntityNames } from '../../common-model/entities';
import {
calculateSpecimenCompletionStats,
dnaSampleFilter,
filterTumourNormalRecords,
getRequiredDonorFieldsForEntityTypes,
getClinicalEntitiesFromDonorBySchemaName,
getClinicalEntitySubmittedData,
Expand Down
28 changes: 19 additions & 9 deletions src/dictionary/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,16 +80,26 @@ export const get = async (req: Request, res: Response) => {
return res.status(200).send(schema);
};

export const getClinicalSchemas = async (withFields: boolean) =>
Copy link
Contributor Author

@demariadaniel demariadaniel Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This function has always returned either [String] or [{ }]; this does not play nicely w/ GraphQL TypeDefs
Open to suggestions + solutions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored for reusability (API vs GQL)
API is currently converting Clinical response to JSON, which masks difference btwn [String] or [{ }]

withFields
? await manager
.instance()
.getSchemasWithFields()
.then(result =>
result.filter(
(s: manager.SchemaWithFields) => s.name !== ClinicalEntitySchemaNames.REGISTRATION,
),
)
: await manager
.instance()
.getSchemaNames()
.then(result => result.filter(s => s !== ClinicalEntitySchemaNames.REGISTRATION));

Copy link
Contributor

@ciaranschutte ciaranschutte Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

is this logic the same apart from the instance method call?

could be little more DRY. not sure which is the async call so not sure where to put await..something roughly likee..

export const getClinicalSchemas = async (withFields: boolean) => {
  const managerInstance = await manager.instance();
  const schemaNameFilter =  (schemaName) => schemaName !== ClinicalEntitySchemaNames.REGISTRATION;
  const schemas = withFields ? managerInstance.getSchemasWithFields().map(schema => schema.name) : managerInstance.getSchemaNames();
  return schemas.filter(schemaNameFilter)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I had a feeling this could be improved. But, we don't want to map getSchemasWithFields(), we need to return the objects with both names + fields, so schemaNameFilter needs to be called differently depending on value of withFields: 67ceec9

export const getClinicalEntities = async (req: Request, res: Response) => {
const includeFields = req.query.includeFields as string;
if (includeFields && includeFields.toLowerCase() === 'true') {
const schemasWithFields = await manager.instance().getSchemasWithFields();
return res
.status(200)
.send(schemasWithFields.filter(s => s.name !== ClinicalEntitySchemaNames.REGISTRATION));
}
const schemas = await manager.instance().getSchemaNames();
return res.status(200).send(schemas.filter(s => s !== ClinicalEntitySchemaNames.REGISTRATION));
const withFields = req?.query?.includeFields?.toLowerCase() === 'true';
const schemas = await getClinicalSchemas(withFields).then(result => result);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's no need for then.
there's two different Promise methods at work here.
the chaining then and the more modern await - they both (in a simple case) do the same thing
waiting until Promise resolves and doing something it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this was unnecessary, removed


return res.status(200).send(schemas);
};

export const getClinicalEntitiesData = async (includeFields: string) => {
Expand Down
38 changes: 18 additions & 20 deletions src/schemas/clinical-mutations/clearRegistration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,26 +22,24 @@ import { loggerFor } from '../../logger';

const L = loggerFor(__filename);

const clearClinicalRegistrationMutation = {
clearClinicalRegistration: async (
obj: unknown,
args: {
shortName: string;
registrationId: string;
},
) => {
const { shortName, registrationId } = args;
return await operations
.deleteRegistration(registrationId, shortName)
.then(() => {
L.info(`Deleted registrationId ${registrationId}`);
return true;
})
.catch(err => {
L.error(`Failed to delete registration`, err);
return false;
});
const clearClinicalRegistration = async (
obj: unknown,
args: {
shortName: string;
registrationId: string;
},
) => {
const { shortName, registrationId } = args;
return await operations
.deleteRegistration(registrationId, shortName)
.then(() => {
L.info(`Deleted registrationId ${registrationId}`);
return true;
})
.catch(err => {
L.error(`Failed to delete registration`, err);
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

nice logging thumbs up emoticon

});
};

export default clearClinicalRegistrationMutation;
export default clearClinicalRegistration;
24 changes: 11 additions & 13 deletions src/schemas/clinical-mutations/commitRegistration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,18 @@

import { commitRegistration } from '../../submission/submission-to-clinical/submission-to-clinical';

const commitClinicalRegistrationMutation = {
commitClinicalRegistration: async (
obj: unknown,
args: { shortName: string; registrationId: string },
) => {
const { shortName: programId, registrationId } = args;
const commitClinicalRegistration = async (
obj: unknown,
args: { shortName: string; registrationId: string },
) => {
const { shortName: programId, registrationId } = args;

const newSampleIds = await commitRegistration({
registrationId,
programId,
});
const newSampleIds = await commitRegistration({
registrationId,
programId,
});

return newSampleIds;
},
return newSampleIds;
};

export default commitClinicalRegistrationMutation;
export default commitClinicalRegistration;
34 changes: 16 additions & 18 deletions src/schemas/clinical-resolvers/clearClinicalSubmissionResolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,23 +21,21 @@ import { GlobalGqlContext } from '../../app';
import submissionAPI from '../../submission/submission-api';
import { convertClinicalSubmissionDataToGql } from '../utils';

const clearClinicalSubmissionResolver = {
clearClinicalSubmission: async (
obj: unknown,
args: { programShortName: string; fileType: string; version: string },
contextValue: any,
) => {
const { programShortName, fileType, version } = args;
const response = await submissionAPI.clearFileDataFromActiveSubmission(
programShortName,
fileType || 'all',
version,
(<GlobalGqlContext>contextValue).egoToken,
);
return convertClinicalSubmissionDataToGql(programShortName, {
submission: response,
});
},
const clearClinicalSubmission = async (
obj: unknown,
args: { programShortName: string; fileType: string; version: string },
contextValue: any,
) => {
const { programShortName, fileType, version } = args;
const response = await submissionAPI.clearFileDataFromActiveSubmission(
programShortName,
fileType || 'all',
version,
(<GlobalGqlContext>contextValue).egoToken,
Copy link
Contributor

Choose a reason for hiding this comment

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

unclear why this <GlobalGqlContext> is needed? Casting just the contextValue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See this convo: https://github.com/icgc-argo/argo-clinical/pull/1055/files/15990796567a4aa6ce4625beca424dfd0e16a533#r1325095439

);
return convertClinicalSubmissionDataToGql(programShortName, {
submission: response,
});
};

export default clearClinicalSubmissionResolver;
export default clearClinicalSubmission;
14 changes: 6 additions & 8 deletions src/schemas/clinical-resolvers/clinicalRegistrationData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,11 @@
import submissionAPI from '../../submission/submission-api';
import { convertRegistrationDataToGql } from '../utils';

const clinicalRegistrationResolver = {
clinicalRegistration: async (obj: unknown, args: { shortName: string }) => {
const registration = await submissionAPI.getRegistrationDataByProgramId(args.shortName);
return convertRegistrationDataToGql(args.shortName, {
registration: registration,
});
},
const clinicalRegistration = async (obj: unknown, args: { shortName: string }) => {
const registration = await submissionAPI.getRegistrationDataByProgramId(args.shortName);
return convertRegistrationDataToGql(args.shortName, {
registration: registration,
});
};

export default clinicalRegistrationResolver;
export default clinicalRegistration;
16 changes: 7 additions & 9 deletions src/schemas/clinical-resolvers/clinicalSearchResults.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,14 @@ type ClinicalVariables = {
filters: ClinicalSearchQuery;
};

const clinicalSearchResultResolver = {
clinicalSearchResults: async (obj: unknown, args: ClinicalVariables) => {
const { programShortName, filters } = args;
const clinicalSearchResults = async (obj: unknown, args: ClinicalVariables) => {
const { programShortName, filters } = args;

const searchResults = (await getClinicalSearchResults(programShortName, filters)) || {
searchResults: [],
};
const searchResults = (await getClinicalSearchResults(programShortName, filters)) || {
searchResults: [],
};

return { ...searchResults, programShortName };
},
return { ...searchResults, programShortName };
};

export default clinicalSearchResultResolver;
export default clinicalSearchResults;
45 changes: 32 additions & 13 deletions src/schemas/clinical-resolvers/clinicalSubmissionDataResolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,41 @@
* ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/

import * as manager from '../../dictionary/manager';
import * as schemaApi from '../../dictionary/api';
import * as configService from '../../submission/persisted-config/service';
import submissionAPI from '../../submission/submission-api';
import get from 'lodash/get';
import { ActiveClinicalSubmission } from '../../submission/submission-entities';
import { DeepReadonly } from 'deep-freeze';
import { convertClinicalSubmissionDataToGql } from '../utils';
import { getClinicalEntitiesData } from '../../dictionary/api';

const clinicalSubmissionResolver = {
clinicalSubmissions: async (obj: unknown, args: { programShortName: string }) => {
const { programShortName } = args;
const clinicalSubmissions = async (obj: unknown, args: { programShortName: string }) => {
const { programShortName } = args;

const submissionData = await submissionAPI.getActiveSubmissionDataByProgramId(programShortName);
return convertClinicalSubmissionDataToGql(programShortName, {
submission: submissionData,
});
},
const submissionData = await submissionAPI.getActiveSubmissionDataByProgramId(programShortName);
return convertClinicalSubmissionDataToGql(programShortName, {
submission: submissionData,
});
};

export default clinicalSubmissionResolver;
export const clinicalSubmissionTypesList = async (
obj: unknown,
args: { includeFields: string },
) => {
const withFields = args?.includeFields?.toLowerCase() === 'true';
const schemas = await schemaApi.getClinicalSchemas(withFields);

return schemas;
};

export const clinicalSubmissionSchemaVersion = async () => {
const schemaVersion = await manager
.instance()
.getCurrent()
.then(result => result.version);
return schemaVersion;
};

export const clinicalSubmissionSystemDisabled = async (obj: unknown, args: {}) => {
return await configService.getSubmissionDisabledState();
};

export default clinicalSubmissions;
36 changes: 17 additions & 19 deletions src/schemas/clinical-resolvers/commitClinicalSubmission.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,24 +23,22 @@ import { convertClinicalSubmissionDataToGql } from '../utils';
import { DeepReadonly } from 'deep-freeze';
import { ActiveClinicalSubmission } from '../../submission/submission-entities';

const commitClinicalSubmissionResolver = {
commitClinicalSubmission: async (
obj: unknown,
args: { programShortName: string; version: string },
contextValue: any,
) => {
const { programShortName, version } = args;
const submissionData = <DeepReadonly<ActiveClinicalSubmission>>(
await submissionAPI.commitActiveSubmissionData(
programShortName,
version,
(<GlobalGqlContext>contextValue).egoToken,
)
);
return convertClinicalSubmissionDataToGql(programShortName, {
submission: submissionData,
});
},
const commitClinicalSubmission = async (
obj: unknown,
args: { programShortName: string; version: string },
contextValue: any,
) => {
const { programShortName, version } = args;
const submissionData = <DeepReadonly<ActiveClinicalSubmission>>(
Copy link
Contributor

Choose a reason for hiding this comment

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

is there one single way we can work around all this DeapReadonly stuff and stick with that approach?
I think this works out the same as using as DeepReadonly....
A utility function or a type guard that can be the de facto way for reusability, single point of functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this one we can handle by casting the return type of commitActiveSubmissionData: {} as DeepReadonly<ActiveClinicalSubmission>

7b08b56

await submissionAPI.commitActiveSubmissionData(
programShortName,
version,
(<GlobalGqlContext>contextValue).egoToken,
)
);
return convertClinicalSubmissionDataToGql(programShortName, {
submission: submissionData,
});
};

export default commitClinicalSubmissionResolver;
export default commitClinicalSubmission;
32 changes: 15 additions & 17 deletions src/schemas/clinical-resolvers/validateClinicalSubmissionResolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,22 +21,20 @@ import submissionAPI from '../../submission/submission-api';
import { GlobalGqlContext } from '../../app';
import { convertClinicalSubmissionDataToGql } from '../utils';

const validateClinicalSubmissionResolver = {
validateClinicalSubmissions: async (
obj: unknown,
args: { programShortName: string; version: string },
contextValue: any,
) => {
const { programShortName, version } = args;
const response = await submissionAPI.validateActiveSubmissionData(
programShortName,
version,
(<GlobalGqlContext>contextValue).egoToken,
);
return convertClinicalSubmissionDataToGql(programShortName, {
submission: response?.submission,
});
},
const validateClinicalSubmission = async (
obj: unknown,
args: { programShortName: string; version: string },
contextValue: any,
) => {
const { programShortName, version } = args;
const response = await submissionAPI.validateActiveSubmissionData(
programShortName,
version,
(<GlobalGqlContext>contextValue).egoToken,
);
return convertClinicalSubmissionDataToGql(programShortName, {
submission: response?.submission,
});
};

export default validateClinicalSubmissionResolver;
export default validateClinicalSubmission;
17 changes: 17 additions & 0 deletions src/schemas/gqlTypeDefs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,21 @@ const typeDefs = gql`
Retrieve current stored Clinical Submission data for a program
"""
clinicalSubmissions(programShortName: String!): ClinicalSubmissionData!

"""
Retrieve current stored Clinical Submission Data Dictionary Schema version
"""
clinicalSubmissionSchemaVersion: String!

"""
Retrieve current Clinical Submission disabled state for both sample_registration and clinical entity files
"""
clinicalSubmissionSystemDisabled: Boolean!

"""
Retrieve current stored Clinical Submission Types list
"""
clinicalSubmissionTypesList(includeFields: String): [SchemaList]!
}

type Mutation {
Expand Down Expand Up @@ -339,6 +354,8 @@ const typeDefs = gql`
oldValue: String!
donorId: String!
}

scalar SchemaList
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This scalar is a workaround for the issue in /src/dictionary/api
clinicalSubmissionTypesList cannot return [String] | [SchemaWithFieldsObject]; unions cannot include primitives

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you link me to more on this please? I don't understand and my gut is telling me that unions should be ok with primitives.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They aren't: https://www.apollographql.com/docs/apollo-server/schema/unions-interfaces/
Only allows Object types
Entire Github thread about it: graphql/graphql-spec#215

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to sleep on it and review in the morning
In Gateway, this is simply [String] because the response array is converted to JSON, so that may be what client expects and I should just stick to the same format
I was trying to format this to see the proper object for GQL Server purposes, which I think is ideal but might require client side changes

Copy link
Contributor

@ciaranschutte ciaranschutte Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

sounds good - let me know where the union is happening tomorrow please - I read the docs + github - very interesting - looks like unions don't work on scalars either

All of a union's included types must be [object types](https://www.apollographql.com/docs/apollo-server/schema/schema/#object-types) (not scalars, input types, etc.). Included types do not need to share any fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh so this Union is related to getSchemasWithFields: #1060 (comment)
Where were either returning a String[] of Schema names or an array of SchemaWithFields objects

`;

export default typeDefs;
Loading