refactor: extract withTransaction helper for GTFS bulk imports

[tejasva-vardhan]

Summary

This PR extracts a centralized withTransaction helper for GTFS bulk operations to eliminate boilerplate and ensure consistent transaction handling across the import process. This is a follow-up to the review suggestions in #653.

Changes

• Added Helper: Introduced Client.withTransaction in helpers.go. This helper handles transaction checking, context-aware creation, deferred rollbacks, and commits.
• Refactored Bulk Operations: Wrapped the core DB-write logic of the following 7 functions in the new helper:
  • bulkInsertStops
  • bulkInsertTrips
  • bulkInsertStopTimes
  • bulkInsertShapes
  • bulkInsertFrequencies
  • bulkInsertCalendarDates
  • buildBlockTripIndex

Impact

• Removes ~70 lines of repetitive transaction boilerplate.
• Standardizes fallback transaction creation to use the context-aware c.DB.BeginTx(ctx, nil), ensuring context cancellation is properly respected across all bulk inserts.
• Centralizes resource management logic to prevent accidental missed commits or rollbacks.

Checklist

• Extracted withTransaction helper
• Refactored all 7 bulk import functions
• Standardized on context-aware BeginTx(ctx, nil)
• Tests and build verified locally (make test)
• Code formatted and linted (make lint)

[tejasva-vardhan]

Hi @aaronbrethorst , I noticed the OpenAPI conformance failures. It seems the schema in openapi.yml is out of sync with the current models. I'll wait for #700 to be merged, then I'll merge main into this branch to fix the test failures. My transaction helper changes are ready to go otherwise.

[aaronbrethorst]

Hey Tejasva, the withTransaction helper is a well-crafted abstraction — it correctly handles both the caller-owned and self-managed transaction cases, and the mechanical refactoring across all 7 functions is faithful. Nice work eliminating that boilerplate.

A few things to address before this can merge.

Critical Issues

1. The Stops field addition needs its own PR

This PR bundles two unrelated changes: the transaction helper refactor (mechanical, low-risk) and the addition of a Stops []string field to ScheduleForRouteEntry + handler.

These should be separate PRs for a couple of reasons:

• They have different risk profiles and review concerns
• The Stops field work deserves its own discussion — PR #700 just removed stops and trips from the OpenAPI spec to align it with the current code. This PR is now re-adding stops (as []string instead of []Stop), which puts the response back out of alignment with the spec. If the intent is to add stops support to the endpoint, that should go through as a deliberate feature PR with the spec updated to match.

Recommendation: Remove the Stops changes from this PR (revert the model and handler changes) and submit the transaction refactor on its own. If you want to add stops to schedule-for-route, open a follow-up PR that also updates the OpenAPI spec.

Important Issues

2. db.Begin() → db.BeginTx(ctx, nil) is a behavioral change

Six of the seven refactored functions previously used db.Begin() (context-unaware). The new withTransaction uses c.DB.BeginTx(ctx, nil), which will cancel the transaction if the context is cancelled. For long-running bulk imports (shapes can be 10k+ rows), a context cancellation could now abort work that previously would have completed.

This is likely an improvement, and your PR description mentions it, but it should be called out explicitly as a deliberate behavioral change, not just a standardization note.

3. Wrap errors in withTransaction for debuggability

Since you already have the label parameter, wrap the BeginTx and Commit errors:

newTx, err := c.DB.BeginTx(ctx, nil)
if err != nil {
    return fmt.Errorf("failed to begin transaction for %s: %w", label, err)
}
// ...
if err := newTx.Commit(); err != nil {
    return fmt.Errorf("failed to commit transaction for %s: %w", label, err)
}
return nil

Small change, big payoff when debugging import failures across 7 callers.

4. bulkInsertCalendarDates lost its logging

Every other refactored function retains LogOperation calls for "inserting_X" and "X_inserted". bulkInsertCalendarDates had its logger removed entirely — it's now the only bulk insert function with zero operational logging. Add the start/end log messages for consistency.

Suggestions

5. Document the tx != nil contract

The godoc says "does not commit" for non-nil tx, but doesn't mention rollback responsibility. Add: "When tx is non-nil, the caller is responsible for committing or rolling back the transaction on error."

Strengths

• The withTransaction helper is well-designed — the nil/non-nil tx branching is clean and correct
• defer SafeRollbackWithLogging correctly handles the post-commit case (it checks for "already committed" and silently returns)
• The mechanical refactoring is faithful — insert logic within each function is preserved exactly
• Good PR description with clear impact summary

Recommended Action: Request Changes

1. Remove the Stops field changes — submit as a separate PR with OpenAPI spec alignment
2. Wrap errors in withTransaction (BeginTx + Commit) using the label param
3. Restore logging to bulkInsertCalendarDates
4. Document the tx != nil rollback contract in the godoc
5. Rebase onto main (to pick up #700) and confirm make test + make lint pass