Proposed fixes

Author: cmdcolin

CRITICAL_BUGS.txt

@@ -1,8 +1,12 @@
 Critical Logic Bugs Found During Grails 7 Migration Review
 ===========================================================
 
-FIXED ISSUES
--------------
+This file contains only CRITICAL bugs: those that cause crashes, data corruption,
+prevent app startup, or completely break major features. Non-critical bugs and
+Groovy 4 compatibility issues are documented in separate files.
+
+CRITICAL BUGS (CAUSES CRASHES, DATA LOSS, OR WON'T START)
+---------------------------------------------------------
 
 1. MISSING RETURN AFTER ERROR RENDER (all controllers) [FIXED]
    ~100 methods across all controllers sent an error response but kept
@@ -11,12 +15,6 @@ FIXED ISSUES
    even after a permission error was already sent back. Server crashes from
    trying to send two responses.
 
-2. Bare `render jsonObject` without `as JSON` [FIXED]
-   ~56 render calls across 4 controllers sent JSON objects without setting
-   the content type.
-   CONSEQUENCE: Clients receive JSON data labeled as HTML. Browser-side
-   JSON parsing fails, breaking the annotation editor UI.
-
 3. PreferenceService NPEs on lines 161, 243, 298-299, 373-374 [FIXED]
    Accessed properties on possibly-null preference objects.
    CONSEQUENCE: Server crash (500 error) when a user has no saved
@@ -43,23 +41,12 @@ FIXED ISSUES
    Set/Map operations on FeatureProperty objects silently malfunction
    (contains() always returns false, remove() never matches, etc.).
 
-8. AnnotatorController variant method duplication [FIXED]
-   7 copy-pasted methods collapsed into a shared helper.
-   CONSEQUENCE: No runtime bug — just maintenance risk where a fix to
-   one copy wouldn't be applied to the other six.
-
 9. PreferenceService empty list .first() calls (lines 196, 698, 709) [FIXED]
    Calls .first() on lists that may be empty.
    CONSEQUENCE: Server crash during login or organism-switching when
    the database has no sequences or organisms. Common during initial
    setup — the app becomes unusable until data is loaded.
 
-10. Quartz scheduling replaced with Spring @Scheduled [FIXED]
-    CleanupPreferencesJob used the Quartz plugin, which has no Grails 7
-    compatible version.
-    CONSEQUENCE: Without this fix, preference cleanup never runs, and
-    stale preferences accumulate indefinitely in the database.
-
 11. FeatureService.groovy NPE risks [FIXED]
     - Line 803: Assumes a transcript always has a feature location.
     - Lines 1528-1529: Assumes CV terms always exist in the database.
@@ -78,29 +65,6 @@ FIXED ISSUES
     500 instead of a helpful error message.
     FIX: Added null checks with error logging and skip/continue.
 
-13. PermissionService.groovy dead code (line 420-421) [FIXED]
-    The fallback branch passes null as a query parameter, so the query
-    always returns nothing.
-    CONSEQUENCE: No user-visible bug — the fallback on line 429 handles
-    it. But a wasted database query runs on every permission check where
-    no sequence name is provided. Minor performance cost.
-    FIX: Removed the dead query; branch now directly sets sequence = null,
-    falling through to the existing fallback on line 429.
-
-14. AnnotationEditorController.getInformation permission mismatch (line 301) [FIXED]
-    Checks WRITE permission without the request context.
-    CONSEQUENCE: May check permissions against the wrong organism.
-    Could show or hide feature information incorrectly.
-    FIX: Changed checkPermissions(PermissionEnum.WRITE) to
-    checkPermissions(inputObject, PermissionEnum.WRITE).
-
-15. GroupController.updateGroup sends no response on success (line 246-281) [FIXED]
-    Only the error path sends a response. Success sends nothing.
-    CONSEQUENCE: Client receives an empty response after a successful
-    group update. If the client expects JSON confirmation, it shows an
-    error despite the operation succeeding.
-    FIX: Added `render new JSONObject() as JSON` after successful save.
-
 17. Groovy 4 log.error() type mismatch (multiple files) [FIXED]
     `log.error(e)` / `log.error(map)` / `log.error(e.printStackTrace())`
     pass non-String arguments to SLF4J Logger.error().
@@ -110,11 +74,6 @@ FIXED ISSUES
     GroupController, VcfController.
     FIX: Changed to `log.error(e.message, e)`.
 
-18. Groovy 4 e.printStackTrace() calls (13 instances, 11 files) [FIXED]
-    Prints to stderr instead of the logging framework.
-    CONSEQUENCE: Error output lost in production (no log file capture).
-    FIX: Replaced all 13 instances with `log.error(e.message, e)`.
-
 19. Groovy 4 Date.minus() removed (FeatureEventController line 216) [FIXED]
     `today.minus(20*365)` used a method removed in Groovy 4.
     CONSEQUENCE: Feature event history queries crash on startup.
@@ -147,7 +106,7 @@ FIXED ISSUES
     simplified 4 repeated if-blocks into a loop.
 
 23. SequenceController.setCurrentSequence null binding + session NPE [FIXED]
-    Two issues:
+    Three issues:
     a) Method signature `setCurrentSequence(Sequence sequenceInstance)`
        relies on Grails 7 domain class data binding, which fails when no
        ID is in the URL path. API callers sending `sequenceName` in JSON
@@ -193,28 +152,6 @@ FIXED ISSUES
     cannot start at all with a PostgreSQL database.
     FIX: Added `failOnError: true, flush: true` and null checks.
 
-28. PermissionService.getOrganismsWithMinimumPermission/getOrganismsWithPermission
-    wrong user (lines 101, 128) [FIXED]
-    Both methods accept a `User user` parameter but call
-    `getOrganismPermissionsForUser(organism, currentUser)` instead of
-    using the passed-in `user`. `currentUser` resolves to `getCurrentUser()`
-    which returns whoever is currently logged in.
-    CONSEQUENCE: When checking permissions for a different user (e.g. admin
-    viewing another user's organism list), the check runs against the admin's
-    own permissions instead. Could show incorrect organism lists in the
-    admin user management UI.
-    FIX: Changed `currentUser` to `user` on both lines.
-
-29. PreferenceService.evaluateSaves dead branch (lines 418-422) [FIXED]
-    Both branches of the `if (onlySaveToken && ...)` / `else` call
-    `evaluateSave()` with identical arguments. The `onlySaveToken` filter
-    has no effect — all tokens are always saved regardless.
-    CONSEQUENCE: When `onlySaveToken` is set (intended to save only one
-    specific client token's preferences), all tokens are saved instead.
-    Minor — causes unnecessary database writes but no data corruption.
-    FIX: Changed to `if (!onlySaveToken || onlySaveToken == ...)` with
-    a single `evaluateSave()` call, so non-matching tokens are skipped.
-
 30. FeatureService.convertFeatureToJSONLite wrong variable in owner loop
     (line 2042) [FIXED]
     Loop iterates `for (owner in gsolFeature.owners)` but body uses
@@ -224,13 +161,6 @@ FIXED ISSUES
     If gsolFeature.owner is null but gsolFeature.owners is non-empty, NPE.
     FIX: Changed to `owner.username`.
 
-31. AnnotationEditorController.getUserPermission dead code (lines 85-88) [FIXED]
-    Built a permissions HashMap with one entry, then immediately overwrote
-    it with `permissionService.getPermissionsForUser(user)`.
-    CONSEQUENCE: No runtime bug — 4 lines of dead code wasting CPU on
-    an unused organism permission lookup every time getUserPermission runs.
-    FIX: Removed the dead lines; kept only the effective call.
-
 32. @Transactional closure conversions — second pass [FIXED]
     Converted remaining .each{}, .collect{}, .findAll{} closures to for
     loops in three more files:
@@ -253,13 +183,6 @@ FIXED ISSUES
     added to the wrong allele. Allele updates are silently corrupted.
     FIX: Changed to use `oldAlleleBases` and `newAlleleBases` respectively.
 
-34. VariantService.updateAlternateAlleles undefined variables in log (line 101) [FIXED]
-    Log message referenced `${oldAltAlleleFrequency}` and `${oldProvenance}`
-    which are never defined in the method.
-    CONSEQUENCE: Groovy resolves undefined variables as null, so the log
-    message prints "null" instead of useful information. Not a crash.
-    FIX: Simplified log message to use defined variables.
-
 35. VariantService.deleteAlleleInfo missing save (line 216) [FIXED]
     Method returns `feature` without calling `feature.save()`. Every other
     similar method in VariantService calls `save(flush: true, failOnError: true)`.
@@ -274,15 +197,6 @@ FIXED ISSUES
     no parent gene (e.g. orphaned transcripts during complex editing).
     FIX: Moved `gene.save()` inside the `if (gene)` block.
 
-37. UserController.deleteUser render before delete (lines 558-559) [FIXED]
-    `render jsonObject as JSON` was called before `user.delete()`. The
-    jsonObject was built from `user.properties` which includes internal
-    Hibernate state. After render, the user object is deleted, but the
-    response may reference stale/detached state.
-    CONSEQUENCE: Response may include internal Hibernate properties. If
-    delete fails, the response already told the client it succeeded.
-    FIX: Build a clean JSONObject, delete first, then render.
-
 38. GroupController.updateGroup wrong type conversion (line 225) [FIXED]
     `dataObject.id.split(',').collect() as Long` casts the entire list
     to Long instead of converting each element.
@@ -291,12 +205,6 @@ FIXED ISSUES
     management UI breaks for multi-group operations.
     FIX: Changed to `.collect { Long.parseLong(it.trim()) }`.
 
-39. GroupController.updateGroup wrong error message (line 265) [FIXED]
-    Error message says "Failed to delete the group" in the updateGroup
-    method. Copy-paste from deleteGroup.
-    CONSEQUENCE: Misleading error message shown to admin users.
-    FIX: Changed to "Failed to find the group".
-
 40. AnnotatorController.updateFeature status handling (lines 257-260) [FIXED]
     Two bugs in status deletion:
     a) `Status oldStatus = data.status` — should be `feature.status` (data.status
@@ -316,58 +224,3 @@ FIXED ISSUES
     CONSEQUENCE: FASTA export crashes with NPE for features that have
     no retrievable sequence (e.g. features on unloaded sequences).
     FIX: Combined null and empty check: `if (seq == null || seq.length() == 0)`.
-
-42. CdsService.setStopCodonReadThrough duplicate relationship (lines 120-135) [FIXED]
-    When `replace=true`, `setChildForType()` replaces the existing
-    relationship. But then lines 125-131 ALWAYS create a new
-    FeatureRelationship regardless, resulting in a duplicate.
-    CONSEQUENCE: Two identical CDS→StopCodonReadThrough relationships
-    exist in the database. Downstream code that expects exactly one
-    (like `hasStopCodonReadThrough()`) may behave incorrectly.
-    FIX: If setChildForType succeeds (returns true), save and return
-    early without creating a new relationship.
-
-43. ReportService wrong domain class in HQL queries (lines 82, 84, 162, 163) [FIXED]
-    Four HQL queries used `TransposableElement.executeQuery(...)` to run
-    queries for `RepeatRegion` and `Exon` counts. GORM dispatches queries
-    through the calling domain class's session context.
-    CONSEQUENCE: RepeatRegion and Exon counts in annotator reports may
-    return incorrect results or fail if TransposableElement's mapping
-    doesn't cover those HQL paths.
-    FIX: Changed to `RepeatRegion.executeQuery(...)` and
-    `Exon.executeQuery(...)` respectively.
-
-44. ReportService transcriptCount overwritten (lines 28-34) [FIXED]
-    Lines 28-31 calculated transcriptCount from a type map, then line 34
-    unconditionally overwrote it with `Transcript.count`. The conditional
-    calculation was dead code.
-    FIX: Removed the dead conditional block.
-
-45. ReportService unreachable admin branch (lines 188-200) [FIXED]
-    Inside `if (!isUserGlobalAdmin(owner))`, a nested
-    `if (isUserGlobalAdmin(owner))` is always false. The entire
-    Organism.listOrderByCommonName branch was dead code.
-    FIX: Removed the dead branch, kept only the non-admin path,
-    and converted the .each closure to a for loop.
-
-46. HomeController CSV column order mismatch (line 92) [FIXED]
-    Header said `total,count,mean,max,min,stddev` but data was written
-    as `total,count,min,max,mean,stddev`.
-    CONSEQUENCE: CSV downloads have swapped min/mean columns. Any
-    pipeline parsing by column name would get wrong values.
-    FIX: Changed header to match data order: `total,count,min,max,mean,stddev`.
-
-47. ProxyController null queryString (line 62) [FIXED]
-    `targetUrl += "?" + request.queryString` concatenates "?null" when
-    there are no query parameters.
-    CONSEQUENCE: Proxy requests without query params produce malformed
-    URLs ending in "?null", which may confuse downstream services.
-    FIX: Added null check on request.queryString before appending.
-
-REMAINING ISSUES
------------------
-
-16. IOServiceController.write() cleanup after error render (line 256-258)
-    After the catch block renders an error, code continues to run
-    outputFile.deleteOnExit(). This is intentional cleanup, not a bug —
-    but noted for completeness.