Add support for confounding variables #877

KochTobi · 2024-10-30T09:17:19Z

Addresses #642

KochTobi · 2024-10-30T09:18:20Z

As confounding variables are valid for all samples in an experiment (including samples where a value might not yet been known), I decided to go for including empty variables in the samples.

sonarqubecloud · 2024-11-19T09:09:00Z

Quality Gate passed

Issues
9 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

@transactional

> The Spring team recommends that you annotate methods of concrete classes with the @transactional annotation, rather than relying on annotated methods in interfaces, even if the latter does work for interface-based and target-class proxies as of 5.0. https://docs.spring.io/spring-framework/reference/data-access/transaction/declarative/annotations.html

…etadata-to-add-confounding-variables # Conflicts: # user-interface/src/main/bundles/dev.bundle

…etadata-to-add-confounding-variables

…etadata-to-add-confounding-variables # Conflicts: # user-interface/src/main/bundles/dev.bundle

…etadata-to-add-confounding-variables

KochTobi · 2025-01-22T15:04:27Z

For this to work, sql/complete-schema.sql has to be executed on the database

sven1103 · 2025-01-27T07:36:58Z

@KochTobi there are some conflicts with the target branch development

sven1103 · 2025-01-27T07:39:56Z

@KochTobi can you elaborate more on the changes in the database? I also encourage to look into database versioning solutions like Flyway. I have some concerns with our current database schema development strategy: hoping for the best and no way back.

sven1103

Some minor change requests, but I have some concerns with how we deal with database changes.

...-management/src/main/java/life/qbic/projectmanagement/application/sample/SampleMetadata.java

sven1103 · 2025-01-27T07:48:29Z

...rc/main/java/life/qbic/projectmanagement/application/sample/SampleRegistrationServiceV2.java

      throws RegistrationException {
    var result = batchRegistrationService.registerBatch(batchLabel, batchIsPilot, projectId,
-        experimentId);
+        ExperimentId.parse(experiment.id()));


I think we overdid it here big time with the Identifier... Imo there is no need to parse, because it does not have any semantics or schema. It is just a string value generated from the UUID generator. You either find an object with the ID in the database, or you dont.
Happy to see some simplification.

sven1103 · 2025-01-27T07:49:20Z

...rc/main/java/life/qbic/projectmanagement/application/sample/SampleRegistrationServiceV2.java

  }

  @PreAuthorize("hasPermission(#projectId, 'life.qbic.projectmanagement.domain.model.project.Project', 'WRITE')")
  @Async
+  @Transactional(propagation = Propagation.REQUIRES_NEW)


What does this annotation do? Why is it needed?

The transactional annotation is needed to indicate a transaction taking place. It starts a transaction in the database and commits changes when exiting the method. The propagation property tells spring that a new transaction is to be created for this operation. The default behaviour is that an existing active transaction is used and if there is none that one is created. I do not think we need to create a new transaction here so I removed the propagation type.

So this guarantees the transactional scope of the method? I am not sure, if I get it. Can you explain with an example, what would happen without it and what undesired side effects it has?

There are different scenarios:

no @Transactional annotation

a @Transactional annotation without Propagation.REQUIRES_NEW

a @Transactional annotation the default propagation

in 1. there will be an exception if the method is called from outside a transactional context. In our case no transactional context exists so the exception will be thrown. Without any transaction, no rollback is possible

in 2. at the start of the method a new transaction will be created, suspending an existing transaction if any already exists in this scope. The changes are made within the new transaction which is committed when the method exits (or rolled back in case of exceptions). After the newly created transaction finishes (commit or rollback) the previously suspended transaction continues.

in 3. when called from a transactional scope (already existing transaction) the operations within the method are appended/added to the existing transaction. If the method throws an exception the complete transaction (including the changes made before this method was called) are rolled back. If no transaction existed when the method is called, a new transaction is created.

The difference between 2. and 3. is that in 2. a transaction is suspended. Rollbacks do not affect a scope outside of the method in 2. Suspending transactions bears a risk of deadlocking the system. If the amount of maximal transactions is limited and the creation of the transaction fails, there might be a scenario where the original transaction is kept in a suspended state further limiting the amount of available transactions.
We also want to rollback an existing transaction in case of exceptions so only rolling back changes made in this method is not what we want I think.
Thus the propagation type of REQUIRES_NEW was wrong here.

Further information: https://docs.spring.io/spring-framework/reference/data-access/transaction/declarative/annotations.html

...rc/main/java/life/qbic/projectmanagement/application/sample/SampleRegistrationServiceV2.java

…etadata-to-add-confounding-variables # Conflicts: # user-interface/src/main/bundles/dev.bundle # user-interface/src/main/java/life/qbic/datamanager/templates/sample/SampleBatchUpdateTemplate.java

KochTobi · 2025-01-27T08:45:13Z

@KochTobi there are some conflicts with the target branch development

Yes I resolved it.

KochTobi · 2025-01-27T08:49:03Z

@KochTobi can you elaborate more on the changes in the database? I also encourage to look into database versioning solutions like Flyway. I have some concerns with our current database schema development strategy: hoping for the best and no way back.

Hi Sven. I simply merged all the sql files to set up the database into one and added the tables for confounding variables. In this case the database schema is simply extended by the tables of confounding variables (samples, experiment and so forth are not touched).
I will happily look into Flyway or other database migration solutions. In the case of this PR I only recorded the current database scheme we have (plus the tables I added).

Even thou the topic of database migration and future proofing is of great importance, I would like to separate the investigation on database migration and future proofing from this PR.

Co-authored-by: Sven F. <[email protected]>

I don't think we need a new transaction when creating samples Co-authored-by: Sven F. <[email protected]>

…etadata-to-add-confounding-variables

Co-authored-by: Sven F. <[email protected]>

sonarqubecloud · 2025-01-28T10:28:48Z

Quality Gate failed

Failed conditions
1 Security Hotspot
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

sven1103

Great work @KochTobi, i am looking forward to the users feedback!

KochTobi · 2025-01-28T11:04:59Z

@KochTobi can you elaborate more on the changes in the database? I also encourage to look into database versioning solutions like Flyway. I have some concerns with our current database schema development strategy: hoping for the best and no way back.

Hi Sven. I simply merged all the sql files to set up the database into one and added the tables for confounding variables. In this case the database schema is simply extended by the tables of confounding variables (samples, experiment and so forth are not touched). I will happily look into Flyway or other database migration solutions. In the case of this PR I only recorded the current database scheme we have (plus the tables I added).

Even thou the topic of database migration and future proofing is of great importance, I would like to separate the investigation on database migration and future proofing from this PR.

#1000 <- to track the need

add todo to open PR

c6cc4fd

github-actions bot added the feature label Oct 30, 2024

KochTobi linked an issue Nov 4, 2024 that may be closed by this pull request

Editing registered metadata to add confounding variables #642

Closed

KochTobi added 13 commits November 4, 2024 14:58

add mermaid plot

13e9651

change to identifier

4178a4f

add sequence diagrams

784327d

update sql scripts

57029c3

Pass schema validation

bc1f9df

Fix order of statements

8d06b5b

do not allow null values for measurement counts

ca4e8c6

refine diagram

ecba811

update sql so it only creates missing tables

458c459

minimal example

294b2e3

add javadoc

7477a5a

wip

98b6a1c

add confounding variables to templates

36d4e4b

KochTobi added 12 commits November 25, 2024 10:00

wip

8979218

Merge branch 'development' into feature-642/#642-editing-registered-m…

ec74ed7

…etadata-to-add-confounding-variables # Conflicts: # user-interface/src/main/bundles/dev.bundle

Merge branch 'development' into feature-642/#642-editing-registered-m…

aaba171

…etadata-to-add-confounding-variables

Merge branch 'development' into feature-642/#642-editing-registered-m…

f26fa2d

…etadata-to-add-confounding-variables

Merge branch 'development' into feature-642/#642-editing-registered-m…

a532a67

…etadata-to-add-confounding-variables # Conflicts: # user-interface/src/main/bundles/dev.bundle

wip

559859d

wip

e510f90

Merge branch 'development' into feature-642/#642-editing-registered-m…

48b5f1c

…etadata-to-add-confounding-variables

Fix file size formatting

5aa1341

Fix connection testing for openbis

d7f0b89

Add equals and hash code to Sample.java object

8bf584d

KochTobi added 4 commits January 22, 2025 14:02

Prevent creation and rename of duplicate confounding variable

03702a3

Prevent duplicate naming and accidental deletion

c7e0a15

prettify frontend

8039cd0

Fix groups not being loaded

aad7966

KochTobi marked this pull request as ready for review January 22, 2025 14:28

KochTobi requested a review from a team as a code owner January 22, 2025 14:28

KochTobi added 4 commits January 22, 2025 15:34

Merge branch 'development' into feature-642/#642-editing-registered-m…

c97bf21

…etadata-to-add-confounding-variables

remove file

329b25d

Address sonar cloud analysis of SampleRepositoryImpl.java

4e8e94e

address further sonarcloud issues

5b0befc

sven1103 self-requested a review January 27, 2025 07:23

sven1103 requested changes Jan 27, 2025

View reviewed changes

Merge branch 'development' into feature-642/#642-editing-registered-m…

a42a936

…etadata-to-add-confounding-variables # Conflicts: # user-interface/src/main/bundles/dev.bundle # user-interface/src/main/java/life/qbic/datamanager/templates/sample/SampleBatchUpdateTemplate.java

KochTobi and others added 2 commits January 27, 2025 09:50

Rename method

f3f2c69

Co-authored-by: Sven F. <[email protected]>

remove Propagation.REQUIRES_NEW

867d0c7

I don't think we need a new transaction when creating samples Co-authored-by: Sven F. <[email protected]>

KochTobi requested a review from sven1103 January 27, 2025 09:05

KochTobi and others added 3 commits January 27, 2025 18:03

Merge branch 'development' into feature-642/#642-editing-registered-m…

c09a8e3

…etadata-to-add-confounding-variables

Replace Map#foreach with Collectors#toMap

757cd32

Co-authored-by: Sven F. <[email protected]>

Ask before deleting confounding variables

66aea21

Co-authored-by: Sven F. <[email protected]>

sven1103 approved these changes Jan 28, 2025

View reviewed changes

KochTobi merged commit 7042182 into development Jan 28, 2025
5 of 6 checks passed

KochTobi deleted the feature-642/#642-editing-registered-metadata-to-add-confounding-variables branch January 28, 2025 11:05

KochTobi mentioned this pull request Feb 12, 2025

Release PR #1013

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for confounding variables #877

Add support for confounding variables #877

KochTobi commented Oct 30, 2024

KochTobi commented Oct 30, 2024

sonarqubecloud bot commented Nov 19, 2024

KochTobi commented Jan 22, 2025

sven1103 commented Jan 27, 2025

sven1103 commented Jan 27, 2025

sven1103 left a comment

sven1103 Jan 27, 2025

KochTobi Jan 27, 2025

sven1103 Jan 27, 2025

KochTobi Jan 27, 2025 •

edited

Loading

sven1103 Jan 27, 2025

KochTobi Jan 28, 2025 •

edited

Loading

KochTobi commented Jan 27, 2025

KochTobi commented Jan 27, 2025

sonarqubecloud bot commented Jan 28, 2025

sven1103 left a comment

KochTobi commented Jan 28, 2025

Add support for confounding variables #877

Add support for confounding variables #877

Conversation

KochTobi commented Oct 30, 2024

KochTobi commented Oct 30, 2024

sonarqubecloud bot commented Nov 19, 2024

Quality Gate passed

KochTobi commented Jan 22, 2025

sven1103 commented Jan 27, 2025

sven1103 commented Jan 27, 2025

sven1103 left a comment

Choose a reason for hiding this comment

sven1103 Jan 27, 2025

Choose a reason for hiding this comment

KochTobi Jan 27, 2025

Choose a reason for hiding this comment

sven1103 Jan 27, 2025

Choose a reason for hiding this comment

KochTobi Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

sven1103 Jan 27, 2025

Choose a reason for hiding this comment

KochTobi Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

KochTobi commented Jan 27, 2025

KochTobi commented Jan 27, 2025

sonarqubecloud bot commented Jan 28, 2025

Quality Gate failed

sven1103 left a comment

Choose a reason for hiding this comment

KochTobi commented Jan 28, 2025

KochTobi Jan 27, 2025 •

edited

Loading

KochTobi Jan 28, 2025 •

edited

Loading