Skip to content

Fix error handling on the os_versions API endpoint - #49899

Merged
lucasmrod merged 6 commits into
fleetdm:mainfrom
Dhvanit41:fix-49483-os-versions-error-handling
Aug 6, 2026
Merged

Fix error handling on the os_versions API endpoint#49899
lucasmrod merged 6 commits into
fleetdm:mainfrom
Dhvanit41:fix-49483-os-versions-error-handling

Conversation

@Dhvanit41

@Dhvanit41 Dhvanit41 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Related issue: Resolves #49483

What & why

The /os_versions API endpoint returned misleading success responses for three invalid inputs. This PR makes each return a proper error:

  1. Invalid platform filter (e.g. ?platform=notrealplatform) previously returned count: 0 with 200 OK, indistinguishable from "no matching OS versions." It now returns a 422 validation error listing the supported platforms (darwin, windows, linux, chrome, ios, ipados, android — matching the documented filter values).

  2. Unknown OS version id (e.g. /os_versions/99999) previously returned 200 OK with a null/zero-filled os_version object. It now returns a not-found (404) error.

  3. Negative max_vulnerabilities (e.g. ?max_vulnerabilities=-5) returned a message reading must be >= 0 — Go's JSON encoder HTML-escapes >. The message is reworded to max_vulnerabilities cannot be negative, which is clearer and avoids the escaped character.

⚠️ Note for reviewer (fix #2)

The single-version handler previously swallowed the datastore's not-found error and returned an empty result on purpose, with the comment: "It is possible the os version exists, but the aggregation job has not run yet." This PR removes that swallow so a missing id returns 404. If you'd prefer to preserve the empty-result behavior for the "not yet aggregated" case, I'm happy to adjust — flagging so the change is intentional and visible.

Checklist for submitter

  • Changes file added for user-visible changes in changes/.
  • Input data is properly validated, SELECT * is avoided, SQL injection is prevented.

Testing

  • Added/updated automated tests (TestOSVersionsErrorHandling in server/service/hosts_test.go, covering all three cases).
  • QA'd all new/changed functionality manually

Summary by CodeRabbit

  • Bug Fixes
    • Unsupported OS platform filters now return a clear validation error instead of an empty-like result.
    • Unknown OS version IDs now return HTTP 404 Not Found rather than a success response with null/zero fields.
    • max_vulnerabilities validation now rejects negative values with an accurate, readable message and consistent HTTP 422 responses.
    • Error responses for OS versions endpoints now reflect the correct status codes.
  • Tests
    • Updated and added coverage to assert the new error-handling and HTTP status expectations.

Previously /os_versions returned misleading successful responses for invalid input. Now:
- an invalid `platform` filter returns a 422 validation error
- an unknown OS version id returns a 404 not-found error
- the max_vulnerabilities validation message no longer contains ">", which the JSON encoder was escaping to "\u003e"

Adds TestOSVersionsErrorHandling covering all three cases.

Resolves fleetdm#49483
@Dhvanit41
Dhvanit41 requested a review from a team as a code owner July 24, 2026 15:58
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 41eda713-7429-4efc-9c79-9a65046df44e

📥 Commits

Reviewing files that changed from the base of the PR and between 09ff6f2 and 7eb1a73.

📒 Files selected for processing (1)
  • server/service/integration_enterprise_test.go

Walkthrough

Updated the OS versions service to reject unsupported platform filters and negative max_vulnerabilities values with corrected validation messages. Changed unknown OS version ID handling to return a wrapped not-found error instead of an empty result. Added tests covering invalid and valid platforms, validation messaging, and missing OS version responses.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change to os_versions error handling.
Description check ✅ Passed The description covers the related issue, behavior changes, testing, and checklist items required by the template.
Linked Issues check ✅ Passed The PR satisfies [#49483] by adding validation errors for invalid platform and max_vulnerabilities values and 404s for unknown OS version IDs.
Out of Scope Changes check ✅ Passed The code changes and tests stay focused on os_versions error handling; no unrelated modifications stand out.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ast-grep (0.44.1)
server/service/integration_enterprise_test.go

ast-grep timed out on this file


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.92%. Comparing base (5b36fb5) to head (75bcb60).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #49899      +/-   ##
==========================================
- Coverage   67.92%   67.92%   -0.01%     
==========================================
  Files        3920     3906      -14     
  Lines      249774   249745      -29     
  Branches    13300    13334      +34     
==========================================
- Hits       169663   169629      -34     
+ Misses      64837    64834       -3     
- Partials    15274    15282       +8     
Flag Coverage Δ
backend 69.31% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Dhvanit41 and others added 4 commits July 24, 2026 22:40
Adjusts existing integration tests that asserted the previous behavior:
- GET /os_versions/{id} with a missing id now expects 404 (was 200 with an empty object)
- the max_vulnerabilities validation message is now "cannot be negative"
Two team-scoped single-version lookups (an OS version id requested for a
team that has no hosts, and by a user without access to the host's team)
now return 404 instead of a 200 with an empty object. One of these cases
already had a code comment saying it "should get 404".

@lucasmrod lucasmrod 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.

Looks good. Thanks for the contribution!

@lucasmrod

Copy link
Copy Markdown
Member

Change approved by @rachaelshaw (during our standup call)

@lucasmrod
lucasmrod merged commit a6b541d into fleetdm:main Aug 6, 2026
39 checks passed
@Dhvanit41
Dhvanit41 deleted the fix-49483-os-versions-error-handling branch August 14, 2026 08:06
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.

Error handling improvements for /api/v1/fleet/os_versions endpoint

2 participants