ba33f2c600399d5b4b97dd5fd987ed2db3c891ba chmalee Tue May 19 13:40:42 2026 -0700 myVariants related fixes from code review. Standardize how the code determines we are about to do something myVariants related by #defining some strings and using sameOk to check for them, remove a dead code block, and have hgc use autoSql to load the myVariants item rather than check the raw sql result, refs #37528 diff --git src/hg/lib/myVariants.c src/hg/lib/myVariants.c index 1ea8eb03d54..02ddd3bf6cf 100644 --- src/hg/lib/myVariants.c +++ src/hg/lib/myVariants.c @@ -1,29 +1,47 @@ #include "common.h" #include "linefile.h" #include "dystring.h" #include "jksql.h" #include "myVariants.h" #include "myVariantsShare.h" #include "customTrack.h" #include "hdb.h" #include "hgConfig.h" #include "cheapcgi.h" #include "trashDir.h" #include "obscure.h" #include "wikiLink.h" +boolean isMyVariantsType(char *type) +/* TRUE if type names the myVariants custom-track type. NULL-safe. */ +{ +return sameOk(type, MYVARIANTS_TYPE); +} + +boolean isMyVariantsTrack(char *trackName) +/* TRUE if trackName is a myVariants custom track (own or shared). NULL-safe. */ +{ +return trackName != NULL && startsWith(MYVARIANTS_TRACK_PREFIX, trackName); +} + +boolean isMyVariantsSharedTrack(char *trackName) +/* TRUE if trackName is a myVariants shared custom track. NULL-safe. */ +{ +return trackName != NULL && startsWith(MYVARIANTS_SHARED_TRACK_PREFIX, trackName); +} + void myVariantsStaticLoad(char **row, struct myVariants *ret) /* Load a row from myVariants table into ret. The contents of ret will be replaced at the next call to this function. */ { int sizeOne; ret->bin = sqlUnsigned(row[0]); ret->chrom = row[1]; ret->chromStart = sqlUnsigned(row[2]); ret->chromEnd = sqlUnsigned(row[3]); ret->name = row[4]; ret->score = sqlUnsigned(row[5]); safecpy(ret->strand, sizeof(ret->strand), row[6]); ret->thickStart = sqlUnsigned(row[7]); ret->thickEnd = sqlUnsigned(row[8]); ret->itemRgb = sqlUnsigned(row[9]); ret->blockCount = sqlUnsigned(row[10]); @@ -403,31 +421,31 @@ dyStringAppendC(dy, c); else dyStringPrintf(dy, "_%02x", c); } return dyStringCannibalize(&dy); } char *myVariantsGetTableName(char *userName) /* Build the SQL table name for this user's myVariants. Encodes the user * name so non-identifier characters (e.g. '@' in email-style logins) don't * fail sqlCheckIdentifier. */ { if (!userName) return NULL; char *encoded = encodeUserNameForIdentifier(userName); -struct dyString *tableName = dyStringCreate("myVariants_%s", encoded); +struct dyString *tableName = dyStringCreate(MYVARIANTS_TRACK_PREFIX "%s", encoded); freeMem(encoded); return dyStringCannibalize(&tableName); } char *myVariantsGetDbTable(char *userName) /* Return the string db.tableName based on the userName for use in sql statements * without specifying the database */ { char *db = myVariantsGetDatabaseForUser(userName); char *tbl = myVariantsGetTableName(userName); if (isNotEmpty(db) && isNotEmpty(tbl)) { struct dyString *ret = dyStringCreate("%s.%s", db, tbl); return dyStringCannibalize(&ret); } @@ -448,33 +466,33 @@ dbTable, targetDb); sqlUpdate(conn, del->string); dyStringFree(&del); hFreeConn(&conn); } struct myVariantsShare *myVariantsResolveSharedTrack(char *trackName, struct cart *cart) /* For a "myVariants_shared_*" custom-track name, look up and revalidate the * share record from hgcentral. Returns NULL if the track is not a shared * track, the cart cookie is missing, the share has been revoked, or the * current user is not authorized (target user mismatch). The returned share * carries the validated owner/db/project/permission; callers should use these * (not the cart-supplied values) for authorization or scoping decisions. * Caller frees with myVariantsShareFree. */ { -if (isEmpty(trackName) || !startsWith("myVariants_shared_", trackName)) +if (isEmpty(trackName) || !isMyVariantsSharedTrack(trackName)) return NULL; -char *token = trackName + strlen("myVariants_shared_"); +char *token = trackName + strlen(MYVARIANTS_SHARED_TRACK_PREFIX); char cartVar[256]; safef(cartVar, sizeof(cartVar), MYVAR_SHARED_CART_PREFIX "%s", token); /* The cart-cookie presence gate is belt-and-suspenders: it ensures the share * was once accepted into this session before we hit hgcentral. The real * authorization is the targetUser check below against the live share row. */ if (cart == NULL || !cartVarExists(cart, cartVar)) return NULL; struct sqlConnection *conn = hConnectCentral(); if (!sqlTableExists(conn, "myVariantsShares")) { hDisconnectCentral(&conn); return NULL; } struct myVariantsShare *share = myVariantsGetShareByToken(conn, token); hDisconnectCentral(&conn); @@ -489,70 +507,70 @@ return NULL; } } return share; } char *myVariantsResolveDbTableForCustomTrack(char *trackName, struct cart *cart) /* For a custom-track name of the form "myVariants_*", return the fully * qualified SQL table (db.tableName) holding the items. Handles both own * tracks and shared tracks. For shared tracks, revalidates the share against * hgcentral; returns NULL if the share has been revoked, downgraded out of * scope, or is not for the current user. */ { if (isEmpty(trackName)) return NULL; -if (startsWith("myVariants_shared_", trackName)) +if (isMyVariantsSharedTrack(trackName)) { struct myVariantsShare *share = myVariantsResolveSharedTrack(trackName, cart); if (share == NULL) return NULL; char *dbTable = myVariantsGetDbTable(share->ownerUser); myVariantsShareFree(&share); return dbTable; } -if (startsWith("myVariants_", trackName)) +if (isMyVariantsTrack(trackName)) { /* trackName is the SQL-identifier-encoded form "myVariants_<encoded>". * Resolve via the current logged-in user (an own track is only viewable * by its owner) and verify the trackName matches the encoded form for * that user before returning their db.tableName. */ char *userName = getUserName(); if (isEmpty(userName)) return NULL; char *expected = myVariantsGetTableName(userName); boolean match = sameOk(expected, trackName); freeMem(expected); if (!match) return NULL; return myVariantsGetDbTable(userName); } return NULL; } char *myVariantsSharedScopeWhere(char *trackName, struct cart *cart) /* For a "myVariants_shared_*" custom-track, return a SQL WHERE-clause * fragment that limits a query to the share's authorized project and db * (e.g. "db='hg38' and project='Variants'", or "db='hg38'" alone when the * share's project is "*"). Returns NULL for non-shared tracks or revoked * shares. Memoized per-process: callers receive a fresh cloneString that * they own. */ { static struct hash *cache = NULL; if (cache == NULL) cache = hashNew(0); -if (isEmpty(trackName) || !startsWith("myVariants_shared_", trackName)) +if (isEmpty(trackName) || !isMyVariantsSharedTrack(trackName)) return NULL; char *cached = hashFindVal(cache, trackName); if (cached != NULL) return cached[0] ? cloneString(cached) : NULL; char *result = NULL; struct myVariantsShare *share = myVariantsResolveSharedTrack(trackName, cart); if (share != NULL && isNotEmpty(share->db)) { struct dyString *dy = sqlDyStringCreate("db='%s'", share->db); if (isNotEmpty(share->project) && !sameString(share->project, "*")) sqlDyStringPrintf(dy, " and project='%s'", share->project); result = dyStringCannibalize(&dy); } myVariantsShareFree(&share); @@ -724,31 +742,31 @@ * ${persistentDir}/<encodedTableName>/<db>.bed. */ char *subdir = isNotEmpty(encodedTableName) ? encodedTableName : "shared"; char *sep = endsWith(persistentDir, "/") ? "" : "/"; char path[PATH_LEN]; safef(path, sizeof path, "%s%s%s/%s.bed", persistentDir, sep, subdir, targetDb); char dirPart[PATH_LEN]; splitPath(path, dirPart, NULL, NULL); makeDirsOnPath(dirPart); safef(tn->forCgi, sizeof tn->forCgi, "%s", path); safef(tn->forHtml, sizeof tn->forHtml, "%s", path); } else { char base[PATH_LEN]; char *hostPort = cgiServerNamePort(); - safef(base, sizeof base, "myVariants_%s_%s_%s", + safef(base, sizeof base, MYVARIANTS_TRACK_PREFIX "%s_%s_%s", hostPort ? hostPort : "localhost", targetDb, isNotEmpty(encodedTableName) ? encodedTableName : "shared"); for (char *p = base; *p; p++) if (*p == '/') *p = '_'; trashDirReusableFile(tn, "ct", base, ".bed"); } } void myVariantsUnlinkCtFile(char *userName, char *targetDb) /* Delete the on-disk ctfile for this user+assembly if it exists. Uses the * same path resolution as myVariantsWriteCtFile so it targets either the * persistent dir (myVariantsDataDir) or the trash fallback. */ { if (isEmpty(userName) || isEmpty(targetDb)) return; char *encodedTableName = myVariantsGetTableName(userName); @@ -793,31 +811,31 @@ hFreeConn(&conn); } /* Collect shared track lines from cart. All authoritative metadata * (owner/db/project/label) comes from the live share row, never from * cart-parsed values. */ struct dyString *sharedLines = dyStringNew(0); if (cart != NULL) { struct hashEl *shareVars = cartFindPrefix(cart, MYVAR_SHARED_CART_PREFIX); struct hashEl *el; for (el = shareVars; el != NULL; el = el->next) { char *token = el->name + strlen(MYVAR_SHARED_CART_PREFIX); char trackName[512]; - safef(trackName, sizeof(trackName), "myVariants_shared_%s", token); + safef(trackName, sizeof(trackName), MYVARIANTS_SHARED_TRACK_PREFIX "%s", token); struct myVariantsShare *share = myVariantsResolveSharedTrack(trackName, cart); if (share == NULL) continue; if (!sameString(share->db, targetDb)) { myVariantsShareFree(&share); continue; } /* Skip if the sharer is the current user - they already see their own track */ if (isNotEmpty(userName) && sameString(share->ownerUser, userName)) { myVariantsShareFree(&share); continue; } /* Verify the owner's table exists */ @@ -834,31 +852,32 @@ * of additional trackDb settings via the CT file track line. */ char *owner = cloneString(share->ownerUser); char *project = cloneString(share->project); char *label = isNotEmpty(share->label) ? cloneString(share->label) : NULL; stripChar(owner, '"'); stripChar(project, '"'); if (label != NULL) stripChar(label, '"'); char *projectLabel = sameString(project, "*") ? "All" : project; char shortLabel[64]; if (isNotEmpty(label)) safef(shortLabel, sizeof(shortLabel), "%s", label); else safef(shortLabel, sizeof(shortLabel), "%s's %s", owner, projectLabel); dyStringPrintf(sharedLines, - "track name=\"myVariants_shared_%s\" type=\"myVariants\" itemRgb=\"on\"" + "track name=\"" MYVARIANTS_SHARED_TRACK_PREFIX "%s\"" + " type=\"" MYVARIANTS_TYPE "\" itemRgb=\"on\"" " visibility=\"pack\"" " shortLabel=\"%s\"" " longLabel=\"Shared annotations: %s (from %s)\"\n", token, shortLabel, projectLabel, owner); freeMem(owner); freeMem(project); freeMem(label); myVariantsShareFree(&share); } hashElFreeList(&shareVars); } if (!hasOwnItems && dyStringLen(sharedLines) == 0) { /* Nothing to write: remove any stale on-disk file so it doesn't leak @@ -903,71 +922,71 @@ if (shortLabel == NULL) shortLabel = sanitizeLabel(diskShort, 80); if (longLabel == NULL) longLabel = sanitizeLabel(diskLong, 200); freeMem(diskShort); freeMem(diskLong); } if (shortLabel == NULL) shortLabel = cloneString("My Annotations"); if (longLabel == NULL) longLabel = cloneString("My Annotations"); } FILE *f = mustOpen(tn.forCgi, "w"); if (hasOwnItems) - fprintf(f, "track name=\"%s\" type=\"myVariants\" itemRgb=\"on\"" + fprintf(f, "track name=\"%s\" type=\"" MYVARIANTS_TYPE "\" itemRgb=\"on\"" " visibility=\"pack\" shortLabel=\"%s\"" " longLabel=\"%s\"\n", encodedTableName, shortLabel, longLabel); if (dyStringLen(sharedLines) > 0) fprintf(f, "%s", dyStringContents(sharedLines)); carefulClose(&f); dyStringFree(&sharedLines); freeMem(encodedTableName); freeMem(shortLabel); freeMem(longLabel); return cloneString(tn.forCgi); } boolean myVariantsHandleCtRemoval(struct customTrack *ct, struct cart *cart, char *database) /* If ct is a myVariants own track: delete the user's rows for the current * assembly, unlink the persistent ctfile, drop the per-db renamed labels * and the visibility var. If ct is a myVariants shared track: drop the * share-acceptance cart var and the visibility var. Returns TRUE when ct * was a myVariants track and this function fully handled the cart cleanup * (caller must skip its own per-track cart cleanup so other-assembly * labels are preserved); FALSE otherwise. */ { if (ct == NULL || ct->tdb == NULL || isEmpty(ct->tdb->track)) return FALSE; char *trackName = ct->tdb->track; -if (!startsWith("myVariants_", trackName)) +if (!isMyVariantsTrack(trackName)) return FALSE; /* Cleanup common to any myVariants removal: drop the track's visibility var * and invalidate the on-disk ctfile pointer so the next entry-point visit * regenerates from current SQL/share state. */ cartRemove(cart, trackName); char mvVar[256]; safef(mvVar, sizeof mvVar, MYVARIANTS_FILE_VAR_PREFIX "%s", database); cartRemove(cart, mvVar); -if (startsWith("myVariants_shared_", trackName)) +if (isMyVariantsSharedTrack(trackName)) { /* Drop the share-acceptance var so the share isn't re-imported. */ - char *token = trackName + strlen("myVariants_shared_"); + char *token = trackName + strlen(MYVARIANTS_SHARED_TRACK_PREFIX); char shareCartVar[256]; safef(shareCartVar, sizeof shareCartVar, MYVAR_SHARED_CART_PREFIX "%s", token); cartRemove(cart, shareCartVar); return TRUE; } /* Own track: delete the user's SQL rows for this assembly, unlink the * persisted ctfile, and drop any renamed-label cart vars so a freshly * created track on this assembly starts at "My Annotations" again. */ char *userName = wikiLinkUserName(); if (isNotEmpty(userName)) { myVariantsDeleteForDb(userName, database); myVariantsUnlinkCtFile(userName, database);