Cohort Definition Validation Feature API #1929

charhart · 2021-08-22T02:58:46Z

This PR complements this Atlas pull request. It includes the needed tables and API calls to create validation studies. This feature corresponds to this ticket.

@BSCrumpton and @jduke99 can answer questions and provide additional details about this pull request.

…ntity. Also added additional param searches

… references to QuestionSetId

… super table query

…er table

…tudy now

…e param is passed

…left

…t query

…manual query

…nd fields

chrisknoll · 2021-08-23T15:46:00Z

no need to worry about that test error, it seems to be something related to the OS used to run the unit tests on the CI env, and temporary directories. I'd like that fixed, but you can ignore it for purposes of this PR.

chrisknoll · 2021-08-23T21:12:15Z

src/main/resources/ddl/results/annotation_result.sql

@@ -0,0 +1,11 @@
+IF OBJECT_ID('@results_schema.annotation_result', 'U') IS NULL
+CREATE TABLE @results_schema.annotation_result(
+    result_id integer NOT NULL AUTO_INCREMENT,


I don't believe this is appropriate. Very few MMP systems (redshift, pdw, impala) support an AUTO_INCREMENT field type.

For CDM results type of data, you just have to provide your own row id (or multi-column row id) that is sent to the server via insert into....

@chrisknoll I believe my last push (aa9f441) should have fixed this. We're testing now!

…id from results table as it isn't necessary

chrisknoll · 2021-09-28T12:42:41Z

2 open issues that we need to investigate are:

Security
Internationalization

@anthonysena and @chrisknoll will look at adding the elements to secure the endpoints, and provide a PR or branch that will add the changes to this PR branch (so not to interrupt anything @cahilton is woring on here), and after that we will look at the changes required for internationalization.

chrisknoll · 2021-09-29T16:23:48Z

@cahilton could you merge your upsstream branch from the latest commits from master and then merge that into your PR branch? I want to ensure we're working off a PR that is based off of the latest commits to master.

chrisknoll · 2021-10-20T18:28:51Z

Actually, I take it back, I think since commits were already merged into your master's branch on your fork, the git history is already set on your master's history, so there's no real easy way to adjust that. In the future, please do new work on a feature branch and make your PRs based off that branch instead of making changes directly into master. When dealing with multiple forks from different contributors, if everyone pushes their own chagnes into their own master branch, it gets really difficult to synchronize it.

chrisknoll · 2021-10-21T13:18:40Z

I think I've identified the following endpoints (and HTTP verbs associated):

GET:annotations/
GET:annotations/{id}
GET:annotations/fullquestion
POST:annotations/
POST:annotations/sample
POST:annotation/answers
GET:annotations/navigation
GET:annotations/questions
POST:annotations/questions
GET:annotations/results/
GET:annotations/results/{id}
GET:annotations/results/completeResults
GET:annotations/sets
POST:annotations/sets
GET:annotations/getsets
GET:annotations/deleteSet/{questionSetId}

I can create the migration script that registers these to shiro, but wanted to ask about one of them:

POST:annotation/answers

In this one case, the path is singular annotation instead of plural annotations of the others. I think this could be confusing. Can the endpoint in AnswerController be modified to specify annotations as the root? Atlas PR will probably need to be modified to handle this change.

Let me know because I need a final list of endpoint paths in order to apply the migration script.

@cahilton

…n2.0-cknoll

chrisknoll · 2021-11-08T14:16:12Z

Update: I've looked at other places of annotations, and most, if not all, are singular, so I'm updating the path to be annotation and so here is the list of endpoints:

GET:annotation/
GET:annotation/{id}
GET:annotation/fullquestion
POST:annotation/
POST:annotation/sample
POST:annotation/answers
GET:annotation/navigation
GET:annotation/questions
POST:annotation/questions
GET:annotation/results/
GET:annotation/results/{id}
GET:annotation/results/completeResults
GET:annotation/sets
POST:annotation/sets
GET:annotation/getsets
GET:annotation/deleteset/{questionSetId}

It would be helpful if the Atlas PR could be updated to reflect these changes. Also note, the case-change in annotation/deleteset.

chrisknoll · 2021-11-08T14:46:18Z

src/main/java/org/ohdsi/webapi/annotation/annotation/AnnotationController.java

+  @Consumes(MediaType.APPLICATION_JSON)
+  @Produces(MediaType.APPLICATION_JSON)
+  public void addResult(@RequestBody String payload) {
+    System.out.println(payload);


We don't usually parse raw JSON text from the POST. Instead, you define a class that represents the data structure, and the service will automatically serialize the request body into the JSON form.

In addition, we shouldn't leave 'System.out.println' in the sourcecode. Use a logger.

chrisknoll · 2021-11-08T14:54:03Z

src/main/java/org/ohdsi/webapi/annotation/annotation/AnnotationController.java

+          ) {
+    List<Annotation> returnAnnotations=null;
+    if (cohortSampleId != 0 && subjectId != 0 && setId != 0) {
+      System.out.println("made it into the search function");


Need to find and remove all System.out calls and move to logging or remove entirely.

chrisknoll · 2021-11-08T15:00:22Z

src/main/java/org/ohdsi/webapi/annotation/answer/AnswerController.java

+  @Path("answers")
+  @Consumes(MediaType.APPLICATION_JSON)
+  @Produces(MediaType.APPLICATION_JSON)
+  public void addQuestion(Answer answer) {


/answers maps over to a function addQuestion which calls answerService.addAnswer?

Renamed end point from 'annotations' to 'annotation'

Annotation2.0 cknoll

jduke99 and others added 30 commits February 1, 2021 12:04

Added Annotation Code

5464709

updating annotation code to test build.

6735945

New Annotation Tables

f1203ec

Updated Annotation

704b5a2

Update CohortSample.java

8e4fc91

annotion link button now works

de445db

removing results from Annotation.java

18319f8

trying to make saving results to annotation_results table work

b6d5b07

working annotation_results saving

b82ef09

initial commit for ResultController and ResultService

65028fc

can now query for all results of a certain annotation ID

7b02d10

added todo in AnnotationController

45ffb5f

default annotation GET now returns summary instead of full question e…

b2166a8

…ntity. Also added additional param searches

removed some printlines, also added todo about performance

7b7ce8e

moving to prepared statements for annotation_results

d769a68

working insertion and 'updating'. Tomorrow will change all QuestionID…

b002dfe

… references to QuestionSetId

working results by Question Set ID, albiet slow

348e67c

querying for results based on QuestionSetId now is much faster

1326f9a

adding in orderered params to all jdbcTemplate queries

791e3f9

removed all instances/mentions of long and changed to int. Also fixed…

ba72b44

… super table query

working getSets query

c695f24

creating annotation study table to aid in query speed for results sup…

1afa631

…er table

more work done on supertable- everything should be using annotation_s…

e5b1e2a

…tudy now

return plain Results from the getResults endpoint

92271b4

changing sampleName to cohortSampleId in AnnotationController

3329bde

changing sampleName to cohortSampleId in AnnotationController pt 2

b8b61f4

complete results now querying by sampleId instead of Study ID when th…

734b8b3

…e param is passed

finding samples and sets by cohort ID now uses inner join instead of …

47c6b31

…left

adding answer text in additon to answer value to the super table

37542bf

setting AnswerValue now

6bd6d66

Saul Crumpton added 8 commits August 22, 2021 11:48

changing AnnotationRepository to use base findById instead of explici…

d58f51c

…t query

updated AnswerRepository to use default findById method instead of query

9c558ec

updated QuestionRepository to use default findById method instead of …

cecc37a

…manual query

removing all chaff from Annotation Result package- unused fields etc

26eff09

updated QuestionSet to use default findById, removed unused imports a…

075af89

…nd fields

removing more unused imports from QuestionSet repository

cdca276

using sequence for annotation_id instead of generated value

530fa59

using generator for all IDs in Question, QuestionSet, Answer

2643f96

rename annotation sequences

45057a4

chrisknoll reviewed Aug 23, 2021

View reviewed changes

corrected sequence names for id generators of entities. Also removed …

aa9f441

…id from results table as it isn't necessary

Merge branch 'OHDSI:master' into annotation2.0-cknoll

41058f3

Merge remote-tracking branch 'remotes/upstream/master' into annotatio…

5a4905f

…n2.0-cknoll

chrisknoll reviewed Nov 8, 2021

View reviewed changes

chrisknoll requested changes Nov 8, 2021

View reviewed changes

chrisknoll reviewed Nov 8, 2021

View reviewed changes

chrisknoll and others added 2 commits November 10, 2021 13:31

Added security migration scripts.

8814282

Renamed end point from 'annotations' to 'annotation'

Merge pull request #1 from cahilton/annotation2.0-cknoll

37d1c60

Annotation2.0 cknoll

anthonysena added this to the V2.12 milestone Feb 15, 2022

chrisknoll self-assigned this Feb 21, 2022

chrisknoll modified the milestones: V2.12, 2.x Backlog Jul 27, 2022

chrisknoll marked this pull request as draft May 23, 2023 17:26

anthonysena removed this from the [LEGACY] 2.x Backlog milestone Jun 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cohort Definition Validation Feature API #1929

Cohort Definition Validation Feature API #1929

charhart commented Aug 22, 2021

chrisknoll commented Aug 23, 2021

chrisknoll Aug 23, 2021

BSCrumpton Aug 23, 2021

chrisknoll commented Sep 28, 2021

chrisknoll commented Sep 29, 2021

chrisknoll commented Oct 20, 2021

chrisknoll commented Oct 21, 2021 •

edited

Loading

chrisknoll commented Nov 8, 2021 •

edited

Loading

chrisknoll Nov 8, 2021

chrisknoll Nov 8, 2021

chrisknoll Nov 8, 2021

Cohort Definition Validation Feature API #1929

Are you sure you want to change the base?

Cohort Definition Validation Feature API #1929

Conversation

charhart commented Aug 22, 2021

chrisknoll commented Aug 23, 2021

chrisknoll Aug 23, 2021

Choose a reason for hiding this comment

BSCrumpton Aug 23, 2021

Choose a reason for hiding this comment

chrisknoll commented Sep 28, 2021

chrisknoll commented Sep 29, 2021

chrisknoll commented Oct 20, 2021

chrisknoll commented Oct 21, 2021 • edited Loading

chrisknoll commented Nov 8, 2021 • edited Loading

chrisknoll Nov 8, 2021

Choose a reason for hiding this comment

chrisknoll Nov 8, 2021

Choose a reason for hiding this comment

chrisknoll Nov 8, 2021

Choose a reason for hiding this comment

chrisknoll commented Oct 21, 2021 •

edited

Loading

chrisknoll commented Nov 8, 2021 •

edited

Loading