Implemented Concept Set editor for Feature Analysis and CC

[wivern]

Resolves OHDSI/Atlas#1595

[chrisknoll]

This looks very strange to have the FeAnalysisService extend an AbstractVocabularyService. We'll defintely have certain services needing to pull in functions from different more-atomoic support services, so woudldn't encapsulation vs. inheritance make more sense here to support the shared functionality?

[chrisknoll]

I merged master into this PR and found some conflicts. Please review and let me know if something was missed.

It's still an outstanding question: having CohrotDefinitionService extend AbstractVocabularyService is strange: Vocabulary service and CohortDefinition services are 2 different things, even tho one has a dependency on the other. Spring lets you easily plug in (via @Autowire) those dependencies, so if there is some Vocabulary function that is shared across vocabulary service and cohortDefinition, put it in one and autowire to the other (it sounds like the ownership of the vocabulary function belongs to VocabularyService, so why not just define the function there, and create a reference to it in CohortDefinitionService?

[chrisknoll]

Pushed a commit to fix a SqlSerer migration script: can't use SELECT NEXT VALUE FOR() in a UNION ALL statement.

[chrisknoll]

Import CC is failing. I had a CC with several cohort defs and a few strata critera defined and when I import the design into this branch, the strataConceptSets are not imported, and the inner criteria do not have their criteria assigned. Attached is the example. Note that when you import, none of the strata have the correct measurement concept set assigned.

ccFailedStrata.txt

[chrisknoll]

I'm debugging this and found some items that are confusing, but want to make sure I am not missing something. also, If you are actively working on this, please indicate this in this issue so that we don't collide with each other:

When performing the import, the concept sets are not being assigned to the newly-imported cohort characterization design via setConceptSetEntity(). I was starting to make a simple change (I thought) to assign the concept set, but realized that it isn't a single concept set that is in the design, but a list..so, the name of this field was confusing. But, looking at what was persisted in the object, and how the getConceptSets() is defiend in CommonConceptSetEntity:

  public List<ConceptSet> getConceptSets() {
    return Objects.nonNull(this.rawExpression) ?
            Utils.deserialize(this.rawExpression, typeFactory -> typeFactory.constructCollectionType(List.class, ConceptSet.class)) : null;
  }

It does interpret the expression as an array, so the only confusion here is the name of the entity (the entity is not a single concept set). We can't change this name since it's a public class that may break an API, but this is just good to know.

However, looking elsewhere in the code for places where cocnept sets are being referneced in the CC, I found updateConceptSet():

  private void updateConceptSet(CohortCharacterizationEntity entity, CohortCharacterizationEntity foundEntity) {
      if (Objects.nonNull(foundEntity.getConceptSetEntity()) && Objects.nonNull(entity.getConceptSetEntity())) {
        foundEntity.getConceptSetEntity().setRawExpression(entity.getConceptSetEntity().getRawExpression());
      } else if (Objects.nonNull(entity.getConceptSetEntity())) {
        CcStrataConceptSetEntity savedEntity = conceptSetRepository.save(entity.getConceptSetEntity());
        foundEntity.setConceptSetEntity(savedEntity);
      }
  }

This is quite confusing: it looks like if the 'updated' concept set has a CCStrataConceptSetEntity, but the existing one does not, it is going to try to create it via conceptSetRepository.save() (?!?). The concept set repository is single-concept-set based, not list-based like is stored in the raw expression of the CcStrataConceptSetEntity. This makes me want to remove all the code realted to the concept set repository, but before I do, I wanted to check in here and see if you already are aware of this.

From my perspective, the followign needs to be added to importCc:

        // add concept sets
        CcStrataConceptSetEntity ccConceptSets = new CcStrataConceptSetEntity();
        ccConceptSets.setCohortCharacterization(persistedCohortCharacterization);
        ccConceptSets.setRawExpression(entity.getConceptSetEntity().getRawExpression());
        persistedCohortCharacterization.setConceptSetEntity(ccConceptSets);

But that still leaves the code in updateConceptSet, which I think is wrong. I think the branch of code that handles the case where the concept set repository is called is a dead-branch because when you serialize out the expression, you always get at least an empty array, so the object is never non-null.

Please share your thoughts. Thank you.

[chrisknoll]

Ok, I found my source of confusion:

The conceptSetRepsitory reference in CC is not an actual 'ConceptSetRepository' that we see in the context of the vocabulary service, but rather it's a separate concpetSetRepository of type 'CcConceptSetRepository' which simply saves a list of concept sets as a JSON form to the cc_strata_concept_set table. So, it's totally fine that this repository exists, however, I do question the need for it: when you save a CC design to the server, the nature of associating the CohortCharacterizationEntity to the CCConceptSet means that savign the 'parent' will save the 'child entitties'. Unless we want to mechanism to save the concept sets directly (in absence of saving a cohort characerization', I don't quite feel this separate repository interface is necessary (will you ever have a CCConceptSet independent of a cohort characterization?

In any case, this code can live on since it isn't as breaking as I thought it was...and I 'll proceed with making the fixes around the 'import' action losing the concept sets,

[chrisknoll]

I've resolved the import CC issue.

[chrisknoll]

I marked this for the June 15 sprint: I think this PR is complete, since I do not see any conflict between atlas-master and this PR (I tested importing a CC design into the master branch on atlas).

I think we're good to go.

Found new issues.

[chrisknoll]

Sorry, I need to make a correction: this change does not work with a the 2.8 (master) atlas UI client, but it does work with the 2.7.6 release...so, I believe we are OK to merge this PR as it will work with 2.7.6 and the open PR for atlas OHDSI/Atlas#2132 will resolve the issue in master once that is completed.

When I thought that this was an issue with 2.7 vs. 2.8, I dismissed my review, but now that I can confirm that 2.7 branch does work with this PR, I can re-approve this.

[chrisknoll]

@wivern, I merged master into this branch to get it ready for the OHDSI/Atlas#2132 (which this branch reflrect the issue that is being fixed in that PR, so I'm getting both sides (webapi/atlas) ready.

I'm experiencing a problem when launching the branch after merge:

Caused by: org.springframework.beans.factory.NoSuchBeanDefinitionException: No qualifying bean of type 'org.ohdsi.webapi.shiro.management.AtlasSecurity' available: expected at least 1 bean which qualifies as autowire candidate. Dependency annotations: {}

I think @anthonysena mentioned that there was an issue related to disabled security on one of the other PRs, but I'm surprised to have this issue based off of a merge from master.

Can you please pull this branch and check if you're getting the same error when security is disabled, and let me know if you know what might be happening here?

Note: I just checked out master branch, and I am getting the same error. So, we're currently in a state where you can't launch master branch when security is disabled, and that's a very big problem.

[wivern]

@chrisknoll There was a mistake in the master branch. Now it's fixed with #1599
Thank you for reporting this.

[chrisknoll]

@wivern,
The Atlas part was merged to master, but this PR remains outstanding, and I'm working to merge master into this branch so I can see it all working. Strange thing tho: what is in this PR that is required by the Atlas changes? I think I've seen the Atlas UI work with the latest master WebAPI, but I could have missed specific functionality in my testing.

Could you update the description of this PR to include a brief bullet list that describes with the intended changes are here? It's a bit hard to tell based on the various merges (and this PR has been open for a long time now).