efc3f584c95755330510d53c767f103d222e20d1 braney Sat Aug 8 13:21:26 2026 -0700 cartTrackVarCatalog: fix the autoScale values, check value lists, refs #37838 The autoScale entry had the wrong vocabulary. The cart holds one of three long literals from wiggleScaleOptionsParent (hui.c:1973). The catalog listed on/off/group, which are the trackDb spellings; wiggleCart.c:454-459 translates those into the literals and never stores them. It also listed "cumulative", which is the C enum identifier wiggleScaleCumulative (hui.h:576), not a value in either vocabulary. This is worth more than a documentation fix. wiggleScaleStringToEnum() errAborts on an unknown string, so autoScale=on in a cart crashes hgTracks instead of being ignored. Any writer generated from this file would have produced exactly that. There were two copies of the bug. The lolly type carried its own autoScale entry with on/off, but lollyTrack.c:485 calls the same wigFetchAutoScaleWithCart(), so it shares the vocabulary and the errAbort. Those two are the only readers of that function, so autoScale is now complete. smoothingWindow listed "..." as if the ellipsis were a value. Enumerated. The other eight wig enums were checked against their C arrays and were all working: stringArrayIx() compares with differentWord(), which ignores case, so the on/ON and Bar/bar mismatches did no harm. Their values now match the arrays exactly, and each entry names the array it was checked against. Two new fields on an entry, both needed by the JSON cart schema: aliases other spellings a writer may use, mapped to the cart value, so JSON can accept autoScale "on" and store the literal valuesSrc the C array the values were verified against --check grows check_values(), which fails on a placeholder in a values list, an alias on an entry with no values, an alias pointing at a non-value, and an alias that is also a value. It also reports the backlog: 39 enums whose value lists have not been checked against the array that gates them. Each one can crash a CGI the same way autoScale could. diff --git src/hg/utils/cartTrackVarCatalog/cartTrackVarCatalog.py src/hg/utils/cartTrackVarCatalog/cartTrackVarCatalog.py index b16e70ff535..085d6972d88 100755 --- src/hg/utils/cartTrackVarCatalog/cartTrackVarCatalog.py +++ src/hg/utils/cartTrackVarCatalog/cartTrackVarCatalog.py @@ -91,43 +91,53 @@ {"prefix": "dup__", "what": "duplicated track", "src": "hg/inc/dupTrack.h:DUP_TRACK_PREFIX"}, ], "valueEncoding": "Everything is a string in the cart hash. The 'type' column below is " "how the reader interprets it, and is what the JSON form could encode " "natively. 'list' vars are multi-valued: the same name appears more " "than once in the var=val encoding and must become a JSON array.", } # --------------------------------------------------------------------------- # helpers # --------------------------------------------------------------------------- def v(name, type_, src, sep=".", values=None, default=None, note=None, - tdb=None, multi=False): + tdb=None, multi=False, aliases=None, valuesSrc=None): """One catalog entry. name variable name after the prefix and separator type_ bool | int | float | string | enum | list | color | hidden src file:line where the tree reads or writes it sep '.' (canonical) or '_' (legacy) - values allowed values for enums + values the values the CART may hold, exactly as the reader compares them + aliases other spellings a writer may use -> the canonical cart value. + Usually the trackDb vocabulary, which is often not the cart + vocabulary; see the autoScale entry for the case that proved it. + valuesSrc the C array the values were checked against. Name it for any + enum whose value list is not obvious from src, so the next reader + can re-verify instead of trusting this file. tdb trackDb setting that supplies the default, if differently named """ d = {"name": name, "type": type_, "sep": sep, "src": src} if values: d["values"] = values + if aliases: + d["aliases"] = aliases + if valuesSrc: + d["valuesSrc"] = valuesSrc if default is not None: d["default"] = default if tdb: d["tdbDefault"] = tdb if multi: d["multi"] = True if note: d["note"] = note return d # --------------------------------------------------------------------------- # LEVEL 2: variables every track can have, whatever its type # --------------------------------------------------------------------------- @@ -454,57 +464,91 @@ # --------------------------------------------------------------------------- # LEVEL 3: the type layer. Keyed by eCfgType where one exists # (hg/inc/trackDb.h:439) plus the types that fall through cfgByCfgType. # --------------------------------------------------------------------------- TYPES = { "wig": { "cfgType": "cfgWig", "tdbTypes": ["wig", "bigWig", "bedGraph", "mathWig", "instaPort"], "cfgUi": "hg/lib/hui.c:wigCfgUi", "cart": "hg/lib/wiggleCart.c", "families": [], "vars": [ v("minY", "float", "hg/inc/wiggle.h:MIN_Y", tdb="viewLimits"), v("maxY", "float", "hg/inc/wiggle.h:MAX_Y", tdb="viewLimits"), - v("autoScale", "enum", "hg/inc/wiggle.h:AUTOSCALE", - values=["on", "off", "group", "cumulative"], - tdb="autoScaleDefault"), + v("autoScale", "enum", "hg/lib/wiggleCart.c:448", + values=["use vertical viewing range setting", + "auto-scale to data view", + "group auto-scale"], + valuesSrc="hg/lib/hui.c:1973 wiggleScaleOptionsParent", + aliases={"off": "use vertical viewing range setting", + "on": "auto-scale to data view", + "group": "group auto-scale"}, + tdb="autoScaleDefault", + note="Two vocabularies, and this entry had the wrong one until " + "2026-08-08. The cart holds one of the three long " + "literals. on/off/group are the *trackDb* spellings, " + "translated to the literals at wiggleCart.c:454-459, and " + "they are never stored. 'cumulative' was in this list by " + "mistake: it is the C enum identifier wiggleScaleCumulative " + "(hui.h:576), not a value. This matters more than a " + "documentation slip because wiggleScaleStringToEnum() " + "errAborts on an unknown value (hui.c:1985), so " + "autoScale=on in a cart CRASHES hgTracks instead of being " + "ignored. A JSON writer must accept the aliases and store " + "the literal."), v("alwaysZero", "enum", "hg/inc/wiggle.h:ALWAYSZERO", - values=["on", "off"]), + values=["OFF", "ON"], + valuesSrc="hg/lib/hui.c:2173 wiggleAlwaysZeroOptions"), v("lineBar", "enum", "hg/inc/wiggle.h:LINEBAR", - values=["Bar", "Points"], tdb="graphTypeDefault"), + values=["points", "bar"], tdb="graphTypeDefault", + valuesSrc="hg/lib/hui.c:2017 wiggleGraphOptions"), v("transformFunc", "enum", "hg/inc/wiggle.h:TRANSFORMFUNC", - values=["NONE", "LOG"]), + values=["NONE", "LOG"], + valuesSrc="hg/lib/hui.c:2145 wiggleTransformFuncOptions"), v("negateValues", "bool", "hg/inc/wiggle.h:DONEGATIVEMODE"), v("sequenceLogo", "bool", "hg/inc/wiggle.h:DOSEQUENCELOGOMODE"), v("horizGrid", "enum", "hg/inc/wiggle.h:HORIZGRID", - values=["on", "off"], tdb="gridDefault"), + values=["ON", "OFF"], tdb="gridDefault", + valuesSrc="hg/lib/hui.c:2198 wiggleGridOptions"), v("yLineOnOff", "enum", "hg/inc/wiggle.h:YLINEONOFF", - values=["on", "off"]), + values=["OFF", "ON"], + valuesSrc="hg/lib/hui.c:1943 wiggleYLineMarkOptions"), v("yLineMark", "float", "hg/inc/wiggle.h:YLINEMARK"), v("smoothingWindow", "enum", "hg/inc/wiggle.h:SMOOTHINGWINDOW", - values=["off", "2", "3", "4", "...", "16"]), + values=["OFF", "2", "3", "4", "5", "6", "7", "8", "9", "10", + "11", "12", "13", "14", "15", "16"], + valuesSrc="hg/lib/hui.c:1913 wiggleSmoothingOptions", + note="The list used to be written 'off, 2, 3, 4, ..., 16', with " + "the ellipsis sitting in the values array as if it were a " + "value. Enumerated now, because a generated validator " + "would have accepted the literal string '...'."), v("windowingFunction", "enum", "hg/inc/wiggle.h:WINDOWINGFUNCTION", - values=["mean+whiskers", "maximum", "mean", "minimum", "sum"]), + values=["mean+whiskers", "maximum", "mean", "minimum", "sum"], + valuesSrc="hg/lib/hui.c:1880 wiggleWindowingOptions"), v("aggregate", "enum", "hg/inc/wiggle.h:AGGREGATE", values=["none", "transparentOverlay", "solidOverlay", "stacked", "add", "subtract"], + valuesSrc="hg/lib/hui.c:2054 aggregateExtraValues " + "(hui.h:652 WIG_AGGREGATE_*)", note="Container-level, but readable at leaf level too."), v("viewFunc", "enum", "hg/inc/wiggle.h:VIEWFUNC", - values=["showAll", "addAll", "subtractAll"]), + values=["showAll", "addAll", "subtractAll"], + valuesSrc="hg/lib/hui.c:2118 viewFuncValues " + "(hui.h:636 WIG_VIEWFUNC_*)"), v("missingMethod", "enum", "hg/lib/hui.c:6257", note="How to render gaps in the data."), v("heightPer", "int", "hg/inc/wiggle.h:HEIGHTPER", default="128"), ], "tdbOnly": ["viewLimits", "viewLimitsMax", "defaultViewLimits", "minLimit", "maxLimit", "maxHeightPixels", "gridDefault", "autoScaleDefault", "graphType", "graphTypeDefault", "spanList"], }, "bedScore": { "cfgType": "cfgBedScore", "tdbTypes": ["bed", "bigBed", "bed +", "bigNarrowPeak", "broadPeak"], "cfgUi": "hg/lib/hui.c:bedScoreCfgUi", "families": ["score", "numericFilter", "textFilter", "filterBy", "highlightBy", "label", "cds", "decorator"], @@ -775,32 +819,42 @@ v("noWhiteout", "bool", "hg/inc/barChartUi.h:58"), ], "tdbOnly": ["barChartBars", "barChartLabel", "barChartColors", "barChartCategoryUrl", "barChartUnit", "barChartMetric", "barChartMaxSize", "barChartSizeWindows", "barChartBarMinWidth", "barChartBarMinPadding", "barChartLimit"], }, "lolly": { "cfgType": "cfgLollipop", "tdbTypes": ["bigLolly"], "cfgUi": "hg/lib/hui.c:lollyCfgUi", "families": ["score", "numericFilter", "filterBy", "label"], "vars": [ v("heightPer", "int", "hg/lib/hui.c:lollyCfgUi"), - v("autoScale", "enum", "hg/lib/hui.c:lollyCfgUi", - values=["on", "off"]), + v("autoScale", "enum", "hg/hgTracks/lollyTrack.c:485", + values=["use vertical viewing range setting", + "auto-scale to data view", + "group auto-scale"], + valuesSrc="hg/lib/hui.c:1973 wiggleScaleOptionsParent", + aliases={"off": "use vertical viewing range setting", + "on": "auto-scale to data view", + "group": "group auto-scale"}, + note="Same var, same reader and same fix as wig.autoScale: " + "lollyTrack.c:485 calls wigFetchAutoScaleWithCart(), so the " + "cart vocabulary is the wiggle one, not on/off. This entry " + "carried on/off until 2026-08-08."), v("minY", "float", "hg/lib/hui.c:lollyCfgUi"), v("maxY", "float", "hg/lib/hui.c:lollyCfgUi"), v("popMethod", "enum", "hg/lib/hui.c:lollyCfgUi", tdb="popMethod"), ], }, "bigDbSnp": { "cfgType": "cfgBigDbSnp", "tdbTypes": ["bigDbSnp"], "cfgUi": "hg/lib/hui.c:bigDbSnpCfgUi", "families": ["filterBy", "label"], "vars": [ v("minMaf", "float", "hg/lib/hui.c:4954"), v("freqProj", "enum", "hg/lib/hui.c:freqSourceSelect", note="Which frequency project supplies MAF."), @@ -1420,30 +1474,80 @@ # --------------------------------------------------------------------------- # reconcile against the tree # --------------------------------------------------------------------------- def all_vars(): """Every catalog entry, from every section.""" out = [] for group in (COMMON, CONTAINER, FAMILIES, TYPES, BY_TRACK_NAME, OTHER_CGIS): for g in group.values(): out.extend(g["vars"]) out.extend(GLOBAL_BUT_TRACK_SPECIFIC) return out +ELLIPSIS = ("...", "…", "etc", "etc.") + + +def check_values(out=sys.stdout): + """Sanity checks on enum value lists. Returns the number of errors. + + The autoScale entry (fixed 2026-08-08) had three separate faults that all + came from curating a value list by eye: it recorded the trackDb vocabulary + instead of the cart vocabulary, it included a C enum identifier as if it + were a value, and nothing pointed at the array the values had to match. + A wrong value here is not a documentation slip - every wiggle + *StringToEnum() errAborts on an unknown string, so it crashes hgTracks. + These checks catch the mechanical part of that mistake. + """ + bad = 0 + noSrc = [] + for e in all_vars(): + vals = e.get("values") + name = e["name"] or "" + # A placeholder in a values list becomes a real accepted value the + # moment anything generates a validator from this file. + for val in vals or []: + if val.strip().lower() in ELLIPSIS: + print("%s: %r in values is a placeholder, not a value; " + "enumerate the list" % (name, val), file=sys.stderr) + bad += 1 + # An alias has to resolve to something the reader will accept. + for alias, target in (e.get("aliases") or {}).items(): + if not vals: + print("%s: alias %r on an entry with no values" + % (name, alias), file=sys.stderr) + bad += 1 + elif target not in vals: + print("%s: alias %r -> %r, which is not in values" + % (name, alias, target), file=sys.stderr) + bad += 1 + elif alias in vals: + print("%s: alias %r is also a value, so it cannot be " + "translated" % (name, alias), file=sys.stderr) + bad += 1 + if e["type"] == "enum" and vals and not e.get("valuesSrc"): + noSrc.append(name) + print("values ok" if not bad else "values: %d problem(s)" % bad, file=out) + # Not an error. It is the backlog: an enum whose values nobody has + # checked against the C array that actually gates them. + print("enums with no valuesSrc: %d (unverified value lists)" % len(noSrc), + file=out) + return bad + + def key(name): """A harvested and a cataloged name reduced to a comparable form. The leading separator goes because the harvester cannot always report it; see the module docstring. """ return name.lstrip("._") WILDCARD_RE = re.compile(r"<[a-zA-Z]+>|\*") def as_pattern(n): """Compile a catalog name with //* stand-ins into a regex.""" rx = re.escape(n) @@ -1771,30 +1875,38 @@ # var has an absolute name with no track component at all full = stand_in if not name else esc(name) elif sep in (".", "_"): # legacy entries already carry their leading underscore joiner = "" if name.startswith(sep) else sep full = stand_in + esc(joiner) + esc(name) else: full = esc(sep) + esc(name) cls = ' class="legacy"' if sep == "_" else "" bits = [] if e.get("multi"): bits.append('multi') if e.get("values"): bits.append('' + esc(" | ".join(e["values"])) + "") + if e.get("aliases"): + bits.append('also accepts ' + + esc(", ".join("%s = %s" % (a, t) for a, t + in sorted(e["aliases"].items()))) + + "") + if e.get("valuesSrc"): + bits.append('values: ' + + esc(e["valuesSrc"]) + "") if e.get("default") is not None: bits.append('default ' + esc(e["default"]) + "") if e.get("tdbDefault"): bits.append('trackDb: ' + esc(e["tdbDefault"]) + "") if e.get("note"): bits.append('' + esc(e["note"]) + "") out.append( "%s%s" "%s%s" % (cls, full, esc(e["type"]), " ".join(bits), esc(e["src"]))) return "\n".join(out) @@ -1984,20 +2096,23 @@ c = counts(cat) for k in sorted(c): print("%-12s %s" % (k, c[k])) # every family referenced by a type must exist bad = 0 for name, t in TYPES.items(): for fam in t.get("families", []): key = fam.split(" ")[0] if key not in FAMILIES and key not in TYPES: print("unknown family %r referenced by type %r" % (fam, name), file=sys.stderr) bad += 1 if bad: return 1 print("families ok") + bad += check_values() + if bad: + return 1 return 0 if __name__ == "__main__": sys.exit(main())