fix: repair spock-diff against spock-enabled clusters#125
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe PR extends the spock-diff feature by propagating subscription origin node names from the database ( ChangesProvider-based Spock subscription reconciliation
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 14 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/consistency/diff/spock_diff_test.go (1)
62-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the mismatch assertion to validate diff payload, not just count.
assert.Len(t, d.Different, 1)can still pass if the wrong pair/details are emitted. Please also assert key fields ond.Different[0](e.g.,Node1.ProviderNode == "n2",Node2.ProviderNode == "n1", and expected replication-set contents) to lock the contract.Suggested assertion hardening
d := compareSubscriptions(n1, n2) assert.Empty(t, d.MissingOnNode1) assert.Empty(t, d.MissingOnNode2) assert.Len(t, d.Different, 1, "replication set difference should be reported") +assert.Equal(t, "n2", d.Different[0].Node1.ProviderNode) +assert.Equal(t, "n1", d.Different[0].Node2.ProviderNode) +assert.ElementsMatch(t, []string{"default"}, d.Different[0].Node1.ReplicationSets) +assert.ElementsMatch(t, []string{"default", "extra"}, d.Different[0].Node2.ReplicationSets)🤖 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 `@internal/consistency/diff/spock_diff_test.go` around lines 62 - 77, The test function TestCompareSubscriptions_DifferentReplicationSets currently only asserts that exactly one difference exists using assert.Len on d.Different, but this does not validate what the actual difference contains. Strengthen the assertion by adding additional checks on the d.Different[0] element to verify the actual payload, specifically asserting that Node1.ProviderNode equals "n2", Node2.ProviderNode equals "n1", and the replication set contents match expectations. This ensures the test validates the correct difference is being reported, not just that one difference exists.
🤖 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.
Inline comments:
In `@db/queries/templates.go`:
- Around line 612-616: The query selects o.node_name aliased as sub_origin_name
from a LEFT OUTER JOIN on spock.node o, but only filters on s.sub_name IS NOT
NULL, leaving s.sub_origin unfiltered. This means o.node_name can be NULL when
s.sub_origin doesn't exist or match a row, but the SubOriginName field in
types.go (line 323) is defined as a non-nullable string, causing rows.Scan at
line 871 to fail. Either wrap o.node_name with COALESCE in the SELECT clause to
provide a sentinel value (and verify downstream code can handle it), or change
the SubOriginName field definition from string to *string to allow NULL values.
First verify whether s.sub_origin is guaranteed to always resolve to a valid
node_id.
In `@internal/consistency/diff/spock_diff.go`:
- Around line 544-553: The subscriptionsByProvider function silently skips any
subscriptions where ProviderNode is empty, which causes compareSubscriptions to
incorrectly report those subscriptions as missing on a node when they actually
exist. Modify subscriptionsByProvider to handle empty ProviderNode values
explicitly by either including them with a placeholder or empty string key in
the map, or by adding logging/error handling to surface the issue instead of
silently filtering them out. This ensures that all subscriptions are tracked in
the index regardless of whether ProviderNode is populated, preventing false
positives in the subscription comparison logic.
In `@tests/integration/spock_diff_test.go`:
- Around line 53-58: In the cleanup function registered with t.Cleanup, you are
silently ignoring errors from filepath.Glob and os.Remove which prevents
visibility into cleanup failures. Modify the cleanup block to check the error
returned by filepath.Glob and log a warning using t.Logf if it occurs, and
similarly check the error returned by os.Remove for each file and log a warning
if removal fails. Use the pattern t.Logf("Warning: ...") to log these failures
so they are captured in test output while keeping the cleanup best-effort.
---
Nitpick comments:
In `@internal/consistency/diff/spock_diff_test.go`:
- Around line 62-77: The test function
TestCompareSubscriptions_DifferentReplicationSets currently only asserts that
exactly one difference exists using assert.Len on d.Different, but this does not
validate what the actual difference contains. Strengthen the assertion by adding
additional checks on the d.Different[0] element to verify the actual payload,
specifically asserting that Node1.ProviderNode equals "n2", Node2.ProviderNode
equals "n1", and the replication set contents match expectations. This ensures
the test validates the correct difference is being reported, not just that one
difference exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb5943fc-60d0-4dad-8573-cab9231e43c5
📒 Files selected for processing (7)
.github/workflows/test.ymldb/queries/queries.godb/queries/templates.gointernal/consistency/diff/spock_diff.gointernal/consistency/diff/spock_diff_test.gopkg/types/types.gotests/integration/spock_diff_test.go
mason-sharp
force-pushed
the
fix/ACE-195/spock-diff
branch
from
270d3a4 to
fd50551
Compare
| if ni.SubOriginName == "" { | ||
| hint := fmt.Sprintf("Subscription '%s' has an unresolved origin node and is excluded from comparison.", sub.SubName) | ||
| if !utils.Contains(config.Hints, hint) { | ||
| config.Hints = append(config.Hints, hint) | ||
| } | ||
| } |
There was a problem hiding this comment.
When ni.SubOriginName == "" the code emits a hint saying the subscription is "excluded from comparison" and sets sub.ProviderNode = "". However, config.Subscriptions = append(config.Subscriptions, sub) still runs unconditionally (it's outside the if ni.SubName != "" block). subscriptionsByProvider correctly skips entries with empty ProviderNode, so this node's lookup for the peer returns false, and MissingOnNode1 gets the peer name — a false-positive mismatch. The hint and the diff report are inconsistent: the user is told the subscription is excluded, then immediately shown a mismatch caused by its exclusion. Fix: add continue (skip the append) when SubOriginName == "".
There was a problem hiding this comment.
SubscriptionsByProvider already skips empty-ProviderNode entries, so
MissingOnNode1 still gets the peer name regardless of the append; it'd only drop the subscription from
the printed config (and if it's the node's only sub, the config would then print "No subscriptions
found" while the hint names it).
With an unresolved origin we can't tell which peer the subscription belongs to, so the "missing" entry
is unavoidable. I reworded the hint to match the diff instead:
▎ Subscription '' has an unresolved origin node; its reciprocal peer cannot be determined and it
▎ may be reported below as a missing subscription.
| sort.Strings(s1.ReplicationSets) | ||
| sort.Strings(s2.ReplicationSets) |
There was a problem hiding this comment.
This is a cosmetic side-effect worth a one-liner fix sets1 := append([]string(nil), s1.ReplicationSets...); sort.Strings(sets1) before sorting. The sort mutates in place. Pretty-printing runs before the comparison loop so the console output is unaffected, and in-scope comparisons use unique per-pair subscriptions so no wrong mismatch is produced. But the SpockConfigs section of the written JSON file will show replication sets in sorted order rather than the database-returned order.
Three distinct bugs prevented spock-diff from working on a spock cluster: - GetSpockNodeAndSubInfo scanned the oid columns spock.node.node_id and spock.subscription.sub_id into int64, failing with "cannot scan oid (OID 26) in binary format into *int64". Cast both to bigint. - GetSpockRepSetInfo scanned NULL set_name (tables in no replication set) into a non-nullable string. Filter NULLs in the query. - compareSubscriptions matched reciprocal subscriptions by reconstructing sub_<a><b> names, which breaks for user-renamed subscriptions and had the direction backwards. Resolve each subscription's origin node (sub_origin) and match reciprocal pairs by node identity instead. Add unit tests for the comparison logic and integration tests that run the full command against the spock cluster, and wire both into CI.
When a subscription's origin node cannot be resolved, the hint previously claimed it was "excluded from comparison", but the exclusion is exactly what causes it to be reported as a missing subscription in the diff below. Reword the hint to state that its reciprocal peer cannot be determined and that it may surface as missing, so the hint and diff tell one story.
compareSubscriptions sorted s1/s2.ReplicationSets in place to compare them order-insensitively, but those slices are shared with the SpockConfigs section of the written JSON, leaking sorted order in place of the database-returned order. Sort copies instead, and cover it with a test.
mason-sharp
force-pushed
the
fix/ACE-195/spock-diff
branch
from
fd50551 to
e4a08cb
Compare
2 participants
Three distinct bugs prevented spock-diff from working on a spock cluster:
Add unit tests for the comparison logic and integration tests that run the full command against the spock cluster, and wire both into CI.