Skip to content

fix(findings): normalize blank components to NULL (SC-13073)#15038

Open
Maffooch wants to merge 2 commits into
bugfixfrom
bugfix-13073-component-none
Open

fix(findings): normalize blank components to NULL (SC-13073)#15038
Maffooch wants to merge 2 commits into
bugfixfrom
bugfix-13073-component-none

Conversation

@Maffooch

Copy link
Copy Markdown
Contributor

Summary

Fixes SC-13073 — removing a Finding's component created a separate "None" component group.

The All Components view groups findings by component_name. Because the database treats an empty string ("") as distinct from NULL, findings whose component was stored as "" (via the API or importers) appeared as a second "None" row, separate from findings with a NULL component.

Note: the finding edit form already stores NULL (Django sets empty_value=None for null=True CharFields), so the inconsistency came from other write paths.

Changes

  • Finding.save() now normalizes blank (empty or whitespace-only) component_name / component_version to NULL, so every write path is consistent.
  • Data migration 0269_normalize_blank_finding_components converts existing empty-string component values to NULL on upgrade.
  • Upgrade note added under docs/content/releases/os_upgrading/3.1.md.

Testing

  • unittests.test_finding_model.TestFindingComponentNormalization — new (verified red→green; empty/whitespace → NULL, real values preserved, NULL + "" findings group together).
  • unittests.test_edit_finding_endpoints.TestEditFindingComponentView — new (clearing components via the edit view stores NULL).
  • test_finding_model → 67 OK, test_edit_finding_endpoints → 14 OK.

🤖 Generated with Claude Code

Findings could store an empty string for component_name/component_version
depending on the write path (the edit form already stores NULL, but the API
and importers do not). Because the database treats "" as distinct from NULL,
the All Components view grouped these into a separate "None" row.

Finding.save() now normalizes blank (empty or whitespace-only) component
values to NULL, and a data migration converts existing empty-string values
so component-less findings group together.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Maffooch Maffooch requested a review from mtesauro as a code owner June 17, 2026 17:50
@github-actions github-actions Bot added New Migration Adding a new migration file. Take care when merging. docs unittests labels Jun 17, 2026
Comment on lines +18 to +26
blank_name = Finding.objects.annotate(
component_name_trimmed=Trim("component_name"),
).filter(Q(component_name="") | Q(component_name_trimmed=""))
name_updated = blank_name.update(component_name=None)

blank_version = Finding.objects.annotate(
component_version_trimmed=Trim("component_version"),
).filter(Q(component_version="") | Q(component_version_trimmed=""))
version_updated = blank_version.update(component_version=None)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we do some chunking with batch_size here to avoid it having getting/updating "millions" of findings at a time?

@valentijnscholten valentijnscholten left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^

Process blank component_name/component_version rows in bounded seek-paged
chunks instead of two unbounded UPDATE statements, so the migration never
locks/writes "millions" of findings at once. Mirrors the page_size idiom in
0201_populate_finding_sla_expiration_date.

Addresses review feedback on PR #15038.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@valentijnscholten valentijnscholten left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm my initial thought was that just using batch_size= on the update would do it?

@Maffooch

Copy link
Copy Markdown
Contributor Author

Now we get logging too! A "hung" migration is nerve racking

@mtesauro mtesauro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@mtesauro mtesauro requested review from Jino-T and dogboat June 18, 2026 18:49
@mtesauro mtesauro added this to the 3.0.100 milestone Jun 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs New Migration Adding a new migration file. Take care when merging. unittests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants