Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problem with postConceptSetDefinition #189

Closed
siirtola opened this issue May 26, 2021 · 9 comments
Closed

Problem with postConceptSetDefinition #189

siirtola opened this issue May 26, 2021 · 9 comments
Assignees
Labels
bug Something isn't working

Comments

@siirtola
Copy link

I might have misunderstood something, but shouldn't the getConceptSetDefinition and postConceptSetDefinition be symmetric? Below, I get a concept set from Atlas ("asthma", just the standard asthma definition) and try to post it under a new name. Atlas responds "Bad Request (HTTP 400)".

# ATLAS Version 2.7.8 Release 
# WebAPI Version 2.7.8 Release 
# 
# Both client and server have the same java version:
# openjdk version "1.8.0_292"
# OpenJDK Runtime Environment (AdoptOpenJDK)(build 1.8.0_292-b10)
# OpenJDK 64-Bit Server VM (AdoptOpenJDK)(build 25.292-b10, mixed mode)

asthma_id <- 30 # standard "asthma" concept
conceptSetDefinition <- ROhdsiWebApi::getConceptSetDefinition(conceptSetId = asthma_id, baseUrl = "http://localhost/WebAPI")

conceptSetDefinition

postDefinition <- ROhdsiWebApi::postConceptSetDefinition(name = "asthma_test", 
                                                         baseUrl = "http://localhost/WebAPI", 
                                                         conceptSetDefinition = conceptSetDefinition)

# Error in .request(url, method = "PUT", config = config, handle = handle,  : 
#                     Bad Request (HTTP 400).

@azimov azimov self-assigned this May 26, 2021
@azimov
Copy link
Collaborator

azimov commented May 28, 2021

@siirtola so the request to post a concept set defintion and get are different. This is because the id and name aren't set as part of the expression (the id will be different because

If you, instead, posted the expression it should work, e.g:

ROhdsiWebApi::postConceptSetDefinition(name = "asthma_test", 
                                                         baseUrl = "http://localhost/WebAPI", 
                                                         conceptSetDefinition = conceptSetDefinition$expression)

You would post a new definition and get a new id.

@siirtola
Copy link
Author

Hi @azimov, thank you for looking into this! I tried posting just the expression, but the outcome was the same, "HTTP 400".

Do you have any suggestions how to debug this?

@siirtola
Copy link
Author

siirtola commented May 28, 2021

Looking from the Atlas end, the new concept set is created, but the adding of concepts to it fails.

@siirtola
Copy link
Author

Hi @azimov, I am still puzzled with this. Would it possible to test this on atlas-demo.ohdsi.org? Are the credentials available somewhere?

@azimov
Copy link
Collaborator

azimov commented Jun 15, 2021

@siirtola I'm sorry I didn't get back to you, I can confirm the issue but I'm trying to determine the circumstances and what else it might be impacting.

@azimov azimov added the bug Something isn't working label Jun 15, 2021
@siirtola
Copy link
Author

@azimov thanks for the confirmation! This was driving me crazy.

@alondhe
Copy link
Collaborator

alondhe commented Jun 17, 2021

Facing the same issue in Atlas/WebAPI 2.8.1.

@alondhe
Copy link
Collaborator

alondhe commented Jun 17, 2021

Looks like the issue is here: https://github.com/OHDSI/ROhdsiWebApi/blob/master/R/PostDefinition.R#L109

This payload is nested one level too deep for the PUT endpoint of "{id}/items" (handled by function saveConceptSetItems). I tried changing the call to use itemsTranspose[[1]] instead, and it worked. Making a PR.

alondhe added a commit that referenced this issue Jun 17, 2021
…to incorrect JSON structure for PUT endpoint of "{id}/items" (handled by function saveConceptSetItems).
@azimov
Copy link
Collaborator

azimov commented Jun 23, 2021

@alondhe thanks for the fix! Looks like it works following the more recent patch. We should try and make a PR next time, even its small its good to have extra eyes review the changes.

@azimov azimov closed this as completed Jun 23, 2021
gowthamrao added a commit that referenced this issue Oct 12, 2021
* REmoved last updated from readme

#185

* Fix build badge in README

Closes #184

* GetDefinition now parses datatime into proper format

* GetDefinition now parses datatime into proper format (missed news)

* Resolves issue #189, where posting concept set definitions fails due to incorrect JSON structure for PUT endpoint of "{id}/items" (handled by function saveConceptSetItems).

* Switching to tempEmulationSchema (use of oracleTempSchema will throw deprecation warning) (#190)

Co-authored-by: Schuemie <MSCHUEMI@its.jnj.com>

* Original commit was incorrect; this allows multiple concepts to be PUT as items to {id}/items.

* Changing environmental variables

* Testing unsecured webapi endpoint

* Update test-webapi.R

* Add Helper tests

* change yml file for github actions and update setup.R

* Add test for InvokeGeneration. Fix documentation.

* ConceptSet specific tests

* Add PersonProfile tests, bug fixes and remove auth tests

* Remove the .isValidUrl function from the package and just use .checkBaseUrl as the main url checking function.

* restructure setup file, test git actions

* removetemp directory setup

* Add unit test commands to extras

* Add some error checking to getWebApiVersion

* Fix invoke generation test so that it handles race conditions (cohort already being generated)

* example export of environmental variable for gh actions

* Fix argument check in getWebApiVersion

* github actions test

* dont set baseUrl in test-invokeGeneration.R

* fix failing invoke generation test. again.

* restore all runners in github workflows

* Remove trailing backslash from base URL

#187

* Remove path, also replace write_csv with write_excel_csv

#194

* Intentionally duplicating code, sot that there is no dependency

#193

* add withr to Suggests in the Description file.

* Update Private.R

* uncommenting tests

* uncommenting tests

* Change coverage badge to reflect testathon branch

* Update README.md

* Executed autogenerated scripts - trailing backslash

#187

* tests for cohort def in package and cancel generation error

* unit tests for cancel, existname, detectname, and insertIntoPackage

* Added simple tests for CohortDefinition.R using mock services.

* Add httptest package to Suggests for testing.

* Remove environment variable check from github actions.

* Depricate insertCohortDefinitionSetInPackage. Move to OhdsiRTools package.

* consolidate get post and delete definition functions

* consolidate generation functions into one file.

* Remove the webapi simulation

* Depricate functions that insert data into study packages

* remove unnecessary dependencies

* update tests for WebApi.R

* Update tests folder

* Add newline at end of .gitignore file

* update docs
* Restore test-Helper.R
* update test-CohortDefinition.R
* update test-CohortDefinition.R
* temporarily limit github actions to two runners
* Create internal package dataframe StandardCategories
* restore all git actions runners
* remove with_mock_dir example
* Add tests
* remove old tests
* update mocks
* clean up test setup.R file
* Adding temporary folder with functions that need to be moved to other packages
* Comment out code related to posting characterizations which is not yet supported
* Add tests related to the study package functions in ROhdsiWebApi
* Remove the 'FunctionsToMove' folder which contained study package functions (Insert __ in package). Restore these functions in StudyPackage.R

* Add readme for tests
* Remove the files for running tests against different Auth mechanisms. Only database authentication can currently be tested.

* Move the scratch space setup to the one test file that requires it.
* Remove tests for depricated study package functions that are no longer depricated.
* Use the partially applied (auto-generated) invokeGeneration functions in test-Generation.R just to get coverage on those functions.
* When testing getResults make sure to use characterizations or pathways that only have one generation. Currently getResults will get all generations for characterizations and pathways.
* update mocks (cached webapi data)

* remove some scratch test code that was integrated into tests/
* add meta definitions tests and Cohort Definition error tests
* unit tests for no vocab surce key
* reset mocks for tests
* add tests for concept set workbook
* fix StudyPackage.R file tests and update cache/mocks
* Update code coverage badge to point to master branch (post-testathon)
* debug github actions
* Remove Insert In Package functions from Deprecated.R
* Comment out checks in insertCohortDefinitionSetInPackage that make it impossible to test
* Update argument names in readr::write_csv
* Use readr for file read and write in insertInPackage functions.
* update mocks (cache)
* restore all github actions runners. Remove github actions debug code.
* Fix test-StudyPackage.R
* Replace readChar with readr::read_file because readChar is causing errors on Ubuntu.
* Remove test that runs .insertSqlForCohortTableInPackage. Issue with running this function on Ubuntu.
* Remove testing of insert cohort creation R script.
* update test setup file with new characterization id.
* Add characterization generation function tests

Co-authored-by: Jamie Gilbert <JGilber2@its.jnj.com>
Co-authored-by: alondhe <alondhe@amgen.com>
Co-authored-by: Martijn Schuemie <schuemie@ohdsi.org>
Co-authored-by: Schuemie <MSCHUEMI@its.jnj.com>
Co-authored-by: Anthony Sena <asena5@its.jnj.com>
Co-authored-by: Adam Black <ablack3@users.noreply.github.com>
Co-authored-by: martin <lavalleem@mymail.vcu.edu>
Co-authored-by: Adam Black <adam.black@odysseusinc.com>
Co-authored-by: Anthony Sena <anthonysena@users.noreply.github.com>
Co-authored-by: mdlavallee92 <44976592+mdlavallee92@users.noreply.github.com>
Co-authored-by: Marc Suchard <msuchard@ucla.edu>
Co-authored-by: Chris Knoll <cknoll@ohdsi.org>
gowthamrao added a commit that referenced this issue Nov 6, 2021
Release by @azimov 
Squashing to ensure clean commit history - because of inadvert merge from test a thon branch

Enhancements:
* Remove trailing backslash from base URL
* Update cohort and concept set definitions (#231)

Bug fixes or optimization:
* GetDefinition now parses datatime into proper format
* Resolves issue #189, where posting concept set definitions fails due to incorrect JSON structure for PUT endpoint of "{id}/items" (handled by function saveConceptSetItems).
* Switching to tempEmulationSchema (use of oracleTempSchema will throw deprecation warning) (#190)
Co-authored-by: Schuemie <MSCHUEMI@its.jnj.com>
* Fix argument check in getWebApiVersion
* Add some error checking to getWebApiVersion
* add withr to Suggests in the Description file.
* Add httptest package to Suggests for testing.
* remove unnecessary dependencies
* Use readr for file read and write in insertInPackage functions.


GitHub actions and testing during HADES Testathon 2021: Lead by @ablack3 
* Tests for  unsecured webapi endpoint
* change yml file for github actions and update setup.R
* Add test for InvokeGeneration. Fix documentation.
* ConceptSet specific tests
* Add PersonProfile tests, bug fixes and remove auth tests
* Remove the .isValidUrl function from the package and just use .checkBaseUrl as the main url checking function.
* restructure setup file, test git actions
* Add unit test commands to extras
* Fix invoke generation test so that it handles race conditions (cohort already being generated)
* example export of environmental variable for gh actions
#187
* Remove path, also replace write_csv with write_excel_csv
#194
#193
#187

* tests for cohort def in package and cancel generation error
* unit tests for cancel, existname, detectname, and insertIntoPackage
* Added simple tests for CohortDefinition.R using mock services.
* Remove environment variable check from github actions.
* Depricate insertCohortDefinitionSetInPackage. Move to OhdsiRTools package.
* consolidate get post and delete definition functions
* consolidate generation functions into one file.
* Remove the webapi simulation
* Depricate functions that insert data into study packages
* update tests for WebApi.R
* Updated github ci build args to remove manual


Co-authored-by: Gowtham Rao <gowthamrao@gmail.com>
Co-authored-by: Gowtham Rao <rao@ohdsi.org>
Co-authored-by: alondhe <alondhe@amgen.com>
Co-authored-by: Martijn Schuemie <schuemie@ohdsi.org>
Co-authored-by: Schuemie <MSCHUEMI@its.jnj.com>
Co-authored-by: Anthony Sena <asena5@its.jnj.com>
Co-authored-by: Adam Black <ablack3@users.noreply.github.com>
Co-authored-by: martin <lavalleem@mymail.vcu.edu>
Co-authored-by: Adam Black <adam.black@odysseusinc.com>
Co-authored-by: Anthony Sena <anthonysena@users.noreply.github.com>
Co-authored-by: mdlavallee92 <44976592+mdlavallee92@users.noreply.github.com>
Co-authored-by: Marc Suchard <msuchard@ucla.edu>
Co-authored-by: Chris Knoll <cknoll@ohdsi.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants