Skip to content

Validate category/subcategory length matches varchar(63) DB constraint#4867

Merged
dejanzele merged 1 commit intoarmadaproject:masterfrom
dejanzele:feat/classifier-length-validation
Apr 23, 2026
Merged

Validate category/subcategory length matches varchar(63) DB constraint#4867
dejanzele merged 1 commit intoarmadaproject:masterfrom
dejanzele:feat/classifier-length-validation

Conversation

@dejanzele
Copy link
Copy Markdown
Member

@dejanzele dejanzele commented Apr 23, 2026

What type of PR is this?

bug-fix / follow-up

What this PR does / why we need it

Follow-up to #4863. PR #4863 added failure_category varchar(63) and failure_subcategory varchar(63) columns to job_run (migration 032), but NewClassifier accepted arbitrarily long values from config. An operator setting a category name longer than 63 chars would only discover the problem at the first failed pod, when the lookout ingester's batch UPDATE hit value too long for type character varying(63) and stalled the batch.

This PR adds length guards at config-load time so the executor refuses to start with bad config rather than silently failing hours later. New constant maxCategoryNameLen = 63 documents the link to migration 032. NewClassifier validates DefaultCategory, DefaultSubcategory, every cfg.Name, and every rule's Subcategory.

Which issue(s) this PR fixes

Fixes #

Special notes for your reviewer

Addresses a review follow-up on #4863. A second follow-up from that review (handling rejected jobs for categorization purposes) is intentionally out of scope here - it's a design discussion rather than a mechanical change and belongs in a separate issue/PR.

Signed-off-by: Dejan Zele Pejchev <pejcev.dejan@gmail.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 23, 2026

Greptile Summary

This PR adds config-load-time length guards to NewClassifier and buildRule so the executor fails fast when a category or subcategory name exceeds the varchar(63) DB constraint, rather than silently stalling ingester batch UPDATEs at runtime. The four new test cases cover all validation paths and the constant is clearly documented with a migration reference.

Confidence Score: 5/5

Safe to merge; only finding is a minor byte-vs-rune precision note with no practical impact on ASCII category names.

All remaining findings are P2 style suggestions. The validation logic is correct and conservative — len() (bytes) can never pass a string that would violate the DB constraint, so no data-integrity risk exists. Tests are thorough and cover every new code path.

No files require special attention.

Important Files Changed

Filename Overview
internal/executor/categorizer/classifier.go Adds maxCategoryNameLen = 63 constant and length guards in NewClassifier and buildRule; logic is correct for ASCII names, though len() (bytes) vs utf8.RuneCountInString() (runes) is a minor style concern.
internal/executor/categorizer/classifier_test.go Adds 4 table-driven test cases covering all new validation paths (category name, subcategory, default category, default subcategory too long); coverage is complete.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[NewClassifier called] --> B{len DefaultCategory > 63?}
    B -- yes --> ERR1[return error]
    B -- no --> C{len DefaultSubcategory > 63?}
    C -- yes --> ERR2[return error]
    C -- no --> D[iterate Categories]
    D --> E{cfg.Name empty?}
    E -- yes --> ERR3[return error]
    E -- no --> F{len cfg.Name > 63?}
    F -- yes --> ERR4[return error]
    F -- no --> G{duplicate name?}
    G -- yes --> ERR5[return error]
    G -- no --> H[buildRule for each rule]
    H --> I{len cfg.Subcategory > 63?}
    I -- yes --> ERR6[return error]
    I -- no --> J[compile regex / validate matchers]
    J --> K[Classifier constructed successfully]
Loading

Reviews (1): Last reviewed commit: "Validate category/subcategory length mat..." | Re-trigger Greptile

Comment thread internal/executor/categorizer/classifier.go
@dejanzele dejanzele merged commit 7af991a into armadaproject:master Apr 23, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants