06620d160679a0d2e77dedbe35b35edeea08c517
chmalee
  Mon Jun 3 16:17:48 2019 -0700
Add checks to hubCheck and hgTrackUi for incorrect tag=val pairs in a subgroup line, refs #13428

diff --git src/hg/utils/hubCheck/hubCheck.c src/hg/utils/hubCheck/hubCheck.c
index 774dd6d..cc916fc 100644
--- src/hg/utils/hubCheck/hubCheck.c
+++ src/hg/utils/hubCheck/hubCheck.c
@@ -443,59 +443,90 @@
     {
     /*  check level */
     struct trackHubSettingSpec *checkLevel = NULL;
     AllocVar(checkLevel);
     checkLevel->level = options->level;
     if (trackHubSettingLevel(hubSetting) < trackHubSettingLevel(checkLevel))
         {
         dyStringPrintf(errors, "Setting '%s' is level '%s'\n", setting, hubSetting->level);
         retVal = 1;
         }
     freez(&checkLevel);
     }
 return retVal;
 }
 
-void hubCheckCompositeSettings(struct trackHub *hub, struct trackHubGenome *genome, struct trackDb *tdb, struct dyString *errors)
-/* Check composite level settings like subgroups, dimensions, sortOrder, etc */
+int hubCheckCompositeSettings(struct trackHub *hub, struct trackHubGenome *genome, struct trackDb *tdb, struct dyString *errors)
+/* Check composite level settings like subgroups, dimensions, sortOrder, etc.
+ * Note that this function returns an int because we want to warn about all errors in a single
+ * composite stanza rather than errAbort on the first error */
 {
+int retVal = 0;
 if (!tdbIsComposite(tdb))
-    return;
+    return retVal;
 
 sortOrder_t *sortOrder = NULL;
 membership_t *membership = NULL;
 struct slRef *subtrackRef, *subtrackRefList = NULL;
 
 // check that if a sortOrder is defined, then subtracks exist in the subgroup
+struct errCatch *errCatch = errCatchNew();
+if (errCatchStart(errCatch))
+    {
+    (void)membersForAllSubGroupsGet(tdb, NULL);
+    }
+errCatchEnd(errCatch);
+if (errCatch->gotWarning || errCatch->gotError)
+    {
+    // don't add a new line because one will already be inserted by the errCatch->message
+    dyStringPrintf(errors, "track %s error: %s", tdb->shortLabel, errCatch->message->string);
+    retVal = 1;
+    }
+
+if (errCatchStart(errCatch))
+    {
     sortOrder = sortOrderGet(NULL, tdb);
     if (sortOrder)
         {
         subtrackRefList = trackDbListGetRefsToDescendantLeaves(tdb->subtracks);
         for (subtrackRef = subtrackRefList; subtrackRef != NULL; subtrackRef = subtrackRef->next)
             {
             struct trackDb *subtrack = subtrackRef->val;
             membership = subgroupMembershipGet(subtrack);
             int i;
             for (i = 0; i < sortOrder->count; i++)
                 {
                 char *col = sortOrder->column[i];
-            if (membership == NULL || stringArrayIx(col, membership->subgroups, membership->count) == -1)
-                dyStringPrintf(errors,
-                    "sortOrder %s defined for all subtracks of the composite track \"%s\", but the track \"%s\" is not a member of this subGroup\n", col, tdb->shortLabel, subtrack->shortLabel);
+                if ( (!sameString(col, SUBTRACK_COLOR_SUBGROUP)) && (membership == NULL || stringArrayIx(col, membership->subgroups, membership->count) == -1))
+                    {
+                    dyStringPrintf(errors, "sortOrder %s defined for all subtracks of the composite track \"%s\", but the track \"%s\" is not a member of this subGroup\n",
+                        col, tdb->shortLabel, subtrack->shortLabel);
+                    retVal = 1;
+                    }
+                }
+            }
         }
     }
+errCatchEnd(errCatch);
+if (errCatch->gotWarning || errCatch->gotError)
+    {
+    // don't add a new line because one will already be inserted by the errCatch->message
+    dyStringPrintf(errors, "track %s error: %s", tdb->shortLabel, errCatch->message->string);
+    retVal = 1;
     }
+errCatchFree(&errCatch);
+return retVal;
 }
 
 void hubCheckParentsAndChildren(struct trackDb *tdb)
 /* Check that a single trackDb stanza has the correct parent and subtrack pointers */
 {
 if (tdbIsSuper(tdb) || tdbIsComposite(tdb) || tdbIsCompositeView(tdb) || tdbIsContainer(tdb))
     {
     if (tdb->subtracks == NULL)
         {
         errAbort("Track \"%s\" is declared superTrack, compositeTrack, view or "
             "container, but has no subtracks", tdb->track);
         }
 
     // Containers should not have a bigDataUrl setting
     if (trackDbLocalSetting(tdb, "bigDataUrl"))
@@ -534,41 +565,44 @@
     struct slPair *metaPairs = trackDbMetaPairs(tdb);
 
     if (metaPairs != NULL)
         {
         printf("%s\n", trackHubSkipHubName(tdb->track));
         struct slPair *pair;
         for(pair = metaPairs; pair; pair = pair->next)
             {
             printf("\t%s : %s\n", pair->name, (char *)pair->val);
             }
         printf("\n");
         }
     slPairFreeValsAndList(&metaPairs);
     }
 
-if (!options->checkFiles)
-    return retVal;
 
 struct errCatch *errCatch = errCatchNew();
 if (errCatchStart(errCatch))
     {
     hubCheckParentsAndChildren(tdb);
     if (tdbIsComposite(tdb))
-        hubCheckCompositeSettings(hub, genome, tdb, errors);
+        {
+        retVal |= hubCheckCompositeSettings(hub, genome, tdb, errors);
+        }
+    if (options->checkFiles)
+        {
         hubCheckBigDataUrl(hub, genome, tdb);
         }
+    }
 errCatchEnd(errCatch);
 if (errCatch->gotError)
     {
     retVal = 1;
     dyStringPrintf(errors, "%s", errCatch->message->string);
     }
 errCatchFree(&errCatch);
 
 if (tdb->subtracks != NULL)
     {
     retVal |= hubCheckTrack(hub, genome, tdb->subtracks,  options, errors);
     }
 
 return retVal;
 }