Refactor Base Schema to not require Donor/Specimen/Sample by joneubank · Pull Request #878 · overture-stack/SONG

joneubank · 2025-01-22T19:27:13Z

Issue Reference

Epic - Separate Donor/Specimen/Sample model from Analysis model #863

Summary

Important

Breaking change! This pull requests changes the core data model. With this change, analyses are no longer required to link to a specific donor, or reference specific samples. This data can be optionally added to a Custom Schema if that is relevant to that type of analysis.

This change removes all references to donors, specimens, and samples from the base schema. Analyses now have very little required information, primarily the Study and list of Files. Since Analyses no longer require donor or sample information, the relation between donors, specimens, and samples is no longer built and validated during analysis submission. See the linked issue for details on the motivation for this change.

Changes

Remove Donor/Specimen/Sample properties from base analysis schema
Remove search by Donor/Specimen/Sample IDs from the Analysis ID Search API since these aren't default properties of analyses
Add External Validation options on custom schemas and validation , to replace the ID Server checks that are not possible now that Donor and Samples are removed from the base schema
- Add External Validation Rules for Analysis Properties Based on Dynamic Schema #877
- Schema registrations sets options values correctly #879

Bug Fixes

The following fixes are included in this work but not captured in the above tickets

Paginated search was broken due to the fileType enum removal. The stored procedure for get_analysis is updated to no longer join with the donor/specimen/sample tables and fixes this type issue
Analysis schema options properties, including fileTypes are set to empty lists by default, and can be cleared by providing an empty list.

…cimen-sample-from-analysis Remove Donor, Specimen, and Sample from Analysis Data Classes in SONG

…amic Schemas

…cimen-sample-from-base-schema Remove Donor, Specimen, and Samples from Base Schema and Allow in Dynamic Schemas

…cimen-sample-search Remove Donor/Specimen/Sample References from Study and Search APIs

…c Schemas

…on-rules-for-analysis Add External Validation Rules for Analysis Properties Based on Dynamic Schema

…ile_type enum

- when missing, uses value from previous schema or sets as empty (default) value - when set as empty array, replaces current value with empty array (removes optional validations) - sets options in get registration by version query

justincorrigible

This is just awesome! long time coming too.
thanks for taking the time to get this done!

Left a couple comments on the docs side, the rest of the changes LGTM 👍 (except for that random semicolon 😆 maybe I'm missing something?)

justincorrigible · 2025-01-28T11:36:46Z

+
+## Options
+
+All properties in options are optional. If no value is provided, a default configuration will be used for the analysis. If this is an update to an existing analysis type, you can omit any option and the value will be maintained from the previous version.


Suggested change

All properties in options are optional. If no value is provided, a default configuration will be used for the analysis. If this is an update to an existing analysis type, you can omit any option and the value will be maintained from the previous version.

All properties in "options" are elective. If no value is provided, a default configuration (shown bellow) will be used for the analysis. If this is an update to an existing analysis type, you can omit any option and its value will be maintained from the previous version (if one is available).

is the following object a representation of the default options? added note here in case that is so, to make it explicit. otherwise, we could include a link to the file/code that contains said default options.

also, if I omit an option because I'm trying to clear it for whatever reason, but it was already set (and therefor the existing value will be maintained), should we explain here how to remove it?

justincorrigible · 2025-01-28T12:41:41Z

+
+### External Validation
+
+External validations configure Song to check a value in the analysis against an external service by sending an HTTP GET request to a configurable URL. The URL needs to return 2XX status message to indicate that if the provided value "is valid", typically meaning that the value for this property is known by that service.


Suggested change

External validations configure Song to check a value in the analysis against an external service by sending an HTTP GET request to a configurable URL. The URL needs to return 2XX status message to indicate that if the provided value "is valid", typically meaning that the value for this property is known by that service.

External validations configure Song to check a value in the analysis against an external service, by sending an HTTP GET request to a configurable URL. The service should respond with a 2XX status message to indicate the value "is valid".

Including all these changes in my next commit, some of them conflict with a change I made so I'll commit the suggestions but not through the GH UI.

justincorrigible · 2025-01-28T12:44:42Z

+}
+```
+
+Song would attempt to validate the donorId by sending a URL to `http://example.com/ABC123/donor/id01`.


Suggested change

Song would attempt to validate the donorId by sending a URL to `http://example.com/ABC123/donor/id01`.

Song would attempt to validate the donorId by sending a validation request to `http://example.com/ABC123/donor/id01`.

justincorrigible · 2025-01-28T12:47:32Z

+
+Song would attempt to validate the donorId by sending a URL to `http://example.com/ABC123/donor/id01`.
+
+It is allowable to use either the `{study}` or `{value}` multiple times in the URL, each instance will be replaced by the corresponding value.


Suggested change

It is allowable to use either the `{study}` or `{value}` multiple times in the URL, each instance will be replaced by the corresponding value.

The URL parsing allows using either the `{study}` or `{value}` placeholders multiple times (e.g. `http://example.com/{study}-{value}/{value}`), each instance will be interpolated accordingly.

justincorrigible · 2025-01-28T12:49:24Z

+It is allowable to use either the `{study}` or `{value}` multiple times in the URL, each instance will be replaced by the corresponding value.
+
+> [!Warning]
+> The URL may cause errors in song if it contains any tokens matching the `{word}` format other than `{study}` and `{value}`


Changing this here for consistency with the rest of your additions... and now I realize I've been noticing we spell SONG/Song/song in different places (beyond this repo).
Is it worth making a pass in a different PR to unify those here? if so, which spelling should we use? in Overture docs, we've been using all caps. TBD

Suggested change

> The URL may cause errors in song if it contains any tokens matching the `{word}` format other than `{study}` and `{value}`

> The URL may cause errors in Song if it contains any tokens matching the `{word}` format other than `{study}` and `{value}`

justincorrigible · 2025-01-28T12:51:17Z

  }

  private boolean isIdSearchMode() {
-    return nonNull(fileId) || nonNull(sampleId) || nonNull(specimenId) || nonNull(donorId);


justincorrigible · 2025-01-28T12:56:14Z

-import lombok.EqualsAndHashCode;
-import lombok.NoArgsConstructor;
-import lombok.ToString;
+import lombok.*;


How's Maven's tree shaking these days? I recall this `.* pattern could add some pounds to the code back when

I'm not sure on this but I know the common practice for code formatters is that after a minimum number of imports from a package the formatter will switch to the .* form. So I would assume there is not a meaningful package size change associated with this. Important to know that this affects docker image size, less impactful than a web page bundle size increase.

justincorrigible · 2025-01-28T13:00:27Z

This makes me soooooo happy! 👏

justincorrigible · 2025-01-28T13:08:17Z

-                  .setSubmitterSpecimenId(randomGenerator.generateRandomUUID().toString());
-              x.getDonor().setSubmitterDonorId(randomGenerator.generateRandomUUID().toString());
-            });
+    ;


typo?

Suggested change

;

joneubank · 2025-01-28T14:59:44Z

I changed the optional property name from externalValidation to externalValidations to be consistent with naming of array properties. It matches fileTypes now (both plural).

justincorrigible · 2025-01-28T15:06:06Z

+
+## Options
+
+The `options` property is not required, if provided extra validations for this analysis can be specified. Similarly, each property in `options` is also optional. If no value is provided for an option, a default configuration will be used for the analysis. If this is an update to an existing analysis type, you can omit any option and its value will be maintained from the previous version.


Suggested change

The `options` property is not required, if provided extra validations for this analysis can be specified. Similarly, each property in `options` is also optional. If no value is provided for an option, a default configuration will be used for the analysis. If this is an update to an existing analysis type, you can omit any option and its value will be maintained from the previous version.

The `options` property is not required. If provided, extra validations for this analysis can be specified. Similarly, each property in `options` is also optional. If no value is provided for an option, a default configuration will be used for the analysis. If this is an update to an existing analysis type, you can omit any option and its value will be maintained from the previous version.

justincorrigible · 2025-01-28T15:07:17Z

+
+If you want to remove the previous value of an option so that this validation is no longer required, for instance removing the restriction on file types so that any file type could be provided, you should provide an empty list for that option.
+
+In the example below, both `fileTypes` and `externalValidations` properties are set to empty arrays, which means that these validations will not be applied to submitted analysis:


love it. very helpful description 👍

Azher2Ali and others added 19 commits October 11, 2024 15:52

Remove Donor, Specimen, and Sample from Analysis Data Classes in SONG

51ef77f

Reverting the changes related to latest develop

38a8547

Resolving conflicts and reverting changes to fileUpdateRequest.json

7158328

Merge pull request #869 from overture-stack/refactor/remove-donor-spe…

a512322

…cimen-sample-from-analysis Remove Donor, Specimen, and Sample from Analysis Data Classes in SONG

Remove Donor, Specimen, and Samples from Base Schema and Allow in Dyn…

258e013

…amic Schemas

Remove Donor-Related Entities from StudyController and Search APIs

7da7be2

Merge pull request #870 from overture-stack/refactor/remove-donor-spe…

161d8d0

…cimen-sample-from-base-schema Remove Donor, Specimen, and Samples from Base Schema and Allow in Dynamic Schemas

Merge pull request #873 from overture-stack/refactor/remove-donor-spe…

516ccea

…cimen-sample-search Remove Donor/Specimen/Sample References from Study and Search APIs

Add External Validation Rules for Analysis Properties Based on Dynami…

fa644ec

…c Schemas

Resolving the build issues and updating the PR with missing files

f4f376f

Changes related to feedback

ffde783

Changes to ValidationService related to externalValiation

fe20ec7

Changes related to the feedback comments

fe0883f

Migrate file_types to options column in analysis_schema

2065205

External valdiation URL template for study and value tokens

57e83af

Retrieve all options data with analysis type

a09c88e

Remove sneaky throws from buildSchema with JSON parsing failure case

6fd36b1

Merge pull request #877 from overture-stack/feature/external-validati…

dd4c9f8

…on-rules-for-analysis Add External Validation Rules for Analysis Properties Based on Dynamic Schema

Fix paginated data lookup by removing stored procedure reference to f…

26ee1cf

…ile_type enum

joneubank marked this pull request as ready for review January 27, 2025 21:17

joneubank changed the title ~~Feat/refactor analyis data model~~ Refactor Base Schema to not require Donor/Specimen/Sample Jan 28, 2025

joneubank requested review from Azher2Ali, justincorrigible and leoraba January 28, 2025 04:06

justincorrigible reviewed Jan 28, 2025

View reviewed changes

joneubank added 3 commits January 28, 2025 09:52

Change schema option name to plural externalValidations

e0b96be

Docs grammar improvements and update property to externalValidations

19d9e32

Remove blank lines

b74d0e0

joneubank requested a review from justincorrigible January 28, 2025 14:59

justincorrigible reviewed Jan 28, 2025

View reviewed changes

Options property description improvements

eb0f328

justincorrigible approved these changes Jan 28, 2025

View reviewed changes

joneubank merged commit 03acf8b into develop Jan 28, 2025

joneubank deleted the feat/refactor-analyis-data-model branch January 28, 2025 15:11

joneubank mentioned this pull request May 29, 2025

Rc/5.3.0 #886

Merged


		## Options

		All properties in options are optional. If no value is provided, a default configuration will be used for the analysis. If this is an update to an existing analysis type, you can omit any option and the value will be maintained from the previous version.

	All properties in options are optional. If no value is provided, a default configuration will be used for the analysis. If this is an update to an existing analysis type, you can omit any option and the value will be maintained from the previous version.
	All properties in "options" are elective. If no value is provided, a default configuration (shown bellow) will be used for the analysis. If this is an update to an existing analysis type, you can omit any option and its value will be maintained from the previous version (if one is available).


		### External Validation

		External validations configure Song to check a value in the analysis against an external service by sending an HTTP GET request to a configurable URL. The URL needs to return 2XX status message to indicate that if the provided value "is valid", typically meaning that the value for this property is known by that service.

	Song would attempt to validate the donorId by sending a URL to `http://example.com/ABC123/donor/id01`.
	Song would attempt to validate the donorId by sending a validation request to `http://example.com/ABC123/donor/id01`.


		Song would attempt to validate the donorId by sending a URL to `http://example.com/ABC123/donor/id01`.

		It is allowable to use either the `{study}` or `{value}` multiple times in the URL, each instance will be replaced by the corresponding value.

	It is allowable to use either the `{study}` or `{value}` multiple times in the URL, each instance will be replaced by the corresponding value.
	The URL parsing allows using either the `{study}` or `{value}` placeholders multiple times (e.g. `http://example.com/{study}-{value}/{value}`), each instance will be interpolated accordingly.

	> The URL may cause errors in song if it contains any tokens matching the `{word}` format other than `{study}` and `{value}`
	> The URL may cause errors in Song if it contains any tokens matching the `{word}` format other than `{study}` and `{value}`


		## Options

		The `options` property is not required, if provided extra validations for this analysis can be specified. Similarly, each property in `options` is also optional. If no value is provided for an option, a default configuration will be used for the analysis. If this is an update to an existing analysis type, you can omit any option and its value will be maintained from the previous version.

	The `options` property is not required, if provided extra validations for this analysis can be specified. Similarly, each property in `options` is also optional. If no value is provided for an option, a default configuration will be used for the analysis. If this is an update to an existing analysis type, you can omit any option and its value will be maintained from the previous version.
	The `options` property is not required. If provided, extra validations for this analysis can be specified. Similarly, each property in `options` is also optional. If no value is provided for an option, a default configuration will be used for the analysis. If this is an update to an existing analysis type, you can omit any option and its value will be maintained from the previous version.


		If you want to remove the previous value of an option so that this validation is no longer required, for instance removing the restriction on file types so that any file type could be provided, you should provide an empty list for that option.

		In the example below, both `fileTypes` and `externalValidations` properties are set to empty arrays, which means that these validations will not be applied to submitted analysis:

Conversation

joneubank commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Reference

Summary

Changes

Bug Fixes

Uh oh!

justincorrigible left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justincorrigible Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joneubank commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joneubank commented Jan 22, 2025 •

edited

Loading

justincorrigible Jan 28, 2025 •

edited

Loading

joneubank commented Jan 28, 2025 •

edited

Loading