VOIP-1078-Migrate-call-manager-database-to-squirrel - #1172
Merged
pchero merged 12 commits intoAug 4, 2026
Conversation
- bin-call-manager: Add db struct tags to models/bridge Bridge (channel_ids as json, reference_id as uuid)
- bin-call-manager: Add db struct tags to models/channel Channel, with src_name/src_number/dst_name/dst_number columns deliberately divergent from their json tag names (plan R-B)
- bin-call-manager: Complete db struct tags on models/outboundconfig OutboundConfig (id/customer_id as uuid, destination_whitelist as json)
- bin-call-manager: Add models/bridge/field.go, models/channel/field.go, models/outboundconfig/field.go Field constants
- bin-call-manager: Document the per-field column/conversion tag table (plan §5 B-3) as doc comments on each model struct
Tag table (column name -> conversion type), plan §5 B-3:
bridge.Bridge (call_bridges)
AsteriskID -> asterisk_id (-), ID -> id (-), Name -> name (-)
Type -> type (-), Tech -> tech (-), Class -> class (-), Creator -> creator (-)
VideoMode -> video_mode (-), VideoSourceID -> video_source_id (-)
ChannelIDs -> channel_ids (json)
ReferenceType -> reference_type (-), ReferenceID -> reference_id (uuid)
TMCreate/TMUpdate/TMDelete -> tm_create/tm_update/tm_delete (-)
channel.Channel (call_channels)
ID -> id (-), AsteriskID -> asterisk_id (-), Name -> name (-), Type -> type (-), Tech -> tech (-)
SIPCallID -> sip_call_id (-), SIPTransport -> sip_transport (-), SIPData -> sip_data (json)
SourceName -> src_name (-), SourceNumber -> src_number (-)
DestinationName -> dst_name (-), DestinationNumber -> dst_number (-)
State -> state (-), Data -> data (json), StasisName -> stasis_name (-), StasisData -> stasis_data (json)
BridgeID -> bridge_id (-), PlaybackID -> playback_id (-)
DialResult -> dial_result (-), HangupCause -> hangup_cause (-)
Direction -> direction (-), MuteDirection -> mute_direction (-)
TMAnswer/TMRinging/TMEnd/TMCreate/TMUpdate/TMDelete -> same-named columns (-)
outboundconfig.OutboundConfig (call_outbound_configs)
ID -> id (uuid), CustomerID -> customer_id (uuid)
Name -> name (-), Detail -> detail (-)
DestinationWhitelist -> destination_whitelist (json), Codecs -> codecs (-)
DefaultOutgoingSourceNumberID -> default_outgoing_source_number_id (uuid)
TMCreate/TMUpdate/TMDelete -> tm_create/tm_update/tm_delete (-)
- bin-call-manager: Migrate BridgeAddChannelID to squirrel with squirrel.Expr for json_array_append (plan Phase 2 validation site) - bin-call-manager: Add bridgeTable var to dbhandler/bridge.go - bin-call-manager: Extract the statement into buildBridgeAddChannelID so the golden test asserts the real builder, not a copy - bin-call-manager: Add pkg/dbhandler/json_expr_golden_test.go, the pure ToSql() string-assertion harness mandated by plan §5 R1 for the JSON-function sites that SQLite cannot execute - bin-call-manager: Add the required WHY comment citing database.md §7.1 and the bin-agent-manager / bin-rag-manager precedents Phase 2 result: Expr in Set() position combined with PlaceholderFormat(Question) produces exactly the pre-migration statement, modulo whitespace, with argument order unchanged (channel_id, tm_update, id): UPDATE call_bridges SET channel_ids = json_array_append(channel_ids, '$', ?), tm_update = ? WHERE id = ? Pattern validated; Phases 3 onward may proceed.
- bin-call-manager: Add pkg/dbhandler/normalization.go documenting the R-A empty-collection-normalization table required by plan Phase 2.5
The table is keyed by (field x call site) rather than by field, because
outboundconfig.DestinationWhitelist normalizes at OutboundConfigCreate but not
at OutboundConfigUpdate; collapsing those rows would silently change one of
them. It records, per site, the current read-side and write-side behavior that
Phases 3, 5, 6 and 7 must preserve, plus which of the three nil-slice storage
outcomes ("[]", "null", SQL NULL) each conversion path produces.
Two consequences are called out explicitly for review:
- bridge.ChannelIDs at BridgeCreate changes stored representation from the JSON
literal "null" to SQL NULL (nil slice through struct-based PrepareFields with
a ,json tag). Not observable via any read path or the cache, since
bridgeGetFromRow normalizes both to []string{}.
- outbound_config's read path stops erroring on NULL/empty destination_whitelist
and yields nil instead, an intentional change tested in Phase 7.
- bin-call-manager: Migrate GroupcallDecreaseCallCount and GroupcallDecreaseGroupcallCount to squirrel, using squirrel.Expr for the atomic "<col> = <col> - 1" decrement - bin-call-manager: Route GroupcallSetCallIDsAndCallCountAndDialIndex and GroupcallSetGroupcallIDsAndGroupcallCountAndDialIndex through GroupcallUpdate - bin-call-manager: Route ConfbridgeSetFlags through ConfbridgeUpdate instead of raw SQL - bin-call-manager: Add normalizeSlice helper and apply it at the three write sites the R-A table marks "YES", preserving nil -> "[]" exactly - bin-call-manager: Add golden ToSql() cases for both Decrease sites - bin-call-manager: Add normalization_test.go asserting the raw stored column value for the three nil-normalization sites The Decrease functions deliberately do NOT route through GroupcallUpdate: that helper runs values through PrepareFields, which would reflect over the squirrel.Expr struct and JSON-marshal it instead of inlining it as SQL. They build their statement directly, with a WHY comment citing database.md §7.1 and the bin-rag-manager arithmetic-Expr precedent. Phase 3 is not low-risk, per the plan: all three setter sites normalize a nil slice today, and the map-based PrepareFields path would silently turn nil into the JSON literal "null". The new tests assert the raw column rather than the value read back, because the read path normalizes nil and would mask exactly that regression. Verified non-vacuous: dropping normalizeSlice makes Test_normalization_ConfbridgeSetFlags_nilStoresEmptyArray fail with "null". The pre-existing Test_ConfbridgeSetFlags covers only non-nil flags and was confirmed NOT to be a safety net for this behavior before relying on it.
- bin-call-manager: Migrate BridgeRemoveChannelID to squirrel with squirrel.Expr for the nested json_remove/replace/json_search delete-by-value expression - bin-call-manager: Add its golden ToSql() case, diffed 1:1 against the pre-migration raw SQL Phase 4, bridge.go (1 of the 14 remaining JSON sites). Generated SQL and arg order (channel_id, tm_update, id) are unchanged from the raw statement.
- bin-call-manager: Migrate all 7 call.go JSON sites to squirrel.Expr (CallAddChainedCallID, CallRemoveChainedCallID, CallAddExternalMediaID, CallRemoveExternalMediaID, CallAddRecordingIDs, CallTXAddChainedCallID, CallTXRemoveChainedCallID) - bin-call-manager: Add pkg/dbhandler/json_expr.go with the three recurring MySQL JSON expression shapes and the shared WHY rationale plus precedent citations - bin-call-manager: Refactor bridge.go's two sites onto the shared expression helpers - bin-call-manager: Add golden ToSql() cases for all 5 call.go builders The 12 remaining JSON sites across call.go and confbridge.go reduce to three SQL shapes (array append, unguarded delete-by-value, guarded delete-by-value), so they are factored into json_expr.go. That leaves three SQL strings to review rather than twelve near-identical ones that could silently drift apart, which matters because none of these sites is executable under SQLite. The TX and non-TX chained_call_ids pairs now share one builder each, so the two executors can no longer diverge. Each site keeps a WHY comment pointing at the shared rationale. Carried over deliberately, not fixed (plan §6 item 3): CallAddRecordingIDs binds recordID.String() while ConfbridgeAddRecordingIDs binds .Bytes(). Noted in a code comment at the call.go site; to be filed as a separate ticket.
- bin-call-manager: Migrate all 5 confbridge.go JSON sites to squirrel.Expr (ConfbridgeAddRecordingIDs, ConfbridgeAddExternalMediaID, ConfbridgeRemoveExternalMediaID, ConfbridgeAddChannelCallID, ConfbridgeRemoveChannelCallID) - bin-call-manager: Reuse the shared array-append and guarded delete-by-value expressions from json_expr.go - bin-call-manager: Keep json_insert and json_remove-by-path inline, since those two shapes occur only here - bin-call-manager: Add golden ToSql() cases for all 5 builders json_insert and json_remove-by-path continue to bind the JSON path as a query argument rather than interpolating it, matching the pre-migration statements. Carried over deliberately, not fixed (plan §6 item 3): ConfbridgeAddRecordingIDs binds recordingID.Bytes() while the semantically identical CallAddRecordingIDs and ConfbridgeAddExternalMediaID bind .String(). Since recording_ids unmarshals as []uuid.UUID, .Bytes() likely stores a malformed element. Documented at the site and pinned by its golden test so the current behavior is explicit; needs its own ticket plus a data backfill/integrity check for already-written rows.
- bin-call-manager: Migrate ChannelSetDataItem to squirrel.Expr for json_set - bin-call-manager: Fix the SQL injection in ChannelSetDataItem by binding the JSON path as a query argument instead of interpolating the key with fmt.Sprintf - bin-call-manager: Add channelTable var to dbhandler/channel.go - bin-call-manager: Add golden ToSql() cases, including one asserting a hostile key stays a bound argument and does not alter the SQL text The injected key was not a trusted constant: pkg/arieventhandler/ari_channel.go:209 passes the ARI ChannelVarset event's variable name straight through to this function, so the interpolation was reachable from event input. The path is also now quoted as $."key" rather than bare $.key, matching ConfbridgeAddChannelCallID. Equivalent for ordinary identifier keys; they differ only for a key containing '.', where the bare form silently addressed a nested path instead of the literal key. This completes all 15 JSON-function sites from plan §1 item 3 (bridge 2, call 7, confbridge 5, channel 1), each with a WHY comment and a golden ToSql() assertion.
- bin-call-manager: Convert the remaining non-JSON bridge.go functions to squirrel (BridgeCreate, bridgeGetFromDB, BridgeEnd)
- bin-call-manager: Replace the hand-written bridgeGetFromRow scan with commondatabasehandler.ScanRow
- bin-call-manager: Delete the bridgeSelect const and the manual column-by-column scan plus its inline timestamp parsing
- bin-call-manager: Drop the now-unused encoding/json and utilhandler imports from bridge.go
BridgeCreate now assigns b.TMCreate/TMUpdate/TMDelete before PrepareFields.
Struct-based PrepareFields emits every db-tagged field unconditionally, so
without the explicit assignment tm_create would be written as SQL NULL. This
matches the existing in-service pattern in recording.go, groupcall.go and
confbridge.go, and makes the passed-in struct consistent with the row written.
Per the R-A read table, bridgeGetFromRow keeps normalizing ChannelIDs nil -> [],
because ScanRow's copyJSON leaves the field nil on SQL NULL or empty.
Per R-A note (a), a nil ChannelIDs at BridgeCreate now stores SQL NULL where the
pre-migration json.Marshal stored the literal "null". Not observable through any
read path or the cache, since both normalize to []string{} on read.
The existing bridge tests exercise the ,uuid read path for reference_id against
the binary(16) SQLite fixture and pass unchanged.
- bin-call-manager: Convert channel.go to squirrel (ChannelCreate, channelGetFromDB, ChannelList, ChannelGetsForRecovery)
- bin-call-manager: Replace the hand-written channelGetFromRow scan with commondatabasehandler.ScanRow
- bin-call-manager: Add a channelUpdate helper and collapse the 14 single-column setters onto it
- bin-call-manager: Fix the SQL injection in ChannelList by applying filters through commondatabasehandler.ApplyFields instead of interpolating the filter key
- bin-call-manager: Change ChannelList's filter type from map[string]string to map[channel.Field]any, updating the DBHandler interface and the generated mock
- bin-call-manager: Delete the channelSelect const and the manual column-by-column scan with its inline timestamp parsing
R-A normalization is applied field-by-field, not as a blanket rule:
channelGetFromRow still normalizes Data and StasisData nil -> {}, while SIPData
is deliberately left nil, matching the pre-migration read behavior exactly. No
write site in this file normalizes, so nil maps keep marshaling to the JSON
literal "null" as before.
The ChannelList filter change also required the "deleted" filter value to become
a Go bool rather than the string "false" (plan §6 item 4): ApplyFields switches
on the value type, and a string would have fallen through to a comparison
against a literal, non-existent "deleted" column instead of being translated to
tm_delete IS NULL / IS NOT NULL. ApplyFields's translation matches this table's
existing semantics; it would be wrong for a 9999-01-01 sentinel table, which is
why the two soft-delete conventions are left un-unified (plan §6 item 6).
Also per plan §6 item 7, LIMIT is now passed to squirrel's Limit() as a uint64
instead of being pre-formatted with strconv.FormatUint. Intentional
simplification, not a behavior change.
ChannelCreate assigns all six timestamp fields before PrepareFields, since the
struct path emits every db-tagged field unconditionally.
The regenerated mock is reduced to the single required ChannelList signature
line; unrelated parameter-renaming churn from mockgen version drift is excluded.
- bin-call-manager: Convert outbound_config.go to squirrel across Create, Delete, GetByID, GetByCustomerID, Update and List - bin-call-manager: Replace outboundConfigGetFromRow's hand-written scan with commondatabasehandler.ScanRow, deleting parseMySQLDateTime and ~40 lines of inline MySQL DATETIME parsing - bin-call-manager: Switch OutboundConfigCreate, OutboundConfigUpdate and OutboundConfigDelete from inline time.Now() to h.utilHandler.TimeNow() - bin-call-manager: Add scripts/database_scripts_test/table_outbound_configs.sql, a new SQLite fixture with genuine binary(16) UUID columns - bin-call-manager: Add pkg/dbhandler/outbound_config_test.go, the first tests this file has ever had (16 cases) - bin-call-manager: Factor the two single-row SELECTs onto a shared outboundConfigGetBy helper OutboundConfigUpdate uses map-based PrepareFields exclusively (plan §5 R-C). Struct-based PrepareFields emits every db-tagged field unconditionally with no nil-skip branch, so it cannot express UpdateRequest's 3-state semantics and would overwrite every untouched column with SQL NULL on every update. DefaultOutgoingSourceNumberID is dereferenced to a value-typed uuid.UUID: processMapValues does not match *uuid.UUID, which would reach the driver and be rendered as a 36-char string into a BINARY(16) column. OutboundConfigCreate keeps struct-based PrepareFields (full-row INSERT) but assigns TMCreate and TMUpdate to now explicitly, deliberately NOT copying recording.go's TMUpdate=nil pattern, since outbound_config sets both. Switching all three functions to the injectable clock is what makes exact-match timestamp assertions possible; previously only non-nil could be asserted. Per plan v12, the Delete case asserts tm_delete with a DIRECT SQL query, because both GetByID and List filter tm_delete IS NULL and a soft-deleted row is unreachable through every dbhandler read method. The test also asserts that unreachability explicitly. Test coverage added for the specific risks: - R-C: id, customer_id and default_outgoing_source_number_id are asserted to be stored as exactly 16 bytes, at both Create and Update. Verified non-vacuous: removing the ,uuid tag makes the assertion fail with the 36-char form. - R-C: a dedicated test pins the pointer-UUID corruption mechanism, asserting that a *uuid.UUID passes through PrepareFields unconverted and that its driver.Valuer output is the 36-char string that overflows BINARY(16) — silent truncation under permissive SQL mode, a write error under strict mode. - R-A/B-2: three-way NULL / empty / non-empty coverage for destination_whitelist at BOTH call sites, pinning that Create normalizes nil to "[]" while Update writes "null" — the two different behaviors the R-A table records. - R-A/B-2: the intentional read-side change is covered — a NULL destination_whitelist used to be a scan error and now reads back as nil. Create's nil normalization is load-bearing rather than cosmetic: destination_whitelist is JSON NOT NULL in MySQL, and struct-based PrepareFields converts a nil slice to SQL NULL, which would violate the constraint.
- bin-call-manager: Rewrite the CLAUDE.md Database Pattern section, which claimed the service uses direct SQL with no Squirrel query builder
- monorepo docs: Remove the stale "Exception — bin-call-manager" note at docs/conventions/database.md §7.0
- monorepo docs: Correct the §7.0a line that described raw SQL paths as "the exception in bin-call-manager"
- monorepo docs: Extend the §7.1 exception list to name MySQL JSON functions explicitly alongside computed/arithmetic expressions
All three locations previously described bin-call-manager as a raw-SQL exception
and are now accurate. The §7.1 exception list previously named only arithmetic
("cost_per_unit * ?"); it now names json_array_append, json_insert, json_set,
json_remove, json_search, JSON_CONTAINS and JSON_EXTRACT, since this migration
introduces the monorepo's largest concentration of JSON-function Expr sites and
the convention should name the pattern it sanctions rather than leave it implied
by cross-service precedent. It also records that SQLite cannot execute these, so
they must be pinned with golden ToSql() assertions.
The CLAUDE.md rewrite documents the struct-vs-map PrepareFields split, the
dereferenced-uuid.UUID requirement, and points at normalization.go's
(field x call site) empty-collection table.
Per plan §6 item 6, the two co-existing soft-delete conventions (tm_delete IS
NULL vs the 9999-01-01 sentinel) are documented as-is rather than unified;
unifying them touches query predicates across every caller and belongs in its
own ticket.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migrate bin-call-manager's pkg/dbhandler from hand-written raw SQL to the Squirrel query builder, bringing it in line with the rest of the monorepo's database convention. This closes a long-standing squirrel-exception gap (call-manager was the only service still on raw SQL) and fixes two SQL injection sites discovered along the way. Implemented per a 13-round-reviewed design plan (docs/plans/2026-08-04-voip-1078-call-manager-squirrel-migration-plan.md, kept in this PR for reviewer reference).
R1 (MySQL integration-test coverage) note: a testcontainers/dockertest-based MySQL integration suite for the 15 untestable JSON sites was evaluated and declined — no precedent for MySQL integration tests in this service and no established CI budget for one; mitigated instead by golden ToSql() string-assertion tests diffed 1:1 against the pre-migration raw SQL for every site, verified by three independent review passes (2 general + 1 security-focused).