[DC-1120]Update Snapshot Builder Settings to include Dataset Table Name and Relationships #1732

rjohanek · 2024-07-09T19:15:47Z

Jira ticket: https://broadworkbench.atlassian.net/browse/1120

Summary of Changes

Update snapshot builder settings

Remove "featureValueGroups"
Rework datasetConceptSets
Remove "featureValueGroupName"
Add “table” object that follows the model of the “SnapshotBuilderTable” that includes datasetTableName, columns and relationships
We’ll leave columns empty for now so that all columns are included in the asset

Rework the Snapshot Builder Access Request

Remove "conceptSets"
Rename "valueSets" to "outputTables" and the “values” list to instead be “columns”
Return an empty list for “columns” for now.

Update how the asset is generated on snapshot create by request id

Remove the manual mapping between the concept name and the table object
Instead, pull the table object from snapshot builder settings based on the name of the outputTable/ConceptSet

There will be a follow up UI change to handle changes to settings/access request - Not yet complete

Testing Strategy

Updated existing snapshotServiceTests
Integration and connected tests as they are should test this already since this is not adding functionality but replacing hardcoded functionality

rjohanek · 2024-07-09T19:17:14Z

src/test/java/bio/terra/service/snapshotbuilder/utils/constants/Observation.java

+public final class Observation {
+  private Observation() {}
+
+  public static final String TABLE_NAME = "observation";


Not that it really matters because it is test data but I noticed there is no observation occurrence table, just observation table in OMOP so I renamed this class

rjohanek · 2024-07-09T19:19:31Z

src/test/java/bio/terra/service/snapshotbuilder/SnapshotBuilderTestData.java

+                      .table(new SnapshotBuilderTable().datasetTableName(Observation.TABLE_NAME)),
+                  new SnapshotBuilderDatasetConceptSet()
+                      .name("Measurement")
+                      .table(new SnapshotBuilderTable().datasetTableName(Measurement.TABLE_NAME)),


Minor comment: I could make a constant like I did for Measurement and use the table name like shown here, or just use the string table names like shown below (because they are only used once), or I could just define constants in this class

pshapiro4broad · 2024-07-10T14:15:09Z

src/main/java/bio/terra/service/snapshot/SnapshotService.java

@@ -688,7 +689,7 @@ public SnapshotAccessRequestResponse getSnapshotAccessRequestById(UUID accessReq
  }

  public AssetSpecification buildAssetFromSnapshotAccessRequest(
-      Dataset dataset, SnapshotAccessRequestResponse snapshotRequestModel) {
+      Dataset dataset, SnapshotAccessRequestResponse snapshotAccessRequest) {


Would it be too much to rename the type as well, in the openapi spec? My thinking here is that the response from the API happens to be a SnapshotAccessRequest object, but not all SnapshotAccessRequest objects are actually a response from the API. Since we use the "response" object in non-response situations, it doesn't make sense for response to be part of the type name.

I'm not sure, maybe there is a better name. We have a SnapshotAccessRequest object that represents the request submitted from the user, not something retrieved from the database, rather the input to the database. This is a subset of the data in the SnapshotAccessRequestResponse object which is always the object retrieved from the database, so in a way it kind of is a response. If we want to keep these types distinct, we could leave it as is or some up with a more precise name. If we don't want these types to be distinct, we could make the fields that don't exist on the SnapshotAccessRequest optional on the SnapshotAccessRequestResponse object and use that to represent both. But, I think I might prefer leaving them distinct

I agree to keep the types separate. I do think we may want to separate the database type from the response type at some point; there may be some data that isn't in the database that we'd want to return, or vice versa. It also makes things confusing in that "request" sometimes means an API request and sometimes means an access request (which itself is part of an API request).

We could swap it to SnapshotAccessRequestRequest and SnapshotAccessRequest? Because technically it is making a request for a SnapshotAccessRequest. But I also think it is okay to leave it as is, at least for now.

pshapiro4broad

This looks OK to me.

src/main/java/bio/terra/service/snapshot/SnapshotService.java

pshapiro4broad · 2024-07-10T19:54:35Z

src/main/java/bio/terra/service/snapshot/SnapshotService.java

@@ -688,7 +689,7 @@ public SnapshotAccessRequestResponse getSnapshotAccessRequestById(UUID accessReq
  }

  public AssetSpecification buildAssetFromSnapshotAccessRequest(
-      Dataset dataset, SnapshotAccessRequestResponse snapshotRequestModel) {
+      Dataset dataset, SnapshotAccessRequestResponse snapshotAccessRequest) {


I agree to keep the types separate. I do think we may want to separate the database type from the response type at some point; there may be some data that isn't in the database that we'd want to return, or vice versa. It also makes things confusing in that "request" sometimes means an API request and sometimes means an access request (which itself is part of an API request).

pshapiro4broad · 2024-07-10T19:58:34Z

src/test/resources/omop/settings.json

+        "primaryTableRelationship": "fpk_person_drug",
+        "secondaryTableRelationships": [
+          "fpk_drug_type_concept",


I think that we should be leveraging TDR's relationship objects here instead of storing fpk names. (So instead of storing the primary table fpk, we'd store the field of the table that contains the value which has a fpk relationship to the primary table.) But that change can be done later, since this PR is about changing where this data is stored, not how it's represented.

src/test/resources/omop/settings.json

… exist yet

# Conflicts: # src/test/java/bio/terra/service/snapshot/SnapshotServiceTest.java

rjohanek · 2024-07-15T17:10:30Z

src/main/java/bio/terra/service/snapshot/SnapshotService.java

    tables.forEach(
        table -> {
          assetModel.addTablesItem(
              new AssetTableModel().name(table.getDatasetTableName()).columns(table.getColumns()));
          // First add all person <-> occurrence relationships
-          assetModel.addFollowItem(table.getPrimaryTableRelationship());
+          if (table.getPrimaryTableRelationship() != null) {
+            assetModel.addFollowItem(table.getPrimaryTableRelationship());


for the person table, table.getPrimaryTableRelationship() is null. This was causing an issue when demographics was selected because there was a null item in the follow list

Given that we're now adding the root table separately, do we still need this check?

Hmm yeah I think I could make the primary table relationship required on SnapshotBuilderTable now. But that means I can't use the allOf inheritance in the yaml file so it's a little less pretty but worth it because it will more accurately capture the types we want. Unless there's some way to say allOf ..except? Or I could create a base type and have them both inherit all of that, but since that base type wouldn't be used anywhere we probably don't want it in the yaml. So I'll just redefine SnapshotBuilderTable and SnapshotBuilderRootTable separately

rjohanek · 2024-07-15T17:11:53Z

src/main/java/bio/terra/service/snapshot/SnapshotService.java

-        table -> table.getSecondaryTableRelationships().forEach(assetModel::addFollowItem));
+        table -> {
+          if (table.getSecondaryTableRelationships() != null) {
+            table.getSecondaryTableRelationships().forEach(assetModel::addFollowItem);


I added the null check here too because when we do include the sample table, it won't have secondary relationships which will cause the same issue I saw above for the person table.

sonarqubecloud · 2024-07-17T13:29:57Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarCloud

s-rubenstein

Approved with a few comments.

s-rubenstein · 2024-07-18T17:57:29Z

src/main/java/bio/terra/service/snapshot/SnapshotService.java

@@ -688,7 +689,7 @@ public SnapshotAccessRequestResponse getSnapshotAccessRequestById(UUID accessReq
  }

  public AssetSpecification buildAssetFromSnapshotAccessRequest(
-      Dataset dataset, SnapshotAccessRequestResponse snapshotRequestModel) {
+      Dataset dataset, SnapshotAccessRequestResponse snapshotAccessRequest) {


We could swap it to SnapshotAccessRequestRequest and SnapshotAccessRequest? Because technically it is making a request for a SnapshotAccessRequest. But I also think it is okay to leave it as is, at least for now.

s-rubenstein · 2024-07-18T17:58:55Z

src/main/java/bio/terra/service/snapshot/SnapshotService.java

    tables.forEach(
        table -> {
          assetModel.addTablesItem(
              new AssetTableModel().name(table.getDatasetTableName()).columns(table.getColumns()));
          // First add all person <-> occurrence relationships
-          assetModel.addFollowItem(table.getPrimaryTableRelationship());
+          if (table.getPrimaryTableRelationship() != null) {
+            assetModel.addFollowItem(table.getPrimaryTableRelationship());


Given that we're now adding the root table separately, do we still need this check?

s-rubenstein · 2024-07-18T18:01:23Z

src/main/java/bio/terra/service/snapshot/SnapshotService.java

-    Set<String> missing = new HashSet<>(valueSetNames);
-    missing.removeAll(tableMap.keySet());
+    Set<String> missing = new HashSet<>(includedTableNames);
+    allTables.stream().map(SnapshotBuilderDatasetConceptSet::getName).forEach(missing::remove);
    if (!missing.isEmpty()) {
      throw new IllegalArgumentException("Unknown value set names: " + missing);


Nit: What do you think about changing this error message now that we don't have value sets?

yeah, maybe unknown concept set name?

s-rubenstein · 2024-07-18T18:04:20Z

src/test/java/bio/terra/service/snapshotbuilder/SnapshotBuilderTestData.java

+                      .name("Procedure")
+                      .table(
+                          new SnapshotBuilderTable()
+                              .datasetTableName(ProcedureOccurrence.TABLE_NAME)),


Do we want to allow SnapshotBuilderTable objects to have no primaryTableRelationships or secondaryTableRelationships?

Yeah, I didn't populate it here because it isn't used for tests, but based on your other comment, I think we could make primary table relationship required, so then I'd need to populate that here

Is there a reason for other non-omop datasets that we wouldn't want to make this required?

Discussed offline - its tricky to represent the ideal state so we're just leaving them optional for now.

…me and Relationships (#1732) Co-authored-by: rjohanek <rjohanek@broadinstitute.org>

raejohanek added 2 commits July 9, 2024 10:08

update settings and snapshot access request types

8b37723

pull tables form settings and update test data

955ae07

rjohanek requested review from a team as code owners July 9, 2024 19:15

rjohanek requested review from fboulnois and aarohinadkarni and removed request for a team July 9, 2024 19:15

rjohanek commented Jul 9, 2024

View reviewed changes

update settings to match dev

3699d67

pshapiro4broad reviewed Jul 10, 2024

View reviewed changes

pshapiro4broad approved these changes Jul 10, 2024

View reviewed changes

raejohanek added 2 commits July 12, 2024 13:56

remove unnecessary toList call

3026a9f

spotlessApply

caa14d3

snf2ye reviewed Jul 12, 2024

View reviewed changes

src/test/resources/omop/settings.json Outdated Show resolved Hide resolved

raejohanek added 2 commits July 15, 2024 09:33

remove genomics from datasetConceptSets because sample table does not…

c25db68

… exist yet

Merge branch 'develop' into rj/dc-1120-update-settings

365ab9f

# Conflicts: # src/test/java/bio/terra/service/snapshot/SnapshotServiceTest.java

fboulnois removed their request for review July 15, 2024 13:59

account for tables with primary or secondary relationships

2fb7a90

rjohanek commented Jul 15, 2024

View reviewed changes

raejohanek added 4 commits July 15, 2024 13:13

update settings to match what was hardcoded in asset creation

889cfe2

add rootTable and dictionaryTable to settings

bd61314

use test data for update settings tests

9be742e

delete measurement table for test data

ef8bd5e

snf2ye approved these changes Jul 17, 2024

View reviewed changes

s-rubenstein approved these changes Jul 18, 2024

View reviewed changes

rjohanek merged commit 6ef160b into develop Jul 19, 2024
13 checks passed

rjohanek deleted the rj/dc-1120-update-settings branch July 19, 2024 14:32

Shakespeared pushed a commit that referenced this pull request Jul 23, 2024

[DC-1120]Update Snapshot Builder Settings to include Dataset Table Na…

17faec9

…me and Relationships (#1732) Co-authored-by: rjohanek <rjohanek@broadinstitute.org>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DC-1120]Update Snapshot Builder Settings to include Dataset Table Name and Relationships #1732

[DC-1120]Update Snapshot Builder Settings to include Dataset Table Name and Relationships #1732

rjohanek commented Jul 9, 2024 •

edited

Loading

rjohanek Jul 9, 2024

rjohanek Jul 9, 2024

pshapiro4broad Jul 10, 2024

rjohanek Jul 10, 2024

pshapiro4broad Jul 10, 2024

s-rubenstein Jul 18, 2024

pshapiro4broad left a comment

pshapiro4broad Jul 10, 2024

pshapiro4broad Jul 10, 2024

rjohanek Jul 15, 2024

s-rubenstein Jul 18, 2024

rjohanek Jul 18, 2024

rjohanek Jul 15, 2024

sonarqubecloud bot commented Jul 17, 2024

s-rubenstein left a comment

s-rubenstein Jul 18, 2024

s-rubenstein Jul 18, 2024

s-rubenstein Jul 18, 2024

rjohanek Jul 18, 2024

s-rubenstein Jul 18, 2024

rjohanek Jul 18, 2024

rjohanek Jul 18, 2024

s-rubenstein Jul 18, 2024

[DC-1120]Update Snapshot Builder Settings to include Dataset Table Name and Relationships #1732

[DC-1120]Update Snapshot Builder Settings to include Dataset Table Name and Relationships #1732

Conversation

rjohanek commented Jul 9, 2024 • edited Loading

Summary of Changes

Testing Strategy

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshapiro4broad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jul 17, 2024

Quality Gate passed

s-rubenstein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjohanek commented Jul 9, 2024 •

edited

Loading