[vishnums2k5]

Fixes #

What it does

Standardizes problem reporting error responses and query parameter parsing for both stop and trip endpoints:

1. Replaces raw http.Error calls with api.serverErrorResponse(w, r, err) so failures return standard OneBusAway JSON error envelopes with proper Content-Type: application/json, currentTime, and version fields.
2. Accepts both OpenAPI specification casing (vehicleID, stopID) and legacy casing (vehicleId, stopId) in reportProblemWithTripHandler.
3. Stores omitted optional string parameters (code, userComment, serviceDate, vehicleID, stopID, userVehicleNumber) as SQL NULL via nulls.NonEmptyString rather than empty strings with Valid: true.

Root cause

reportProblemWithStopHandler and reportProblemWithTripHandler previously used http.Error, which emitted plain-text responses without the standard OneBusAway envelope structure. Additionally, testdata/openapi.yml specifies vehicleID and stopID as query parameters, whereas the handler only looked up vehicleId and stopId. Lastly, empty strings were being converted to sql.NullString{Valid: true} via nulls.String(""), persisting empty strings rather than database NULL values.

Testing

• Test failing on main before this change:
  • TestReportProblemWithStopDatabaseFailure: Returned text/plain 500 without JSON body/version.
  • TestReportProblemWithStopNullStorage: Failed assertion because omitted fields had Valid: true.
  • TestReportProblemWithTripDatabaseFailure: Returned text/plain 500 without JSON body/version.
  • TestReportProblemWithTripOpenAPICasingAndNulls: Parameters vehicleID and stopID were ignored, and omitted fields stored "" instead of NULL.
• Manual check:
  • go test -tags "sqlite_fts5 sqlite_math_functions" -v ./internal/restapi -run "TestReportProblemWith": PASS
  • make test: PASS (all packages)
  • make test-pure: PASS (all packages)
  • go vet (both CGO and purego): PASS

Response comparison (delete if no Java-served response changed)

Checklist

Ready-for-review requirement: Every box must be checked. If any statement is not true, keep the pull request as Draft.

• This PR addresses one issue.
• It is under ~200 changed lines, or the description says why it can't be split.
• For a bug fix, the new or changed test fails on main and passes on this branch.
• Each commit is one logical change with a clear message.
• I did not hand-edit gtfsdb/ (sqlc-generated) or testdata/openapi.yml (synced from upstream).
• Handlers pass r.Context() to DB/service calls, not context.Background().
• The response comparison above is filled in, or this PR doesn't change a Java-served response.
• Any new endpoint matches testdata/openapi.yml, or an issue asking for it is open on OneBusAway/sdk-config.
• I read every line of this diff and can explain it if asked.

Notes for reviewers (optional)

Both vehicleID/stopID (OpenAPI) and vehicleId/stopId (legacy) are accepted to ensure backward compatibility with both official OpenAPI-generated SDKs and existing web clients.

Summary by CodeRabbit

• Bug Fixes
  • Stop and trip problem reports now return consistent server-error responses when the database is unavailable or a report cannot be saved.
  • Trip reports accept both vehicleID/stopID and vehicleId/stopId parameter casing.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 50817057-dab5-4901-bf1b-5f7642898bde

📥 Commits

Reviewing files that changed from the base of the PR and between b0ad9a1 and b0e31e0.

📒 Files selected for processing: 4

• internal/restapi/report_problem_with_stop_handler.go
• internal/restapi/report_problem_with_stop_handler_test.go
• internal/restapi/report_problem_with_trip_handler.go
• internal/restapi/report_problem_with_trip_handler_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

The stop and trip problem-report handlers now route initialization and database-write errors through serverErrorResponse. Empty optional string values are stored as SQL NULL. The trip handler also accepts OpenAPI-cased vehicleID and stopID query parameters.

Changes: Problem report handlers

Layer / File(s)	Summary
Stop report handling internal/restapi/report_problem_with_stop_handler.go, internal/restapi/report_problem_with_stop_handler_test.go	The handler uses serverErrorResponse for initialization and write errors. Empty report code and user comment values are treated as nullable. Tests check the error response and SQL NULL values for omitted fields.
Trip report handling internal/restapi/report_problem_with_trip_handler.go, internal/restapi/report_problem_with_trip_handler_test.go	The handler uses serverErrorResponse for initialization and write errors, accepts uppercase-ID query keys with camel-case fallback, and treats empty optional strings as nullable. Tests check the error response, accepted query keys, and SQL NULL values.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: fletcherw

Merge Risk: ⚪ Minimal · up to b0e31

The report-handler changes have no identified issue that needs resolution before merge.

Pre-merge checks: 3 · 2

Failed checks: 2 warnings

Check name	Status	Explanation	Resolution
Description check		The description clearly explains the handler changes, root cause, tests, and checklist status. However, the response comparison section is empty even though the PR changes Java-served error responses,…	Fill in the required response comparison with the request URLs, Java reference, Maglev responses, and any intentional differences. Add the triaged issue number or keep the PR as Draft if no issue exists.
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 3 passed

Check name	Status	Explanation
Title check		The title is concise and accurately summarizes the primary changes to problem-reporting response envelopes and parameters.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Description check

Explanation

The description clearly explains the handler changes, root cause, tests, and checklist status. However, the response comparison section is empty even though the PR changes Java-served error responses, and the issue reference remains blank.

Resolution

Fill in the required response comparison with the request URLs, Java reference, Maglev responses, and any intentional differences. Add the triaged issue number or keep the PR as Draft if no issue exists.

• Fix all pre-merge checks with AI

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[aaronbrethorst]

Thanks @vishnums2k5! The switch to serverErrorResponse and storing omitted strings as NULL both look good. Java leaves these fields null when they're omitted, so this matches it. One thing needs to change before this can land.

Accepting vehicleID/stopID on report-problem-with-trip changes what we store compared with Java. ReportProblemWithTripAction only has setVehicleId/setStopId, and Struts matches parameter names case-sensitively, so production drops vehicleID= and stopID= without a word. You can see the same case sensitivity on a read endpoint: stops-for-location?maxCount=1 returns 1 stop, while maxcount=1 is ignored and returns 49. With this PR, a request that sends vehicleID=1_3521 stores the vehicle, and on Java it doesn't.

This could well be the right call. The js-sdk sends vehicleID/stopID because that's what openapi.yml says, so on Java those values get lost. But it's a deviation from Java, so it needs a maintainer decision and an Implementation Decisions entry first. The simplest way forward is to drop the casing fallback from this PR and open an issue proposing it. That also gets this PR back to one concern. Please also link an issue in the description and fill in the response comparison, since the 500 body changes from plain text to JSON.

• The new 500 text is "internal server error" with version: 2. Java returns the exception message with version: 1 (DC-10). That's the same as every other maglev handler, so it's fine here.