[DC-781] Create service endpoint stub for creating a Snapshot Request #1537

rjohanek · 2023-11-09T21:01:30Z

Create stub service endpoint for requesting access to a snapshot (creating a request)

src/main/java/bio/terra/service/snapshotbuilder/SnapshotBuilderService.java

src/main/resources/api/data-repository-openapi.yaml

src/test/java/bio/terra/app/controller/DatasetsApiControllerTest.java

pshapiro4broad

I'm not sure the oneOf yaml will work; adding a test that specifies a complete snapshot request would be a way to help validate this.

src/main/java/bio/terra/service/snapshotbuilder/SnapshotBuilderService.java

pshapiro4broad · 2023-11-14T18:59:17Z

src/main/resources/api/data-repository-openapi.yaml

+      description: Criteria Group specifying a Cohort.
+
+    SnapshotBuilderAnyCriteria:
+      oneOf:


Wow, it will be great if oneOf really works now. I've been avoiding it for a while because the java codegen wasn't usable.

Looking at the code this generates, I'm not sure it's working yet. This generates a type SnapshotBuilderAnyCriteria which is what SnapshotBuilderCriteriaGroup uses for its criteria field. However the oneOf subtypes SnapshotBuilderDomainOption and SnapshotBuilderProgramDataOption are not subtypes of SnapshotBuilderAnyCriteria. They are subtypes of OneOfSnapshotBuilderAnyCriteria (which make them sibling types of SnapshotBuilderAnyCriteria).

Long story short, I don't see how in Java we can get from SnapshotBuilderCriteriaGroup.criteria to a subtype class like SnapshotBuilderDomainOption. I'd like to see an example of that before merging this change in.

Ah okay I see I will look into this. Instead of defining AnyCriteria as its own component, I could define SnapshotBuilderCriteriaGroup.criteria as a list of type SnapshotBuilderDomainOption or SnapshotBuilderProgramDataOption maybe

Okay removing the explicit definition of AnyCriteria seems to get rid of the extra class definition. So now there is OneOfSnapshotBuilderCriteriaGroupCriteriaItems which has two subtypes SnapshotBuilderDomainOption or SnapshotBuilderProgramDataOption. The name is not ideal, but I wonder what you think of this approach.

pshapiro4broad · 2023-11-14T19:09:39Z

src/test/java/bio/terra/app/controller/DatasetsApiControllerTest.java

+        new SnapshotBuilderAccessRequest()
+            .name("name")
+            .researchPurposeStatement("purpose")
+            .datasetRequest(new SnapshotBuilderRequest());


It's a bit messy but I think we need to test the entire API as specified in the yaml, so the request needs to include cohorts, concept sets and value set.

pshapiro4broad · 2023-11-15T15:21:40Z

src/main/resources/api/data-repository-openapi.yaml

+    post:
+      tags:
+        - datasets
+      description: Create a new snapshot request.


I wonder if a user journey type description would be helpful here. Something like "As a researcher, create a snapshot request for a dataset, given a set of criteria and required concept sets and values."?

pshapiro4broad · 2023-11-15T15:34:01Z

src/main/resources/api/data-repository-openapi.yaml

+          type: array
+          items:
+            oneOf:
+              - $ref: '#/components/schemas/SnapshotBuilderDomainOption'


I think you may need a new type for this. The Option versions of these types are like "prototype" or "source" versions. For a selected domain option, the only value needed is the option's concept code. The Domain root (which I think the UI calls category) may also be helpful to include for non-OMOP cases. For OMOP data the code is unique across all categories so it's sufficient by itself.

Note that a future version of the cohort builder will support other constraints on the codes, like occurrence count and occurrence times.

pshapiro4broad · 2023-11-15T15:34:22Z

src/main/resources/api/data-repository-openapi.yaml

+          items:
+            oneOf:
+              - $ref: '#/components/schemas/SnapshotBuilderDomainOption'
+              - $ref: '#/components/schemas/SnapshotBuilderProgramDataOption'


ProgramDataOption might work fine for both cases, even though it is representing a different thing. It may be that the same data is needed for both.

I could also do SnapshotBuilderConcept (which is the root of DomainOption) and ProgramDataOption, just to keep them separate.

pshapiro4broad · 2023-11-15T15:35:37Z

src/test/java/bio/terra/app/controller/DatasetsApiControllerTest.java

+  void testCreateSnapshotRequest() throws Exception {
+    mockValidators();
+    SnapshotBuilderAccessRequest expected =
+        new SnapshotBuilderAccessRequest()


Another approach here would be to write the object as JSON. If it's less code I would do it that way instead.

s-rubenstein · 2023-11-16T17:26:00Z

src/main/java/bio/terra/service/snapshotbuilder/SnapshotBuilderService.java

@@ -24,6 +25,12 @@ public SnapshotBuilderSettings updateSnapshotBuilderSettings(
    return snapshotBuilderSettingsDao.upsertSnapshotBuilderSettingsByDataset(id, settings);
  }

+  public SnapshotBuilderAccessRequest createSnapshotRequest(
+      // TODO: add given request to the database, return real JobModel with the id of the new entry.


Nit: This comment is no longer accurate wrt the JobModel. Also, could you reference the specific ticket number?

pshapiro4broad

Looks good, just a few minor comments left

pshapiro4broad · 2023-11-17T16:02:50Z

src/main/resources/api/data-repository-openapi.yaml

+        conceptSets:
+          type: array
+          items:
+            $ref: '#/components/schemas/SnapshotBuilderDatasetConceptSets'


I think this type should be called SnapshotBuilderDatasetConceptSet since it's a single concept set. It's an existing type but it'd be good to rename it now if it's not a lot of extra work.

Suggested change

$ref: '#/components/schemas/SnapshotBuilderDatasetConceptSets'

$ref: '#/components/schemas/SnapshotBuilderDatasetConceptSet'

pshapiro4broad · 2023-11-17T16:03:27Z

src/main/resources/api/data-repository-openapi.yaml

+      discriminator:
+        propertyName: kind
+        mapping:
+          list: '#/components/schemas/SnapshotBuilderProgramDataListCriteria'
+          range: '#/components/schemas/SnapshotBuilderProgramDataRangeCriteria'
+          domain: '#/components/schemas/SnapshotBuilderCriteria'


pshapiro4broad · 2023-11-17T16:06:06Z

src/test/java/bio/terra/app/controller/DatasetsApiControllerTest.java

@@ -239,6 +245,61 @@ void testUpdateSnapshotBuilderSettings() throws Exception {
    verify(snapshotBuilderService).updateSnapshotBuilderSettings(DATASET_ID, SETTINGS);
  }

+  @Test
+  void testCreateCriteriaData() throws Exception {
+    TestUtils.mapFromJson(


I realize the point here is that the json conversion doesn't throw, but I think a test should have at least one assert per conversion. Something like

var criteria = TestUtils.mapFromJson(...); assertThat(critera.name(), is("name"));

pshapiro4broad · 2023-11-17T16:07:32Z

src/test/java/bio/terra/app/controller/DatasetsApiControllerTest.java

+                    .addCohortsItem(
+                        new SnapshotBuilderCohort()
+                            .name("cohort")
+                            .addCriteriaGroupsItem(new SnapshotBuilderCriteriaGroup()))


Can you add at least one criteria to the group? It'd be good to have another check for round-trip operations on the oneOf objects.

sonarqubecloud · 2023-11-17T17:05:02Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

83.3% Coverage
0.0% Duplication

raejohanek added 3 commits November 9, 2023 15:51

Add stub service method, controller, and api

14bc405

Add test of requestSnapshot

3dcaf9b

Clarify comment

bf62a9b

rjohanek requested review from snf2ye, nmalfroy, samanehsan and okotsopoulos as code owners November 9, 2023 21:01

rjohanek and others added 4 commits November 9, 2023 16:04

Merge branch 'develop' into DC-781

fe5d534

Resolve error made in merge

3c7f31a

Resolve error made in merge

e96134e

gradle apply spotless

c32e418

rjohanek requested review from pshapiro4broad and s-rubenstein November 9, 2023 21:20

s-rubenstein reviewed Nov 9, 2023

View reviewed changes

src/main/java/bio/terra/service/snapshotbuilder/SnapshotBuilderService.java Outdated Show resolved Hide resolved

src/main/resources/api/data-repository-openapi.yaml Outdated Show resolved Hide resolved

src/main/resources/api/data-repository-openapi.yaml Outdated Show resolved Hide resolved

s-rubenstein reviewed Nov 9, 2023

View reviewed changes

src/test/java/bio/terra/app/controller/DatasetsApiControllerTest.java Outdated Show resolved Hide resolved

src/test/java/bio/terra/app/controller/DatasetsApiControllerTest.java Show resolved Hide resolved

raejohanek added 2 commits November 13, 2023 08:51

update naming to match convention

a6a4df6

update method to return given request

bb77602

rjohanek requested a review from s-rubenstein November 13, 2023 17:35

fix spotbug violation

ddd5222

pshapiro4broad requested changes Nov 14, 2023

View reviewed changes

raejohanek added 2 commits November 14, 2023 15:31

update naming and expand test

34d2aac

remove separate definition AnyCriteria, update test

37b36d5

rjohanek requested a review from pshapiro4broad November 14, 2023 22:09

pshapiro4broad reviewed Nov 15, 2023

View reviewed changes

raejohanek added 2 commits November 15, 2023 14:17

change SnapshotBuilderDomainOption to SnapshotBuilderConcept

4defd5c

Merge branch 'develop' into DC-781

8e3fa46

s-rubenstein approved these changes Nov 16, 2023

View reviewed changes

raejohanek added 2 commits November 16, 2023 15:33

update data types for criteria to use discriminator

2cd3d8d

update TODO comment

3e38ff5

pshapiro4broad approved these changes Nov 17, 2023

View reviewed changes

change name ConceptSets->ConceptSet and expand tests

3f63d4a

rjohanek merged commit d460c6b into develop Nov 17, 2023

rjohanek deleted the DC-781 branch November 17, 2023 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DC-781] Create service endpoint stub for creating a Snapshot Request #1537

[DC-781] Create service endpoint stub for creating a Snapshot Request #1537

rjohanek commented Nov 9, 2023

pshapiro4broad left a comment

pshapiro4broad Nov 14, 2023

rjohanek Nov 14, 2023 •

edited

Loading

rjohanek Nov 14, 2023

pshapiro4broad Nov 14, 2023

pshapiro4broad Nov 15, 2023

pshapiro4broad Nov 15, 2023

pshapiro4broad Nov 15, 2023

rjohanek Nov 15, 2023

pshapiro4broad Nov 15, 2023

s-rubenstein Nov 16, 2023

pshapiro4broad left a comment

pshapiro4broad Nov 17, 2023

pshapiro4broad Nov 17, 2023

pshapiro4broad Nov 17, 2023

pshapiro4broad Nov 17, 2023

sonarqubecloud bot commented Nov 17, 2023

	$ref: '#/components/schemas/SnapshotBuilderDatasetConceptSets'
	$ref: '#/components/schemas/SnapshotBuilderDatasetConceptSet'

[DC-781] Create service endpoint stub for creating a Snapshot Request #1537

[DC-781] Create service endpoint stub for creating a Snapshot Request #1537

Conversation

rjohanek commented Nov 9, 2023

pshapiro4broad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjohanek Nov 14, 2023 • edited Loading

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

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

sonarqubecloud bot commented Nov 17, 2023

rjohanek Nov 14, 2023 •

edited

Loading