Fix POST /queries returning 500 on JSON null name/query (#43031)

[juan-fdz-hawa]

Related issue: Fixes #43031

Make sure we reject nil Name or Query in NewQuery with a BadRequestError before Verify().

Checklist for submitter

If some of the following don't apply, delete the relevant line.

• Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
  See Changes files for more information.

Testing

• Added/updated automated tests
• QA'd all new/changed functionality manually

Summary by CodeRabbit

• Bug Fixes

  • Fixed the query creation endpoint to return HTTP 400 instead of HTTP 500 when required fields (name and query) are provided as null values in the request payload.

• Tests

  • Added test coverage for query creation endpoints to verify proper error handling when null values are submitted for required fields.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

_{Tip: disable this comment in your organization's Code Review settings.}

[coderabbitai]

Walkthrough

This PR fixes an issue where POST /api/v1/fleet/queries returned HTTP 500 instead of HTTP 400 when the name or query fields were provided as JSON null. The fix adds explicit nil validation in Service.NewQuery to reject null values for these required fields before database operations. Unit and integration tests verify the new validation behavior for both the direct /queries endpoint and the bulk /spec/queries endpoint.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title accurately summarizes the main change: fixing POST /queries to return 400 instead of 500 when name or query is JSON null.
Description check	✅ Passed	The PR description covers required sections: related issue reference, completion of relevant checklist items for changes file and testing, but omits several non-critical sections.
Linked Issues check	✅ Passed	All code changes directly address the requirements in issue #43031: nil checks for Name/Query in NewQuery method, returning BadRequestError, and comprehensive test coverage for the fix.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the HTTP 500 error for JSON null name/query; no out-of-scope modifications detected.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 43031-post-queries-returns-http-500-when-name-or-query-is-json-null

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

server/service/queries_test.go (1)

143-165: ⚡ Quick win

Assert the specific bad-request error for nil required fields.

These new cases currently only assert “any error.” Tightening to BadRequestError + message will better lock the regression fix.

Proposed test hardening

 testCases := []struct {
 	name         string
 	queryPayload fleet.QueryPayload
 	shouldErr    bool
+	errContains  string
 }{
@@
 	{
 		"Nil name",
 		fleet.QueryPayload{
 			Query:   ptr.String("select 1"),
 			Logging: ptr.String("snapshot"),
 		},
 		true,
+		"name and query fields are required",
 	},
 	{
 		"Nil query",
 		fleet.QueryPayload{
 			Name:    ptr.String("test query"),
 			Logging: ptr.String("snapshot"),
 		},
 		true,
+		"name and query fields are required",
 	},
 	{
 		"Nil name and query",
 		fleet.QueryPayload{
 			Logging: ptr.String("snapshot"),
 		},
 		true,
+		"name and query fields are required",
 	},
 }
@@
 			if tt.shouldErr {
 				assert.Error(t, err)
 				assert.Nil(t, query)
+				if tt.errContains != "" {
+					assert.ErrorContains(t, err, tt.errContains)
+				}
 			} else {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/service/queries_test.go` around lines 143 - 165, Update the
table-driven tests in queries_test.go for the cases "Nil name", "Nil query", and
"Nil name and query" to assert the exact bad-request type and message instead of
any error: when invoking the handler/validator with fleet.QueryPayload missing
Name or Query, assert the returned error is a BadRequestError (use the project's
BadRequestError type/constructor) and that its error string contains the
expected validation message (e.g., "name is required" for the Nil name case,
"query is required" for the Nil query case, and an appropriate combined message
for Nil name and query); locate the checks around the failing test cases in
queries_test.go and replace the generic any-error assertions with these specific
type-and-message assertions.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@server/service/queries_test.go`:
- Around line 143-165: Update the table-driven tests in queries_test.go for the
cases "Nil name", "Nil query", and "Nil name and query" to assert the exact
bad-request type and message instead of any error: when invoking the
handler/validator with fleet.QueryPayload missing Name or Query, assert the
returned error is a BadRequestError (use the project's BadRequestError
type/constructor) and that its error string contains the expected validation
message (e.g., "name is required" for the Nil name case, "query is required" for
the Nil query case, and an appropriate combined message for Nil name and query);
locate the checks around the failing test cases in queries_test.go and replace
the generic any-error assertions with these specific type-and-message
assertions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0a481f83-8e26-44ee-8f2b-692a7f80adec

📥 Commits

Reviewing files that changed from the base of the PR and between 9a5fae6 and 44b3f72.

📒 Files selected for processing (4)

• changes/43031-post-queries-null-name-or-query-500
• server/service/integration_core_test.go
• server/service/queries.go
• server/service/queries_test.go

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.86%. Comparing base (81bc4a3) to head (44b3f72).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #45253      +/-   ##
==========================================
+ Coverage   66.81%   66.86%   +0.05%     
==========================================
  Files        2724     2724              
  Lines      219027   219087      +60     
  Branches    10754    10754              
==========================================
+ Hits       146342   146499     +157     
+ Misses      59519    59423      -96     
+ Partials    13166    13165       -1     

Flag	Coverage Δ
backend	68.74% <100.00%> (+0.06%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 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.

[juan-fdz-hawa]

Failures unrelated to changes