Merge pull request #70 from uc-cdis/fix_cohort_listing

Fix cohort listing and improve existing warning on startup

Author: pieterlukasse

models/cohortdata.go

@@ -169,8 +169,8 @@ func (h CohortData) ValidateObservationData(observationConceptIdsToCheck []int64
 			log.Printf("INFO: no issues found in observation table of data source %d.", source.SourceId)
 		} else {
 			log.Printf("WARNING: !!! found a total of %d `person` records with duplicated `observation` entries for one or more concepts "+
-				"where this is not expected (in data source=%d). These are the entries found: %v !!!",
-				len(personConceptAndCount), source.SourceId, personConceptAndCount)
+				"where this is not expected (in data source=%d).",
+				len(personConceptAndCount), source.SourceId)
 			countIssues += len(personConceptAndCount)
 		}
 	}

models/cohortdefinition.go

@@ -3,6 +3,8 @@ package models
 import (
 	"fmt"
 
+	"log"
+
 	"github.com/uc-cdis/cohort-middleware/db"
 	"github.com/uc-cdis/cohort-middleware/utils"
 )
@@ -80,11 +82,20 @@ func (h CohortDefinition) GetAllCohortDefinitionsAndStatsOrderBySizeDesc(sourceI
 	meta_result := query.Scan(&cohortDefinitionStats)
 
 	// add name details:
+	finalList := []*CohortDefinitionStats{}
 	for _, cohortDefinitionStat := range cohortDefinitionStats {
 		var cohortDefinition, _ = h.GetCohortDefinitionById(cohortDefinitionStat.Id)
-		cohortDefinitionStat.Name = cohortDefinition.Name
+		if cohortDefinition == nil {
+			// unexpected issue: cohortDefinition not found. Warn and skip:
+			log.Printf("WARNING: found a cohort of size %d with missing cohort_definition record",
+				cohortDefinitionStat.CohortSize)
+			continue
+		} else {
+			cohortDefinitionStat.Name = cohortDefinition.Name
+			finalList = append(finalList, cohortDefinitionStat)
+		}
 	}
-	return cohortDefinitionStats, meta_result.Error
+	return finalList, meta_result.Error
 }
 
 func (h CohortDefinition) GetCohortName(cohortId int) (string, error) {

tests/models_tests/models_test.go

@@ -1,6 +1,7 @@
 package models_tests
 
 import (
+	"fmt"
 	"log"
 	"os"
 	"strings"
@@ -609,6 +610,34 @@ func TestGetAllCohortDefinitionsAndStatsOrderBySizeDesc(t *testing.T) {
 	}
 }
 
+// Tests whether the code deals correctly with the (error) situation where
+// the `cohort_definition` and `cohort` tables are not in sync (more specifically
+// the situation where a cohort still exists in `cohort` table but not in `cohort_definition`).
+func TestGetAllCohortDefinitionsAndStatsOrderBySizeDescWhenCohortDefinitionIsMissing(t *testing.T) {
+	setUp(t)
+	cohortDefinitions, _ := cohortDefinitionModel.GetAllCohortDefinitionsAndStatsOrderBySizeDesc(testSourceId)
+	if len(cohortDefinitions) != len(allCohortDefinitions) {
+		t.Errorf("Found %d", len(cohortDefinitions))
+	}
+
+	// remove one cohort_definition record and verify that the list is now indeed smaller:
+	firstCohort := cohortDefinitions[0]
+	tests.ExecAtlasSQLString(fmt.Sprintf("delete from %s.cohort_definition where id = %d",
+		db.GetAtlasDB().Schema, firstCohort.Id))
+	cohortDefinitions, _ = cohortDefinitionModel.GetAllCohortDefinitionsAndStatsOrderBySizeDesc(testSourceId)
+	if len(cohortDefinitions) != len(allCohortDefinitions)-1 {
+		t.Errorf("Number of cohor_definition records expected to be %d, found %d",
+			len(allCohortDefinitions)-1, len(cohortDefinitions))
+	}
+	// restore:
+	tests.ExecAtlasSQLString(fmt.Sprintf("insert into %s.cohort_definition (id,name,description) "+
+		"values (%d, '%s', '%s')",
+		db.GetAtlasDB().Schema,
+		firstCohort.Id,
+		firstCohort.Name,
+		firstCohort.Name))
+}
+
 func TestGetCohortName(t *testing.T) {
 	setUp(t)
 	allCohortDefinitions, _ := cohortDefinitionModel.GetAllCohortDefinitions()

tests/testutils.go

@@ -55,7 +55,7 @@ func ExecSQLScript(sqlFilePath string, sourceId int) {
 }
 
 func ExecAtlasSQLString(sqlString string) {
-	ExecSQLScript(sqlString, -1)
+	ExecSQLString(sqlString, -1)
 }
 
 func ExecSQLString(sqlString string, sourceId int) {