Enhance YSON String Representation#1252
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant clusterServer
participant packs
participant backend
participant database
Client->>clusterServer: CompactDocument(request)
clusterServer->>packs: Compact(ctx, backend, projectID, docInfo)
packs->>backend: BuildInternalDocForServerSeq(...)
backend-->>packs: Document state
packs->>database: CompactChangeInfos(...)
database-->>packs: Result
packs-->>clusterServer: Error or nil
clusterServer-->>Client: Response
Possibly related PRs
Suggested labels
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
server/packs/packs.go (3)
33-38: Avoid shadowing the Go std-libencoding/jsonpackageImporting Yorkie’s internal JSON package as plain
jsoncan mislead readers into thinking the standard library is being used.
Consider aliasing to make the intent explicit:- "github.com/yorkie-team/yorkie/pkg/document/json" + yorkiejson "github.com/yorkie-team/yorkie/pkg/document/json"and update the references (
newDoc.Update(func(r *yorkiejson.Object, …) accordingly.
This tiny rename improves readability and reduces cognitive load.
331-347: Bubble up a typed error for “document attached”
Compactreturnsfmt.Errorf("document is attached"), which makes upstream callers rely on string matching to detect this condition.
Define and reuse a sentinel error, e.g.:var ErrDocumentAttached = errors.New("document is attached")and return
ErrDocumentAttachedhere.
This enables clean error handling (errors.Is(err, packs.ErrDocumentAttached)).
373-376: String-based deep-equality check can be expensive
root.(yson.Object).Marshal()allocates an entire JSON-like representation just to compare equality.
For large documents this is O(n) memory. Consider:if !reflect.DeepEqual(root, newRoot) { … }or provide an
Equalmethod on the YSON object to compare in-place without stringification.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/backend/database/memory/database.go(2 hunks)server/packs/packs.go(2 hunks)server/rpc/cluster_server.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
server/packs/packs.go (5)
server/backend/backend.go (2)
Backend(48-77)New(80-205)server/documents/documents.go (1)
IsDocumentAttached(299-306)api/types/resource_ref_key.go (1)
DocRefKey(24-27)server/logging/logging.go (2)
DefaultLogger(85-90)New(63-76)pkg/document/yson/to_yson.go (1)
FromCRDT(110-127)
server/rpc/cluster_server.go (1)
server/packs/packs.go (1)
Compact(330-391)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: bench
- GitHub Check: build
- GitHub Check: complex-test
🔇 Additional comments (3)
server/backend/database/memory/database.go (1)
1130-1195: Comment-only change – no functional impactThe renumbering of the “step” comments is purely cosmetic; the underlying queries, deletes, and inserts remain unchanged.
No action needed.server/packs/packs.go (1)
377-385: Ensure exactly one change is generated
CompactChangeInfosonly accepts0or1change(s).
newDoc.CreateChangePack()will generate one change with the full snapshot in normal cases, but future modifications toUpdateor presence handling could silently produce >1, triggering “invalid number of changes” at runtime.Add a safety assertion before calling the DB:
if n := len(changes); n != 1 { return fmt.Errorf("compaction expected 1 change, got %d", n) }or relax the constraint in
CompactChangeInfos.server/rpc/cluster_server.go (1)
167-169: Refactor looks great – single-responsibility preservedDelegating the heavy-weight compaction logic to
packs.Compacttrims ~150 lines from this RPC handler and keeps the server surface slim.
Nice separation of concerns!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1252 +/- ##
==========================================
- Coverage 37.63% 37.43% -0.20%
==========================================
Files 179 179
Lines 28242 28176 -66
==========================================
- Hits 10629 10549 -80
- Misses 16734 16753 +19
+ Partials 879 874 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Go Benchmark Analysis 📊
This is a comparison result between the previous(7df49d3) and the current commit(6dc8f00).
Significant Changes (≥20% difference)
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| BenchmarkVersionVector/clients_1000/ (7_pushpull_after_detach(ms)) | 17.00 ms | 23.00 ms | 🔴 +35.29% |
Key Observations 🔍
- The benchmark suite
BenchmarkVersionVectorshows a significant increase of +35.29% in theclients_1000test for7_pushpull_after_detach(ms). This indicates a notable performance degradation in push-pull operations for a large number of clients. - The benchmark suite
BenchmarkWebhookWithLimitdemonstrates a slight performance improvement across different scenarios, with reductions ranging from 0.44% to 1.00% in response times for varying numbers of webhooks and endpoints with limits.
Overall, the majority of benchmarks display minor fluctuations, with some showcasing incremental improvements or deteriorations in performance metrics.
Clock Analysis
Lamport vs VersionVector
BenchmarkVersionVector Test
Lamport (v0.5.2)
| Metric | 10 clients | 100 clients | 1000 clients |
|---|---|---|---|
| Total Operation Time | 84.96 ms | 793.94 ms | 34.79 s |
| Memory Allocations | 35.11 MB | 219.92 MB | 4.91 GB |
| Number of Allocations (allocs/op) | 69,271 | 1,246,728 | 81,485,288 |
| ChangePack Size | 138.0 B | 137.0 B | 141.0 B |
| Snapshot Size | 379.0 B | 3.08 KB | 30.08 KB |
| Push-Pull Time | 2.0 ms | 1.5 ms | 4.0 ms |
| Attach Time | 4.5 ms | 11.0 ms | 31.0 ms |
| ChangePack After Detach | 138.0 B | 140.0 B | 141.0 B |
| Snapshot After Detach | 136.0 B | 137.0 B | 139.0 B |
| Push-Pull After Detach | 2.5 ms | 5.0 ms | 9.5 ms |
Version Vector (current)
| Metric | 10 clients | 100 clients | 1000 clients |
|---|---|---|---|
| Total Operation Time | 130.80 ms | 1.13 s | 20.29 s |
| Memory Allocations | 17.52 MB | 178.63 MB | 3.64 GB |
| Number of Allocations (allocs/op) | 64,306 | 879,559 | 43,196,913 |
| ChangePack Size | 745.00 B | 6.14 KB | 60.16 KB |
| Snapshot Size | 399.00 B | 3.10 KB | 30.10 KB |
| Push-Pull Time | 5.00 ms | 6.00 ms | 16.00 ms |
| Attach Time | 5.00 ms | 8.00 ms | 64.00 ms |
| ChangePack After Detach | 805.00 B | 6.21 KB | 60.22 KB |
| Snapshot After Detach | 156.00 B | 157.00 B | 161.00 B |
| Push-Pull After Detach | 5.00 ms | 6.00 ms | 23.00 ms |
BenchmarkSyncConcurrency Test
| Metric | Clients | Lamport (v0.5.2) | Version Vector (current) |
|---|---|---|---|
| Total Operation Time | 1-100-10-10 | 7.48 s | 15.55 s |
| 100-100-10-10 | 9.62 s | 28.12 s | |
| 300_100-10-10 | 14.77 s | 54.64 s | |
| Memory Allocations | 1-100-10-10 | 1.15 GB | 8.41 GB |
| 100-100-10-10 | 1.45 GB | 16.80 GB | |
| 300_100-10-10 | 2.19 GB | 33.23 GB | |
| Number of Allocations (allocs/op) | 1-100-10-10 | 17,107,766 | 176,494,978 |
| 100-100-10-10 | 20,084,480 | 338,119,643 | |
| 300_100-10-10 | 30,359,215 | 660,518,775 |
Summary
- The Version Vector clock system shows improved performance compared to Lamport across all client scales. It demonstrates lower total operation times, memory allocations, and number of allocations per operation.
- The Version Vector clock system also exhibits more stable behavior in the concurrent synchronization tests, showcasing consistent performance metrics across varying levels of client concurrency.
Detailed Test Results
BenchmarkDeletionConcurrency
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| concurrent_text_delete_range_100/ (ns/op) | 865.26 ms | 860.91 ms | 🟢 -0.50% |
| concurrent_text_delete_range_100/ (B/op) | 73.54 MB | 72.49 MB | 🟢 -1.43% |
| concurrent_text_delete_range_100/ (allocs/op) | 789,075 allocs | 788,998 allocs | ⚪ 0% |
| concurrent_text_delete_range_1000/ (ns/op) | 7.17 s | 7.02 s | 🟢 -2.13% |
| concurrent_text_delete_range_1000/ (B/op) | 394.73 MB | 392.89 MB | 🟢 -0.47% |
| concurrent_text_delete_range_1000/ (allocs/op) | 7,486,405 allocs | 7,487,335 allocs | 🔴 +0.01% |
| concurrent_tree_delete_range_100/ (ns/op) | 882.28 ms | 868.77 ms | 🟢 -1.53% |
| concurrent_tree_delete_range_100/ (B/op) | 79.84 MB | 79.62 MB | 🟢 -0.28% |
| concurrent_tree_delete_range_100/ (allocs/op) | 826,342 allocs | 826,258 allocs | 🟢 -0.01% |
| concurrent_tree_delete_range_1000/ (ns/op) | 7.80 s | 7.64 s | 🟢 -2.08% |
| concurrent_tree_delete_range_1000/ (B/op) | 919.61 MB | 914.26 MB | 🟢 -0.58% |
| concurrent_tree_delete_range_1000/ (allocs/op) | 7,793,038 allocs | 7,792,356 allocs | ⚪ 0% |
| concurrent_text_edit_delete_all_100/ (ns/op) | 758.42 ms | 746.56 ms | 🟢 -1.56% |
| concurrent_text_edit_delete_all_100/ (B/op) | 67.11 MB | 66.63 MB | 🟢 -0.72% |
| concurrent_text_edit_delete_all_100/ (allocs/op) | 658,363 allocs | 658,247 allocs | 🟢 -0.02% |
| concurrent_text_edit_delete_all_1000/ (ns/op) | 5.91 s | 5.84 s | 🟢 -1.32% |
| concurrent_text_edit_delete_all_1000/ (B/op) | 330.84 MB | 328.30 MB | 🟢 -0.77% |
| concurrent_text_edit_delete_all_1000/ (allocs/op) | 5,996,746 allocs | 5,997,051 allocs | ⚪ 0% |
| concurrent_tree_edit_delete_all_100/ (ns/op) | 768.40 ms | 761.88 ms | 🟢 -0.85% |
| concurrent_tree_edit_delete_all_100/ (B/op) | 69.11 MB | 68.41 MB | 🟢 -1.00% |
| concurrent_tree_edit_delete_all_100/ (allocs/op) | 698,982 allocs | 699,049 allocs | ⚪ 0% |
| concurrent_tree_edit_delete_all_1000/ (ns/op) | 6.48 s | 6.39 s | 🟢 -1.38% |
| concurrent_tree_edit_delete_all_1000/ (B/op) | 728.78 MB | 725.20 MB | 🟢 -0.49% |
| concurrent_tree_edit_delete_all_1000/ (allocs/op) | 6,402,636 allocs | 6,401,999 allocs | ⚪ 0% |
BenchmarkDocument
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| constructor_test/ (ns/op) | 1599.00 ns | 1486.00 ns | 🟢 -7.07% |
| constructor_test/ (B/op) | 1.45 KB | 1.45 KB | ⚪ 0% |
| constructor_test/ (allocs/op) | 25 allocs | 25 allocs | ⚪ 0% |
| status_test/ (ns/op) | 1158.00 ns | 1092.00 ns | 🟢 -5.70% |
| status_test/ (B/op) | 1.42 KB | 1.42 KB | ⚪ 0% |
| status_test/ (allocs/op) | 23 allocs | 23 allocs | ⚪ 0% |
| equals_test/ (ns/op) | 8726.00 ns | 8140.00 ns | 🟢 -6.72% |
| equals_test/ (B/op) | 7.84 KB | 7.84 KB | ⚪ 0% |
| equals_test/ (allocs/op) | 135 allocs | 135 allocs | ⚪ 0% |
| nested_update_test/ (ns/op) | 18771.00 ns | 17648.00 ns | 🟢 -5.98% |
| nested_update_test/ (B/op) | 12.66 KB | 12.66 KB | ⚪ 0% |
| nested_update_test/ (allocs/op) | 267 allocs | 267 allocs | ⚪ 0% |
| delete_test/ (ns/op) | 25292.00 ns | 23848.00 ns | 🟢 -5.71% |
| delete_test/ (B/op) | 16.20 KB | 16.20 KB | ⚪ 0% |
| delete_test/ (allocs/op) | 350 allocs | 350 allocs | ⚪ 0% |
| object_test/ (ns/op) | 9533.00 ns | 8996.00 ns | 🟢 -5.63% |
| object_test/ (B/op) | 7.22 KB | 7.22 KB | ⚪ 0% |
| object_test/ (allocs/op) | 123 allocs | 123 allocs | ⚪ 0% |
| array_test/ (ns/op) | 30212.00 ns | 29807.00 ns | 🟢 -1.34% |
| array_test/ (B/op) | 12.51 KB | 12.51 KB | ⚪ 0% |
| array_test/ (allocs/op) | 282 allocs | 282 allocs | ⚪ 0% |
| text_test/ (ns/op) | 36223.00 ns | 33585.00 ns | 🟢 -7.28% |
| text_test/ (B/op) | 15.97 KB | 15.97 KB | ⚪ 0% |
| text_test/ (allocs/op) | 513 allocs | 513 allocs | ⚪ 0% |
| text_composition_test/ (ns/op) | 35332.00 ns | 33582.00 ns | 🟢 -4.95% |
| text_composition_test/ (B/op) | 19.24 KB | 19.24 KB | ⚪ 0% |
| text_composition_test/ (allocs/op) | 517 allocs | 517 allocs | ⚪ 0% |
| rich_text_test/ (ns/op) | 95847.00 ns | 90198.00 ns | 🟢 -5.89% |
| rich_text_test/ (B/op) | 40.96 KB | 40.97 KB | ⚪ 0% |
| rich_text_test/ (allocs/op) | 1,207 allocs | 1,207 allocs | ⚪ 0% |
| counter_test/ (ns/op) | 20161.00 ns | 18993.00 ns | 🟢 -5.79% |
| counter_test/ (B/op) | 12.37 KB | 12.37 KB | ⚪ 0% |
| counter_test/ (allocs/op) | 264 allocs | 264 allocs | ⚪ 0% |
| text_edit_gc_100/ (ns/op) | 1.55 ms | 1.50 ms | 🟢 -3.60% |
| text_edit_gc_100/ (B/op) | 875.53 KB | 875.47 KB | ⚪ 0% |
| text_edit_gc_100/ (allocs/op) | 17,588 allocs | 17,587 allocs | ⚪ 0% |
| text_edit_gc_1000/ (ns/op) | 59.20 ms | 57.31 ms | 🟢 -3.19% |
| text_edit_gc_1000/ (B/op) | 46.94 MB | 46.94 MB | ⚪ 0% |
| text_edit_gc_1000/ (allocs/op) | 188,596 allocs | 188,602 allocs | ⚪ 0% |
| text_split_gc_100/ (ns/op) | 2.30 ms | 2.26 ms | 🟢 -1.96% |
| text_split_gc_100/ (B/op) | 1.58 MB | 1.58 MB | ⚪ 0% |
| text_split_gc_100/ (allocs/op) | 15,959 allocs | 15,959 allocs | ⚪ 0% |
| text_split_gc_1000/ (ns/op) | 140.30 ms | 135.52 ms | 🟢 -3.40% |
| text_split_gc_1000/ (B/op) | 137.81 MB | 137.81 MB | ⚪ 0% |
| text_split_gc_1000/ (allocs/op) | 185,023 allocs | 185,004 allocs | 🟢 -0.01% |
| text_100/ (ns/op) | 253021.00 ns | 240890.00 ns | 🟢 -4.79% |
| text_100/ (B/op) | 122.66 KB | 122.66 KB | ⚪ 0% |
| text_100/ (allocs/op) | 5,185 allocs | 5,185 allocs | ⚪ 0% |
| text_1000/ (ns/op) | 2.67 ms | 2.62 ms | 🟢 -1.93% |
| text_1000/ (B/op) | 1.17 MB | 1.17 MB | ⚪ 0% |
| text_1000/ (allocs/op) | 51,088 allocs | 51,088 allocs | ⚪ 0% |
| array_1000/ (ns/op) | 1.34 ms | 1.30 ms | 🟢 -2.71% |
| array_1000/ (B/op) | 1.13 MB | 1.13 MB | ⚪ 0% |
| array_1000/ (allocs/op) | 12,883 allocs | 12,883 allocs | ⚪ 0% |
| array_10000/ (ns/op) | 14.44 ms | 14.27 ms | 🟢 -1.16% |
| array_10000/ (B/op) | 10.29 MB | 10.29 MB | 🔴 +0.02% |
| array_10000/ (allocs/op) | 130,736 allocs | 130,742 allocs | ⚪ 0% |
| array_gc_100/ (ns/op) | 146229.00 ns | 138359.00 ns | 🟢 -5.38% |
| array_gc_100/ (B/op) | 104.11 KB | 104.11 KB | ⚪ 0% |
| array_gc_100/ (allocs/op) | 1,372 allocs | 1,372 allocs | ⚪ 0% |
| array_gc_1000/ (ns/op) | 1.52 ms | 1.47 ms | 🟢 -3.51% |
| array_gc_1000/ (B/op) | 1.18 MB | 1.18 MB | 🔴 +0.02% |
| array_gc_1000/ (allocs/op) | 13,931 allocs | 13,932 allocs | ⚪ 0% |
| counter_1000/ (ns/op) | 218834.00 ns | 205266.00 ns | 🟢 -6.20% |
| counter_1000/ (B/op) | 194.29 KB | 194.29 KB | ⚪ 0% |
| counter_1000/ (allocs/op) | 5,775 allocs | 5,775 allocs | ⚪ 0% |
| counter_10000/ (ns/op) | 2.33 ms | 2.27 ms | 🟢 -2.81% |
| counter_10000/ (B/op) | 2.23 MB | 2.23 MB | ⚪ 0% |
| counter_10000/ (allocs/op) | 59,782 allocs | 59,782 allocs | ⚪ 0% |
| object_1000/ (ns/op) | 1.57 ms | 1.50 ms | 🟢 -4.38% |
| object_1000/ (B/op) | 1.48 MB | 1.48 MB | 🔴 +0.01% |
| object_1000/ (allocs/op) | 10,927 allocs | 10,928 allocs | ⚪ 0% |
| object_10000/ (ns/op) | 16.45 ms | 16.72 ms | 🔴 +1.64% |
| object_10000/ (B/op) | 12.75 MB | 12.75 MB | 🔴 +0.01% |
| object_10000/ (allocs/op) | 111,230 allocs | 111,235 allocs | ⚪ 0% |
| tree_100/ (ns/op) | 798715.00 ns | 774867.00 ns | 🟢 -2.99% |
| tree_100/ (B/op) | 534.70 KB | 534.71 KB | ⚪ 0% |
| tree_100/ (allocs/op) | 4,716 allocs | 4,716 allocs | ⚪ 0% |
| tree_1000/ (ns/op) | 55.32 ms | 52.25 ms | 🟢 -5.54% |
| tree_1000/ (B/op) | 43.85 MB | 43.85 MB | ⚪ 0% |
| tree_1000/ (allocs/op) | 46,125 allocs | 46,125 allocs | ⚪ 0% |
| tree_10000/ (ns/op) | 7.13 s | 6.75 s | 🟢 -5.29% |
| tree_10000/ (B/op) | 4.30 GB | 4.30 GB | ⚪ 0% |
| tree_10000/ (allocs/op) | 460,201 allocs | 460,210 allocs | ⚪ 0% |
| tree_edit_gc_100/ (ns/op) | 2.87 ms | 2.82 ms | 🟢 -1.79% |
| tree_edit_gc_100/ (B/op) | 2.47 MB | 2.47 MB | ⚪ 0% |
| tree_edit_gc_100/ (allocs/op) | 11,565 allocs | 11,565 allocs | ⚪ 0% |
| tree_edit_gc_1000/ (ns/op) | 228.33 ms | 220.80 ms | 🟢 -3.30% |
| tree_edit_gc_1000/ (B/op) | 214.66 MB | 214.66 MB | ⚪ 0% |
| tree_edit_gc_1000/ (allocs/op) | 115,143 allocs | 115,145 allocs | ⚪ 0% |
| tree_split_gc_100/ (ns/op) | 2.08 ms | 2.01 ms | 🟢 -3.28% |
| tree_split_gc_100/ (B/op) | 1.55 MB | 1.55 MB | ⚪ 0% |
| tree_split_gc_100/ (allocs/op) | 8,538 allocs | 8,538 allocs | ⚪ 0% |
| tree_split_gc_1000/ (ns/op) | 152.61 ms | 142.31 ms | 🟢 -6.75% |
| tree_split_gc_1000/ (B/op) | 137.62 MB | 137.61 MB | ⚪ 0% |
| tree_split_gc_1000/ (allocs/op) | 95,887 allocs | 95,889 allocs | ⚪ 0% |
BenchmarkDocumentDeletion
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| single_text_delete_all_10000/ (ns/op) | 18.99 ms | 20.63 ms | 🔴 +8.60% |
| single_text_delete_all_10000/ (B/op) | 11.30 MB | 11.30 MB | ⚪ 0% |
| single_text_delete_all_10000/ (allocs/op) | 86,134 allocs | 86,134 allocs | ⚪ 0% |
| single_text_delete_all_100000/ (ns/op) | 305.51 ms | 279.33 ms | 🟢 -8.57% |
| single_text_delete_all_100000/ (B/op) | 112.71 MB | 112.71 MB | ⚪ 0% |
| single_text_delete_all_100000/ (allocs/op) | 866,053 allocs | 866,058 allocs | ⚪ 0% |
| single_tree_delete_all_1000/ (ns/op) | 53.68 ms | 57.50 ms | 🔴 +7.13% |
| single_tree_delete_all_1000/ (B/op) | 44.84 MB | 44.84 MB | ⚪ 0% |
| single_tree_delete_all_1000/ (allocs/op) | 55,203 allocs | 55,202 allocs | ⚪ 0% |
| single_text_delete_range_100/ (ns/op) | 460652.00 ns | 480132.00 ns | 🔴 +4.23% |
| single_text_delete_range_100/ (B/op) | 264.21 KB | 264.21 KB | ⚪ 0% |
| single_text_delete_range_100/ (allocs/op) | 7,417 allocs | 7,417 allocs | ⚪ 0% |
| single_text_delete_range_1000/ (ns/op) | 9.11 ms | 9.32 ms | 🔴 +2.27% |
| single_text_delete_range_1000/ (B/op) | 6.59 MB | 6.59 MB | ⚪ 0% |
| single_text_delete_range_1000/ (allocs/op) | 76,773 allocs | 76,773 allocs | ⚪ 0% |
| single_tree_delete_range_100/ (ns/op) | 996881.00 ns | 1.01 ms | 🔴 +1.79% |
| single_tree_delete_range_100/ (B/op) | 731.46 KB | 731.51 KB | ⚪ 0% |
| single_tree_delete_range_100/ (allocs/op) | 6,129 allocs | 6,129 allocs | ⚪ 0% |
| single_tree_delete_range_1000/ (ns/op) | 62.62 ms | 65.47 ms | 🔴 +4.55% |
| single_tree_delete_range_1000/ (B/op) | 54.74 MB | 54.74 MB | ⚪ 0% |
| single_tree_delete_range_1000/ (allocs/op) | 60,831 allocs | 60,826 allocs | ⚪ 0% |
BenchmarkRPC
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| client_to_server/ (ns/op) | 294.66 ms | 288.76 ms | 🟢 -2.00% |
| client_to_server/ (B/op) | 13.36 MB | 13.34 MB | 🟢 -0.14% |
| client_to_server/ (allocs/op) | 171,447 allocs | 171,219 allocs | 🟢 -0.13% |
| client_to_client_via_server/ (ns/op) | 356.89 ms | 355.24 ms | 🟢 -0.46% |
| client_to_client_via_server/ (B/op) | 20.51 MB | 21.81 MB | 🔴 +6.34% |
| client_to_client_via_server/ (allocs/op) | 280,841 allocs | 281,812 allocs | 🔴 +0.35% |
| attach_large_document/ (ns/op) | 1.30 s | 1.31 s | 🔴 +0.83% |
| attach_large_document/ (B/op) | 1.86 GB | 1.87 GB | 🔴 +0.66% |
| attach_large_document/ (allocs/op) | 11,134 allocs | 11,103 allocs | 🟢 -0.28% |
| adminCli_to_server/ (ns/op) | 565.10 ms | 563.85 ms | 🟢 -0.22% |
| adminCli_to_server/ (B/op) | 21.89 MB | 21.50 MB | 🟢 -1.82% |
| adminCli_to_server/ (allocs/op) | 316,680 allocs | 316,681 allocs | ⚪ 0% |
BenchmarkLocker
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| (ns/op) | 82.38 ns | 83.37 ns | 🔴 +1.20% |
| (B/op) | 32.00 B | 32.00 B | ⚪ 0% |
| (allocs/op) | 1 allocs | 1 allocs | ⚪ 0% |
BenchmarkLockerParallel
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| (ns/op) | 46.11 ns | 45.55 ns | 🟢 -1.21% |
| (B/op) | 0.00 B | 0.00 B | ⚪ 0% |
| (allocs/op) | 0 allocs | 0 allocs | ⚪ 0% |
BenchmarkLockerMoreKeys
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| (ns/op) | 180.80 ns | 171.90 ns | 🟢 -4.92% |
| (B/op) | 31.00 B | 31.00 B | ⚪ 0% |
| (allocs/op) | 0 allocs | 0 allocs | ⚪ 0% |
BenchmarkRWLocker
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| RWLock_rate_2/ (ns/op) | 49.13 ns | 49.49 ns | 🔴 +0.73% |
| RWLock_rate_2/ (B/op) | 0.00 B | 0.00 B | ⚪ 0% |
| RWLock_rate_2/ (allocs/op) | 0 allocs | 0 allocs | ⚪ 0% |
| RWLock_rate_10/ (ns/op) | 44.02 ns | 44.31 ns | 🔴 +0.66% |
| RWLock_rate_10/ (B/op) | 0.00 B | 0.00 B | ⚪ 0% |
| RWLock_rate_10/ (allocs/op) | 0 allocs | 0 allocs | ⚪ 0% |
| RWLock_rate_100/ (ns/op) | 60.73 ns | 60.30 ns | 🟢 -0.71% |
| RWLock_rate_100/ (B/op) | 2.00 B | 2.00 B | ⚪ 0% |
| RWLock_rate_100/ (allocs/op) | 0 allocs | 0 allocs | ⚪ 0% |
| RWLock_rate_1000/ (ns/op) | 88.30 ns | 88.79 ns | 🔴 +0.55% |
| RWLock_rate_1000/ (B/op) | 8.00 B | 8.00 B | ⚪ 0% |
| RWLock_rate_1000/ (allocs/op) | 0 allocs | 0 allocs | ⚪ 0% |
BenchmarkPresenceConcurrency
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| 0-100-10/ (ns/op) | 1.80 s | 1.77 s | 🟢 -1.67% |
| 0-100-10/ (B/op) | 426.02 MB | 425.76 MB | 🟢 -0.06% |
| 0-100-10/ (allocs/op) | 5,605,273 allocs | 5,604,293 allocs | 🟢 -0.02% |
| 100-100-10/ (ns/op) | 3.15 s | 3.17 s | 🔴 +0.75% |
| 100-100-10/ (B/op) | 705.07 MB | 706.29 MB | 🔴 +0.17% |
| 100-100-10/ (allocs/op) | 8,280,624 allocs | 8,278,870 allocs | 🟢 -0.02% |
| 300-100-10/ (ns/op) | 6.59 s | 6.62 s | 🔴 +0.49% |
| 300-100-10/ (B/op) | 1.36 GB | 1.35 GB | 🟢 -0.61% |
| 300-100-10/ (allocs/op) | 15,914,521 allocs | 15,909,746 allocs | 🟢 -0.03% |
BenchmarkChange
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| Push_10_Changes/ (ns/op) | 4.36 ms | 4.33 ms | 🟢 -0.65% |
| Push_10_Changes/ (B/op) | 140.41 KB | 140.35 KB | 🟢 -0.04% |
| Push_10_Changes/ (allocs/op) | 1,524 allocs | 1,523 allocs | 🟢 -0.07% |
| Push_100_Changes/ (ns/op) | 18.50 ms | 18.12 ms | 🟢 -2.05% |
| Push_100_Changes/ (B/op) | 768.86 KB | 763.90 KB | 🟢 -0.65% |
| Push_100_Changes/ (allocs/op) | 8,770 allocs | 8,768 allocs | 🟢 -0.02% |
| Push_1000_Changes/ (ns/op) | 152.02 ms | 149.02 ms | 🟢 -1.97% |
| Push_1000_Changes/ (B/op) | 7.08 MB | 7.14 MB | 🔴 +0.85% |
| Push_1000_Changes/ (allocs/op) | 83,147 allocs | 83,151 allocs | ⚪ 0% |
| Pull_10_Changes/ (ns/op) | 3.17 ms | 3.22 ms | 🔴 +1.51% |
| Pull_10_Changes/ (B/op) | 106.23 KB | 106.11 KB | 🟢 -0.11% |
| Pull_10_Changes/ (allocs/op) | 1,121 allocs | 1,120 allocs | 🟢 -0.09% |
| Pull_100_Changes/ (ns/op) | 5.24 ms | 5.46 ms | 🔴 +4.32% |
| Pull_100_Changes/ (B/op) | 340.07 KB | 339.88 KB | 🟢 -0.06% |
| Pull_100_Changes/ (allocs/op) | 4,758 allocs | 4,760 allocs | 🔴 +0.04% |
| Pull_1000_Changes/ (ns/op) | 11.20 ms | 11.37 ms | 🔴 +1.44% |
| Pull_1000_Changes/ (B/op) | 2.15 MB | 2.15 MB | 🟢 -0.06% |
| Pull_1000_Changes/ (allocs/op) | 43,363 allocs | 43,364 allocs | ⚪ 0% |
BenchmarkSnapshot
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| Push_3KB_snapshot/ (ns/op) | 21.59 ms | 21.97 ms | 🔴 +1.78% |
| Push_3KB_snapshot/ (B/op) | 904.32 KB | 909.32 KB | 🔴 +0.55% |
| Push_3KB_snapshot/ (allocs/op) | 8,771 allocs | 8,766 allocs | 🟢 -0.06% |
| Push_30KB_snapshot/ (ns/op) | 156.35 ms | 155.11 ms | 🟢 -0.80% |
| Push_30KB_snapshot/ (B/op) | 7.11 MB | 7.33 MB | 🔴 +3.02% |
| Push_30KB_snapshot/ (allocs/op) | 83,269 allocs | 83,219 allocs | 🟢 -0.06% |
| Pull_3KB_snapshot/ (ns/op) | 7.53 ms | 8.16 ms | 🔴 +8.40% |
| Pull_3KB_snapshot/ (B/op) | 1.01 MB | 1.01 MB | 🟢 -0.06% |
| Pull_3KB_snapshot/ (allocs/op) | 17,237 allocs | 17,235 allocs | 🟢 -0.01% |
| Pull_30KB_snapshot/ (ns/op) | 19.70 ms | 20.78 ms | 🔴 +5.44% |
| Pull_30KB_snapshot/ (B/op) | 8.37 MB | 8.36 MB | 🟢 -0.03% |
| Pull_30KB_snapshot/ (allocs/op) | 168,507 allocs | 168,510 allocs | ⚪ 0% |
BenchmarkSplayTree
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| stress_test_100000/ (ns/op) | 0.18 ns | 0.18 ns | 🟢 -0.65% |
| stress_test_100000/ (B/op) | 0.00 B | 0.00 B | ⚪ 0% |
| stress_test_100000/ (allocs/op) | 0 allocs | 0 allocs | ⚪ 0% |
| stress_test_200000/ (ns/op) | 0.37 ns | 0.39 ns | 🔴 +3.78% |
| stress_test_200000/ (B/op) | 0.00 B | 0.00 B | ⚪ 0% |
| stress_test_200000/ (allocs/op) | 0 allocs | 0 allocs | ⚪ 0% |
| stress_test_300000/ (ns/op) | 0.55 ns | 0.60 ns | 🔴 +8.17% |
| stress_test_300000/ (B/op) | 0.00 B | 0.00 B | ⚪ 0% |
| stress_test_300000/ (allocs/op) | 0 allocs | 0 allocs | ⚪ 0% |
| random_access_100000/ (ns/op) | 0.01 ns | 0.01 ns | 🔴 +4.59% |
| random_access_100000/ (B/op) | 0.00 B | 0.00 B | ⚪ 0% |
| random_access_100000/ (allocs/op) | 0 allocs | 0 allocs | ⚪ 0% |
| random_access_200000/ (ns/op) | 0.03 ns | 0.03 ns | 🟢 -2.32% |
| random_access_200000/ (B/op) | 0.00 B | 0.00 B | ⚪ 0% |
| random_access_200000/ (allocs/op) | 0 allocs | 0 allocs | ⚪ 0% |
| random_access_300000/ (ns/op) | 0.05 ns | 0.05 ns | 🔴 +3.56% |
| random_access_300000/ (B/op) | 0.00 B | 0.00 B | ⚪ 0% |
| random_access_300000/ (allocs/op) | 0 allocs | 0 allocs | ⚪ 0% |
| editing_trace_bench/ (ns/op) | 0.00 ns | 0.00 ns | 🔴 +24.72% |
| editing_trace_bench/ (B/op) | 0.00 B | 0.00 B | ⚪ 0% |
| editing_trace_bench/ (allocs/op) | 0 allocs | 0 allocs | ⚪ 0% |
BenchmarkSync
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| memory_sync_10_test/ (ns/op) | 7315.00 ns | 7519.00 ns | 🔴 +2.79% |
| memory_sync_10_test/ (B/op) | 1.34 KB | 1.34 KB | ⚪ 0% |
| memory_sync_10_test/ (allocs/op) | 35 allocs | 35 allocs | ⚪ 0% |
| memory_sync_100_test/ (ns/op) | 56403.00 ns | 56592.00 ns | 🔴 +0.34% |
| memory_sync_100_test/ (B/op) | 9.52 KB | 9.49 KB | 🟢 -0.30% |
| memory_sync_100_test/ (allocs/op) | 268 allocs | 268 allocs | ⚪ 0% |
| memory_sync_1000_test/ (ns/op) | 562858.00 ns | 612083.00 ns | 🔴 +8.75% |
| memory_sync_1000_test/ (B/op) | 81.00 KB | 75.76 KB | 🟢 -6.46% |
| memory_sync_1000_test/ (allocs/op) | 2,272 allocs | 2,109 allocs | 🟢 -7.17% |
| memory_sync_10000_test/ (ns/op) | 7.76 ms | 8.54 ms | 🔴 +10.13% |
| memory_sync_10000_test/ (B/op) | 757.15 KB | 749.83 KB | 🟢 -0.97% |
| memory_sync_10000_test/ (allocs/op) | 20,504 allocs | 20,408 allocs | 🟢 -0.47% |
BenchmarkSyncConcurrency
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| 1-100-10/ (ns/op) | 15.33 s | 15.55 s | 🔴 +1.43% |
| 1-100-10/ (B/op) | 8.41 GB | 8.41 GB | 🔴 +0.05% |
| 1-100-10/ (allocs/op) | 176,505,379 allocs | 176,494,978 allocs | ⚪ 0% |
| 100-100-10/ (ns/op) | 28.06 s | 28.12 s | 🔴 +0.23% |
| 100-100-10/ (B/op) | 16.82 GB | 16.80 GB | 🟢 -0.11% |
| 100-100-10/ (allocs/op) | 338,181,045 allocs | 338,119,643 allocs | 🟢 -0.02% |
| 300_100-10/ (ns/op) | 54.85 s | 54.64 s | 🟢 -0.38% |
| 300_100-10/ (B/op) | 33.28 GB | 33.23 GB | 🟢 -0.13% |
| 300_100-10/ (allocs/op) | 660,511,069 allocs | 660,518,775 allocs | ⚪ 0% |
BenchmarkTextEditing
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| (ns/op) | 5.99 s | 6.04 s | 🔴 +0.85% |
| (B/op) | 3.94 GB | 3.95 GB | ⚪ 0% |
| (allocs/op) | 21,371,936 allocs | 21,372,071 allocs | ⚪ 0% |
BenchmarkTree
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| 10000_vertices_to_protobuf/ (ns/op) | 4.71 ms | 4.79 ms | 🔴 +1.56% |
| 10000_vertices_to_protobuf/ (B/op) | 6.68 MB | 6.68 MB | ⚪ 0% |
| 10000_vertices_to_protobuf/ (allocs/op) | 90,026 allocs | 90,026 allocs | ⚪ 0% |
| 10000_vertices_from_protobuf/ (ns/op) | 210.60 ms | 211.17 ms | 🔴 +0.27% |
| 10000_vertices_from_protobuf/ (B/op) | 442.15 MB | 442.14 MB | ⚪ 0% |
| 10000_vertices_from_protobuf/ (allocs/op) | 280,046 allocs | 280,046 allocs | ⚪ 0% |
| 20000_vertices_to_protobuf/ (ns/op) | 9.71 ms | 10.34 ms | 🔴 +6.52% |
| 20000_vertices_to_protobuf/ (B/op) | 13.53 MB | 13.53 MB | ⚪ 0% |
| 20000_vertices_to_protobuf/ (allocs/op) | 180,029 allocs | 180,030 allocs | ⚪ 0% |
| 20000_vertices_from_protobuf/ (ns/op) | 841.49 ms | 861.39 ms | 🔴 +2.36% |
| 20000_vertices_from_protobuf/ (B/op) | 1.70 GB | 1.70 GB | ⚪ 0% |
| 20000_vertices_from_protobuf/ (allocs/op) | 560,077 allocs | 560,076 allocs | ⚪ 0% |
| 30000_vertices_to_protobuf/ (ns/op) | 15.12 ms | 15.84 ms | 🔴 +4.72% |
| 30000_vertices_to_protobuf/ (B/op) | 19.94 MB | 19.94 MB | ⚪ 0% |
| 30000_vertices_to_protobuf/ (allocs/op) | 270,030 allocs | 270,031 allocs | ⚪ 0% |
| 30000_vertices_from_protobuf/ (ns/op) | 1.92 s | 1.94 s | 🔴 +0.81% |
| 30000_vertices_from_protobuf/ (B/op) | 3.75 GB | 3.75 GB | ⚪ 0% |
| 30000_vertices_from_protobuf/ (allocs/op) | 840,213 allocs | 840,122 allocs | 🟢 -0.01% |
BenchmarkVersionVector
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| clients_10/ (ns/op) | 132.81 ms | 130.80 ms | 🟢 -1.51% |
| clients_10/ (1_changepack(bytes)) | 745.00 B | 745.00 B | ⚪ 0% |
| clients_10/ (2_snapshot(bytes)) | 399.00 B | 399.00 B | ⚪ 0% |
| clients_10/ (3_pushpull(ms)) | 5.00 ms | 5.00 ms | ⚪ 0% |
| clients_10/ (4_attach(ms)) | 5.00 ms | 5.00 ms | ⚪ 0% |
| clients_10/ (5_changepack_after_detach(bytes)) | 805.00 B | 805.00 B | ⚪ 0% |
| clients_10/ (6_snapshot_after_detach(bytes)) | 156.00 B | 156.00 B | ⚪ 0% |
| clients_10/ (7_pushpull_after_detach(ms)) | 5.00 ms | 5.00 ms | ⚪ 0% |
| clients_10/ (B/op) | 19.13 MB | 17.52 MB | 🟢 -8.45% |
| clients_10/ (allocs/op) | 64,365 allocs | 64,306 allocs | 🟢 -0.09% |
| clients_100/ (ns/op) | 1.13 s | 1.13 s | ⚪ 0% |
| clients_100/ (1_changepack(bytes)) | 6.14 KB | 6.14 KB | ⚪ 0% |
| clients_100/ (2_snapshot(bytes)) | 3.10 KB | 3.10 KB | ⚪ 0% |
| clients_100/ (3_pushpull(ms)) | 6.00 ms | 6.00 ms | ⚪ 0% |
| clients_100/ (4_attach(ms)) | 8.00 ms | 8.00 ms | ⚪ 0% |
| clients_100/ (5_changepack_after_detach(bytes)) | 6.21 KB | 6.21 KB | ⚪ 0% |
| clients_100/ (6_snapshot_after_detach(bytes)) | 157.00 B | 157.00 B | ⚪ 0% |
| clients_100/ (7_pushpull_after_detach(ms)) | 6.00 ms | 6.00 ms | ⚪ 0% |
| clients_100/ (B/op) | 178.59 MB | 178.63 MB | 🔴 +0.02% |
| clients_100/ (allocs/op) | 879,435 allocs | 879,559 allocs | 🔴 +0.01% |
| clients_1000/ (ns/op) | 20.37 s | 20.29 s | 🟢 -0.42% |
| clients_1000/ (1_changepack(bytes)) | 60.16 KB | 60.16 KB | ⚪ 0% |
| clients_1000/ (2_snapshot(bytes)) | 30.10 KB | 30.10 KB | ⚪ 0% |
| clients_1000/ (3_pushpull(ms)) | 15.00 ms | 16.00 ms | 🔴 +6.67% |
| clients_1000/ (4_attach(ms)) | 64.00 ms | 64.00 ms | ⚪ 0% |
| clients_1000/ (5_changepack_after_detach(bytes)) | 60.22 KB | 60.22 KB | ⚪ 0% |
| clients_1000/ (6_snapshot_after_detach(bytes)) | 161.00 B | 161.00 B | ⚪ 0% |
| clients_1000/ (7_pushpull_after_detach(ms)) | 17.00 ms | 23.00 ms | 🔴 +35.29% |
| clients_1000/ (B/op) | 3.64 GB | 3.64 GB | ⚪ 0% |
| clients_1000/ (allocs/op) | 43,194,277 allocs | 43,196,913 allocs | ⚪ 0% |
BenchmarkWebhook
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| Send_10_Webhooks_to_10_Endpoints/ (ns/op) | 12.35 ms | 12.25 ms | 🟢 -0.78% |
| Send_10_Webhooks_to_10_Endpoints/ (B/op) | 808.29 KB | 808.44 KB | 🔴 +0.02% |
| Send_10_Webhooks_to_10_Endpoints/ (allocs/op) | 10,116 allocs | 10,117 allocs | ⚪ 0% |
| Send_100_Webhooks_to_10_Endpoints/ (ns/op) | 123.76 ms | 122.27 ms | 🟢 -1.21% |
| Send_100_Webhooks_to_10_Endpoints/ (B/op) | 8.08 MB | 8.08 MB | 🟢 -0.03% |
| Send_100_Webhooks_to_10_Endpoints/ (allocs/op) | 101,159 allocs | 101,156 allocs | ⚪ 0% |
| Send_10_Webhooks_to_100_Endpoints/ (ns/op) | 125.13 ms | 123.92 ms | 🟢 -0.97% |
| Send_10_Webhooks_to_100_Endpoints/ (B/op) | 8.28 MB | 8.26 MB | 🟢 -0.28% |
| Send_10_Webhooks_to_100_Endpoints/ (allocs/op) | 102,533 allocs | 102,361 allocs | 🟢 -0.17% |
| Send_100_Webhooks_to_100_Endpoints/ (ns/op) | 1.25 s | 1.23 s | 🟢 -1.21% |
| Send_100_Webhooks_to_100_Endpoints/ (B/op) | 82.38 MB | 82.36 MB | 🟢 -0.02% |
| Send_100_Webhooks_to_100_Endpoints/ (allocs/op) | 1,022,423 allocs | 1,022,392 allocs | ⚪ 0% |
| Send_10_Webhooks_to_1000_Endpoints/ (ns/op) | 2.99 s | 2.97 s | 🟢 -0.69% |
| Send_10_Webhooks_to_1000_Endpoints/ (B/op) | 209.56 MB | 209.67 MB | 🔴 +0.05% |
| Send_10_Webhooks_to_1000_Endpoints/ (allocs/op) | 1,716,317 allocs | 1,716,439 allocs | ⚪ 0% |
BenchmarkWebhookWithLimit
| Benchmark suite | Previous | Current | Change |
|---|---|---|---|
| Send_10_Webhooks_to_10_Endpoints_with_limit/ (ns/op) | 3.96 ms | 3.94 ms | 🟢 -0.44% |
| Send_10_Webhooks_to_10_Endpoints_with_limit/ (B/op) | 306.34 KB | 306.35 KB | ⚪ 0% |
| Send_10_Webhooks_to_10_Endpoints_with_limit/ (allocs/op) | 2,913 allocs | 2,913 allocs | ⚪ 0% |
| Send_100_Webhooks_to_10_Endpoints_with_limit/ (ns/op) | 4.25 ms | 4.22 ms | 🟢 -0.77% |
| Send_100_Webhooks_to_10_Endpoints_with_limit/ (B/op) | 428.91 KB | 428.84 KB | 🟢 -0.02% |
| Send_100_Webhooks_to_10_Endpoints_with_limit/ (allocs/op) | 4,714 allocs | 4,714 allocs | ⚪ 0% |
| Send_10_Webhooks_to_100_Endpoints_with_limit/ (ns/op) | 39.84 ms | 39.47 ms | 🟢 -0.92% |
| Send_10_Webhooks_to_100_Endpoints_with_limit/ (B/op) | 3.08 MB | 3.08 MB | 🟢 -0.02% |
| Send_10_Webhooks_to_100_Endpoints_with_limit/ (allocs/op) | 29,174 allocs | 29,176 allocs | ⚪ 0% |
| Send_100_Webhooks_to_100_Endpoints_with_limit/ (ns/op) | 42.49 ms | 42.07 ms | 🟢 -1.00% |
| Send_100_Webhooks_to_100_Endpoints_with_limit/ (B/op) | 4.37 MB | 4.37 MB | ⚪ 0% |
| Send_100_Webhooks_to_100_Endpoints_with_limit/ (allocs/op) | 47,193 allocs | 47,191 allocs | ⚪ 0% |
| Send_10_Webhooks_to_1000_Endpoints_with_limit/ (ns/op) | 395.78 ms | 398.16 ms | 🔴 +0.60% |
| Send_10_Webhooks_to_1000_Endpoints_with_limit/ (B/op) | 44.47 MB | 44.91 MB | 🔴 +0.99% |
| Send_10_Webhooks_to_1000_Endpoints_with_limit/ (allocs/op) | 380,086 allocs | 380,120 allocs | ⚪ 0% |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
pkg/document/yson/yson_test.go (2)
286-288: Line exceeds 120 characters – breaksllllinter and readabilityThe raw assertion string is ~360 chars long. You can keep tests linter-clean and readable by:
assert.Equal(t, - `["hello",Int(32),Long(64),1.23,null,true,BinData("AQID"),Date("2025-01-02T15:04:05.058Z"),Counter(Int(32)),Counter(Long(64)),["nested",Long(1)],{"nested":"nest-obj"},Tree({"type":"p","children":[{"type":"text","value":"Tree in array"}]}),Text([{"val":"Hello","attrs":{"color":"red"}},{"val":"World","attrs":{"color":"blue"}}])]`, + `[` + + `"hello",` + + `Int(32),` + + `Long(64),` + + `1.23,` + + `null,` + + `true,` + + `BinData("AQID"),` + + `Date("2025-01-02T15:04:05.058Z"),` + + `Counter(Int(32)),` + + `Counter(Long(64)),` + + `["nested",Long(1)],` + + `{"nested":"nest-obj"},` + + `Tree({"type":"p","children":[{"type":"text","value":"Tree in array"}]}),` + + `Text([{"val":"Hello","attrs":{"color":"red"}},{"val":"World","attrs":{"color":"blue"}}])` + + `]`, )This keeps each line well below the 120-char limit without altering semantics.
🧰 Tools
🪛 golangci-lint (1.64.8)
287-287: The line is 359 characters long, which exceeds the maximum of 120 characters.
(lll)
289-293: Commented-out assertions hide missing functionality – mark test ast.SkipinsteadSince
Unmarshalis not implemented, the test is partially disabled. Leaving large blocks commented can mislead future readers and will not surface once the feature lands. Prefer an explicit skip with a TODO:-// TODO(hackerwins): Implement the Unmarshal function -// unmarshaled := &yson.Array{} -// assert.NoError(t, yson.Unmarshal(marshaled, unmarshaled)) -// assert.Equal(t, arr, unmarshaled) +t.Skip("Unmarshal not implemented yet – see issue #<id>")This keeps the intent visible in test output and CI dashboards.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/document/yson/to_yson.go(1 hunks)pkg/document/yson/yson.go(4 hunks)pkg/document/yson/yson_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/document/yson/to_yson.go
🧰 Additional context used
🪛 golangci-lint (1.64.8)
pkg/document/yson/yson_test.go
287-287: The line is 359 characters long, which exceeds the maximum of 120 characters.
(lll)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: complex-test
- GitHub Check: build
- GitHub Check: bench
🔇 Additional comments (1)
pkg/document/yson/yson.go (1)
167-176:Counter.Marshalsilently trustsValuetype – potential panics
Counterallows anyinterface{}inValue, but thefmt.Sprintfbranch forInt/Longassumes the contained type matches the counter type. If callers accidentally set the wrong Go type, the string may embed a%!v(<type=value>)error, or worse, downstream logic may interpret an incompatible numeric width.Consider validating the concrete type:
case crdt.IntegerCnt: - return fmt.Sprintf("Counter(Int(%v))", y.Value) + if v, ok := y.Value.(int32); ok { + return fmt.Sprintf("Counter(Int(%d))", v) + } + return "Counter(Int(INVALID))"or store counters as
int64internally to enforce consistency.
hackerwins
force-pushed
the
revise-compact
branch
from
214b0cc to
b950ce3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
pkg/document/yson/yson_test.go (1)
38-45:⚠️ Potential issueTypo in key path – likely unintended nesting
SetDouble("1.4", 1.79)is missing the"k"prefix used by the surrounding keys ("k1.0","k1.1"…).
This creates a sibling root key"1"with child"4"instead of"k1.4".- SetDouble("1.4", 1.79). + SetDouble("k1.4", 1.79).
♻️ Duplicate comments (4)
pkg/document/yson/yson.go (4)
196-211: Attribute/Value escaping is still unsafe inText.MarshalValues and attribute keys are injected with
%s; any embedded quote or back-slash will break the resulting JSON fragment (e.g.,Hello "Yorkie").
Use%qorencoding/jsonto escape properly, exactly as pointed out in the previous review.
213-234:TreeNode.Marshalrepeats the unescaped-string bugNeither
node.Valuenor the attribute keys/values are escaped. Malformed output is produced when the content contains quotes, back-slashes, or control characters.Refer to the earlier suggestion to marshal via
encoding/json(or at least%q) to guarantee validity.
451-471:⚠️ Potential issueGlobal
strings.ReplaceAllstill corrupts literals
preprocessTypeValuesblindly replaces every occurrence of)and macro prefixes, even inside quoted strings.
Input like["text with Int(", "foo)bar"]will be mangled, yielding invalid JSON and data loss.A state-aware scanner that transforms tokens only when outside of string literals (and that tracks parenthesis depth) is required. Until then, unmarshalling untrusted data is unsafe.
135-157: 🛠️ Refactor suggestionDeterministic float formatting is still missing
marshalPrimitiveformatsfloat64values withfmt.Sprintf("%v", v), which can switch between fixed-point and scientific notation depending on the magnitude.
Round-tripping or equality tests may fail when1e-9becomes"1e-09"on the second marshal.case float64: - return fmt.Sprintf("%v", v) + // use deterministic formatting (same rule Go’s JSON encoder applies) + return strconv.FormatFloat(v, 'f', -1, 64)
🧹 Nitpick comments (1)
pkg/document/yson/yson_test.go (1)
254-277: Add test cases for quotes/back-slashes in Text & Tree valuesCurrent round-trip tests pass only because they use simple ASCII without quotes.
Given the escaping issues flagged inText.MarshalandTreeNode.Marshal, please add cases such as:yson.Text{Nodes: []yson.TextNode{ {Value: `He said "hi"`, Attributes: map[string]string{"note": `contains "quote"`}}, }}and verify successful marshal/unmarshal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/document/yson/to_yson.go(1 hunks)pkg/document/yson/yson.go(6 hunks)pkg/document/yson/yson_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/document/yson/to_yson.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/document/yson/yson.go (3)
pkg/document/json/tree.go (2)
TreeNode(49-49)Tree(53-57)pkg/document/crdt/tree.go (2)
TreeNode(135-151)Tree(548-555)pkg/document/crdt/counter.go (2)
IntegerCnt(36-36)LongCnt(37-37)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: complex-test
- GitHub Check: build
- GitHub Check: bench
hackerwins
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
pkg/document/yson/yson.go (1)
193-207:⚠️ Potential issue
Text.Marshaldoes not escape embedded quotes / back-slashes
node.Valueand attribute keys/values are injected with%s; a value such as
He said "hi"generates invalid JSON:{"val":"He said "hi""}.Use
%qor delegate toencoding/jsonfor correctness:-attrs = append(attrs, fmt.Sprintf(`"%s":"%s"`, k, v)) +encodedKey, _ := gojson.Marshal(k) +encodedVal, _ := gojson.Marshal(v) +attrs = append(attrs, fmt.Sprintf(`%s:%s`, encodedKey, encodedVal)) ... -nodes = append(nodes, fmt.Sprintf(`{"val":"%s"}`, node.Value)) +val, _ := gojson.Marshal(node.Value) +nodes = append(nodes, fmt.Sprintf(`{"val":%s}`, val))
♻️ Duplicate comments (2)
pkg/document/yson/yson.go (2)
451-471:⚠️ Potential issueNaïve
strings.ReplaceAllstill corrupts data inside quoted literalsPrevious review pointed this out; the global
)→}substitution mangles strings that legitimately contain).At minimum, limit substitutions to wrapper depth:
- for old, new := range replacements { - data = strings.ReplaceAll(data, old, new) - } +var b strings.Builder +depth := 0 +for i := 0; i < len(data); i++ { + switch data[i] { + case '(': + depth++ + case ')': + if depth > 0 { + depth-- + if depth == 0 { b.WriteByte('}') ; continue } + } + } + b.WriteByte(data[i]) +} +data = b.String()A proper lexer would be more robust.
213-234:⚠️ Potential issue
TreeNode.Marshalsuffers from the same escaping issueThe current implementation breaks as soon as
Value,Type, or any attribute contains",\, or control characters.Recommend serialising through
encoding/jsoninstead of manual string building:-func (n *TreeNode) Marshal() string { - if n.Type == "text" { - return fmt.Sprintf(`{"type":"%s","value":"%s"}`, n.Type, n.Value) - } - ... -} +func (n *TreeNode) Marshal() string { + data, _ := gojson.Marshal(n) + return string(data) +}This guarantees well-formed JSON and keeps maintenance cost low.
🧹 Nitpick comments (2)
pkg/document/yson/yson.go (1)
135-156: Float / bool formatting is non-deterministic and may break round-trips
fmt.Sprintf("%v", v)(and the bool variant) produces different output depending on locale and value magnitude (1e-09,0.000000001, upper-caseTRUE, …).
This makes marshalled strings brittle for equality tests and complicates diffs.-case bool: - return fmt.Sprintf("%v", v) -case float64: - return fmt.Sprintf("%v", v) +case bool: + return strconv.FormatBool(v) +case float64: + // Deterministic, locale-independent representation + return strconv.FormatFloat(v, 'f', -1, 64)pkg/document/yson/yson_test.go (1)
226-260: Missing edge-case tests for escaping / special charactersThe new suite validates the happy path but still doesn’t check that quotes, back-slashes, or wrapper-like substrings inside string/byte/date values survive a marshal → unmarshal round-trip.
Consider adding cases such as:
obj := yson.Object{ "quote": `He said "hi"`, "paren": "looks like Int(42) but isn't", }This will catch the escaping issues raised in implementation comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/document/yson/to_yson.go(1 hunks)pkg/document/yson/yson.go(5 hunks)pkg/document/yson/yson_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/document/yson/to_yson.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: complex-test
- GitHub Check: build
- GitHub Check: bench
🔇 Additional comments (1)
pkg/document/yson/yson_test.go (1)
292-310: Hard-coded order in expected YSON makes test brittle
expectedstrings rely on the ordering produced byMarshal, but any future change in map key sorting or formatting will fail the test even though semantics are intact.Prefer constructing
expectedvia struct literals (as you did foractual) and comparing Go values, or parse the string and useassert.JSONEqto ignore ordering.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
pkg/document/yson/yson_test.go (1)
445-456:⚠️ Potential issueTextNode value not being set in parseText
In the
parseTextfunction, you correctly check for the existence of the "val" field, but you don't actually set the value field in the textNode.The "val" field value is not being set on the TextNode:
if _, ok := n["val"].(string); !ok { return text, errors.New("missing val field") } + +// Set the value from the "val" field +textNode.Value = n["val"].(string)This issue will cause text nodes to have empty values after unmarshaling.
♻️ Duplicate comments (5)
pkg/document/yson/yson.go (5)
170-179:⚠️ Potential issueUse error handling instead of panic in Counter.Marshal
This issue was previously identified in a past review comment but hasn't been addressed. Using panic in library code can crash applications that use this code.
Return an error string instead of panicking:
func (y Counter) Marshal() string { switch y.Type { case crdt.IntegerCnt: return fmt.Sprintf("Counter(Int(%v))", y.Value) case crdt.LongCnt: return fmt.Sprintf("Counter(Long(%v))", y.Value) default: - panic(fmt.Sprintf("unsupported type: %T", y.Value)) + return fmt.Sprintf(`Counter(Unsupported("%T"))`, y.Value) } }
181-197:⚠️ Potential issueText.Marshal doesn't properly escape quotes in text and attribute values
The function uses string formatting without properly escaping quotes and special characters in the node value and attribute values.
This can produce invalid JSON if the text or attributes contain quotes or special characters:
func (y Text) Marshal() string { var nodes []string for _, node := range y.Nodes { if len(node.Attributes) == 0 { - nodes = append(nodes, fmt.Sprintf(`{"val":"%s"}`, node.Value)) + valJSON, _ := gojson.Marshal(node.Value) + nodes = append(nodes, fmt.Sprintf(`{"val":%s}`, string(valJSON))) continue } attrs := make([]string, 0, len(node.Attributes)) for k, v := range node.Attributes { - attrs = append(attrs, fmt.Sprintf(`"%s":"%s"`, k, v)) + keyJSON, _ := gojson.Marshal(k) + valJSON, _ := gojson.Marshal(v) + attrs = append(attrs, fmt.Sprintf(`%s:%s`, string(keyJSON), string(valJSON))) } sort.Strings(attrs) - nodes = append(nodes, fmt.Sprintf(`{"val":"%s","attrs":{%s}}`, node.Value, strings.Join(attrs, ","))) + valJSON, _ := gojson.Marshal(node.Value) + nodes = append(nodes, fmt.Sprintf(`{"val":%s,"attrs":{%s}}`, string(valJSON), strings.Join(attrs, ","))) } return fmt.Sprintf("Text([%s])", strings.Join(nodes, ",")) }
203-224:⚠️ Potential issueTreeNode.Marshal doesn't properly escape quotes in values and attribute keys/values
Similar to Text.Marshal, this function uses string formatting without properly escaping quotes and special characters.
This can produce invalid JSON if node type, value, or attributes contain quotes or special characters:
func (n *TreeNode) Marshal() string { if n.Type == "text" { - return fmt.Sprintf(`{"type":"%s","value":"%s"}`, n.Type, n.Value) + typeJSON, _ := gojson.Marshal(n.Type) + valueJSON, _ := gojson.Marshal(n.Value) + return fmt.Sprintf(`{"type":%s,"value":%s}`, string(typeJSON), string(valueJSON)) } var children []string for _, child := range n.Children { children = append(children, child.Marshal()) } if len(n.Attributes) == 0 { - return fmt.Sprintf(`{"type":"%s","children":[%s]}`, n.Type, strings.Join(children, ",")) + typeJSON, _ := gojson.Marshal(n.Type) + return fmt.Sprintf(`{"type":%s,"children":[%s]}`, string(typeJSON), strings.Join(children, ",")) } var attrs []string for k, v := range n.Attributes { - attrs = append(attrs, fmt.Sprintf(`"%s":"%s"`, k, v)) + keyJSON, _ := gojson.Marshal(k) + valJSON, _ := gojson.Marshal(v) + attrs = append(attrs, fmt.Sprintf(`%s:%s`, string(keyJSON), string(valJSON))) } sort.Strings(attrs) - return fmt.Sprintf(`{"type":"%s","attrs":{%s},"children":[%s]}`, - n.Type, strings.Join(attrs, ","), strings.Join(children, ",")) + typeJSON, _ := gojson.Marshal(n.Type) + return fmt.Sprintf(`{"type":%s,"attrs":{%s},"children":[%s]}`, + string(typeJSON), strings.Join(attrs, ","), strings.Join(children, ",")) }
314-317:⚠️ Potential issueUnsafe float to integer conversions risk precision loss
Converting float64 to int32/int64 without checking for overflow or decimal truncation can lead to subtle bugs.
Add validation before converting to ensure the values can be accurately represented:
case "Int": - return int32(raw["value"].(float64)), nil + val := raw["value"].(float64) + // Check if value is within int32 range and has no fractional part + if val > float64(math.MaxInt32) || val < float64(math.MinInt32) || val != math.Trunc(val) { + return nil, fmt.Errorf("value %f cannot be accurately represented as int32", val) + } + return int32(val), nil case "Long": - return int64(raw["value"].(float64)), nil + val := raw["value"].(float64) + // Check if value is within precisely representable range and has no fractional part + if val > float64(1<<53) || val < -float64(1<<53) || val != math.Trunc(val) { + return nil, fmt.Errorf("value %f cannot be accurately represented as int64", val) + } + return int64(val), nilNote: In JavaScript/JSON, numbers are represented as IEEE 754 double-precision values, and integers beyond 2^53 lose precision.
524-544:⚠️ Potential issueNaive string replacement in preprocessTypeValues risks corrupting data
The current approach using
strings.ReplaceAllcan corrupt data inside quoted literals if they contain tokens likeInt(or).Implement a state-aware parser that only replaces tokens outside of string literals:
func preprocessTypeValues(data string) (string, error) { var result strings.Builder inString := false escaped := false i := 0 for i < len(data) { // Handle string literals if data[i] == '"' && !escaped { inString = !inString result.WriteByte(data[i]) i++ continue } // Handle escape sequences if data[i] == '\\' && !escaped { escaped = true result.WriteByte(data[i]) i++ continue } // Reset escape flag if escaped { escaped = false result.WriteByte(data[i]) i++ continue } // Only do replacements outside of string literals if !inString { // Check for type wrappers found := false replacements := map[string]string{ "Int(": `{"type":"Int","value":`, "Long(": `{"type":"Long","value":`, "BinData(\"": `{"type":"BinData","value":"`, "Date(\"": `{"type":"Date","value":"`, "Tree(": `{"type":"Tree","value":`, "Text(": `{"type":"Text","value":`, "Counter(": `{"type":"Counter","value":`, ")": `}`, } for old, new := range replacements { if strings.HasPrefix(data[i:], old) { result.WriteString(new) i += len(old) found = true break } } if found { continue } } // Default: copy character as-is result.WriteByte(data[i]) i++ } return result.String(), nil }This implementation tracks string context and only performs replacements outside of quoted literals.
🧹 Nitpick comments (4)
pkg/document/yson/yson_test.go (4)
89-92: Use pointers for TreeNode children to maintain consistencyIn this test case, you're creating a TreeNode with children but not using pointers. However, elsewhere in the codebase (e.g., line 136), TreeNode children are created as values rather than pointers. This inconsistency could lead to confusion.
Consider standardizing the approach across the codebase.
r.SetNewTree("k7"). - Edit(0, 0, &yson.TreeNode{ + Edit(0, 0, yson.TreeNode{ Type: "p", Children: []yson.TreeNode{{Type: "text", Value: "Hello world"}}, }, 0)
226-226: Test function name changed without reflecting full functionalityThe test function has been renamed from
TestYSONMarshalUnmarshaltoTestYSONMarshal, but it still tests both marshaling and unmarshaling functionality.Consider keeping the original name or using a more descriptive name like
TestYSONStringRepresentationto accurately reflect the test's purpose.
247-260: Verify the test uses known hardcoded values rather than marshaled outputThe test is unmarshaling from the string obtained by marshaling an object and then comparing against a hardcoded expected value. While this verifies round-trip capability, it doesn't validate that the marshaled format strictly matches the expected string representation.
Consider adding assertions to directly verify the string format produced by
Marshal():obj := yson.Object{...} +// Check that Marshal() produces the expected string format +expectedStr := `{"key1":"value1","key2":Int(42),...}` +assert.Equal(t, expectedStr, obj.Marshal()) actual := yson.Object{} assert.NoError(t, yson.Unmarshal(obj.Marshal(), &actual))
289-309: Test case doesn't validate the actual marshaled string formatSimilar to the Object test case, this test validates round-trip capability but doesn't directly validate the marshaled string format.
Consider adding assertions to directly verify the string format produced by
Marshal():arr := yson.Array{...} +// Check that Marshal() produces the expected string format +expectedStr := `["hello",Int(32),Long(64),...]` +assert.Equal(t, expectedStr, arr.Marshal()) actual := yson.Array{} assert.NoError(t, yson.Unmarshal(arr.Marshal(), &actual))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/document/yson/to_yson.go(1 hunks)pkg/document/yson/yson.go(4 hunks)pkg/document/yson/yson_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/document/yson/to_yson.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/document/yson/yson_test.go (3)
pkg/document/yson/yson.go (9)
TreeNode(72-87)Counter(60-63)Text(104-106)TextNode(95-101)Tree(90-92)Object(69-69)Unmarshal(228-304)Array(66-66)Element(52-57)pkg/document/json/tree.go (2)
TreeNode(49-49)Tree(53-57)pkg/document/crdt/counter.go (2)
IntegerCnt(36-36)LongCnt(37-37)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: complex-test
- GitHub Check: bench
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
pkg/document/yson/yson.go (8)
482-483:⚠️ Potential issueMissing error handling for type assertions in parseTreeNode attributes
The attributes are accessed with type assertions but no error checking, which could cause panics.
Add proper error handling:
for k, v := range attrs { - node.Attributes[k] = v.(string) + strVal, ok := v.(string) + if !ok { + return TreeNode{}, fmt.Errorf("attribute value must be string, got %T", v) + } + node.Attributes[k] = strVal }
442-448:⚠️ Potential issueMissing error handling in parseText for type assertions
The function uses type assertions without checking for errors, which could lead to panic if the input doesn't match the expected format. This was highlighted in a previous review comment.
Add proper error checking:
- n := node.(map[string]interface{}) + n, ok := node.(map[string]interface{}) + if !ok { + return text, fmt.Errorf("expected map for text node, got %T", node) + } textNode := TextNode{} - if _, ok := n["val"].(string); !ok { + val, ok := n["val"].(string) + if !ok { return text, errors.New("missing val field") } + textNode.Value = val // This line is missing in the original implementation
452-453:⚠️ Potential issueMissing error handling for type assertions in parseText attributes
The attributes are accessed with type assertions but no error checking, which could cause panics.
Add proper error handling:
for k, v := range attrs { - textNode.Attributes[k] = v.(string) + strVal, ok := v.(string) + if !ok { + return text, fmt.Errorf("attribute value must be string, got %T", v) + } + textNode.Attributes[k] = strVal }
171-179:⚠️ Potential issueReplace panic with error return in Counter.Marshal
Similar to
marshalPrimitive, this method usespanicfor unsupported counter types, which would crash the application. This was highlighted in a previous review comment but hasn't been addressed.Apply this change:
default: - panic(fmt.Sprintf("unsupported type: %T", y.Value)) + return fmt.Sprintf(`Counter(Unsupported("%T"))`, y.Value)
487-494:⚠️ Potential issueMissing error handling for type assertions in parseTreeNode children
The children are accessed with type assertions but no error checking, which could cause panics.
Add proper error handling:
if children, ok := raw["children"].([]interface{}); ok { for _, child := range children { - childNode, err := parseTreeNode(child.(map[string]interface{})) + childMap, ok := child.(map[string]interface{}) + if !ok { + return TreeNode{}, fmt.Errorf("expected map for tree child, got %T", child) + } + childNode, err := parseTreeNode(childMap) if err != nil { return TreeNode{}, err } node.Children = append(node.Children, childNode) } }
144-145:⚠️ Potential issueReplace panic with error return in marshalPrimitive
The
marshalPrimitivefunction still usespanicfor unsupported types, which can crash applications using this library. A prior review comment also raised this issue but it hasn't been addressed.Apply this change to return a safer representation:
default: - panic(fmt.Sprintf("unsupported type: %T", v)) + return fmt.Sprintf(`Unsupported("%T")`, v)
470-473:⚠️ Potential issueMissing error handling in parseTreeNode for type assertions
Similar to parseText, this function uses type assertions without checking for errors, which was pointed out in a previous review comment.
Add proper error checking:
node := TreeNode{ - Type: raw["type"].(string), + Type: "", } + typeVal, ok := raw["type"].(string) + if !ok { + return TreeNode{}, fmt.Errorf("missing or invalid type field in tree node") + } + node.Type = typeVal
502-519:⚠️ Potential issueNaïve string replacement in preprocessTypeValues can corrupt data
The function uses simple
strings.ReplaceAllfor preprocessing, which can corrupt data if the replaced tokens appear inside string literals. This was highlighted in a previous review comment.Consider implementing a proper parser that's aware of string context:
-func preprocessTypeValues(data string) (string, error) { - replacements := map[string]string{ - `Int(`: `{"type":"Int","value":`, - `Long(`: `{"type":"Long","value":`, - `BinData("`: `{"type":"BinData","value":"`, - `Date("`: `{"type":"Date","value":"`, - `Tree(`: `{"type":"Tree","value":`, - `Text(`: `{"type":"Text","value":`, - `Counter(`: `{"type":"Counter","value":`, - `)`: `}`, - } - - // Replace custom types with JSON-compatible formats - for old, new := range replacements { - data = strings.ReplaceAll(data, old, new) - } - - return data, nil +func preprocessTypeValues(data string) (string, error) { + var b strings.Builder + inString := false + escaped := false + + for i := 0; i < len(data); i++ { + c := data[i] + + // Handle string literals + if c == '"' && !escaped { + inString = !inString + b.WriteByte(c) + continue + } + + if inString { + if c == '\\' && !escaped { + escaped = true + } else { + escaped = false + } + b.WriteByte(c) + continue + } + + // Outside string literals, look for type wrappers + if checkTypeWrapper(data, i, "Int(") { + b.WriteString(`{"type":"Int","value":`) + i += len("Int(") - 1 + } else if checkTypeWrapper(data, i, "Long(") { + b.WriteString(`{"type":"Long","value":`) + i += len("Long(") - 1 + } else if checkTypeWrapper(data, i, "BinData(\"") { + b.WriteString(`{"type":"BinData","value":"`) + i += len("BinData(\"") - 1 + } else if checkTypeWrapper(data, i, "Date(\"") { + b.WriteString(`{"type":"Date","value":"`) + i += len("Date(\"") - 1 + } else if checkTypeWrapper(data, i, "Tree(") { + b.WriteString(`{"type":"Tree","value":`) + i += len("Tree(") - 1 + } else if checkTypeWrapper(data, i, "Text(") { + b.WriteString(`{"type":"Text","value":`) + i += len("Text(") - 1 + } else if checkTypeWrapper(data, i, "Counter(") { + b.WriteString(`{"type":"Counter","value":`) + i += len("Counter(") - 1 + } else if c == ')' { + b.WriteByte('}') + } else { + b.WriteByte(c) + } + } + + return b.String(), nil +} + +// Helper to check if a type wrapper pattern exists at the current position +func checkTypeWrapper(data string, pos int, pattern string) bool { + if pos+len(pattern) > len(data) { + return false + } + return data[pos:pos+len(pattern)] == pattern }
🧹 Nitpick comments (3)
pkg/document/yson/yson.go (2)
314-317: Potential precision loss when casting float64 to int32/int64When parsing Int and Long values from JSON, the code implicitly casts from float64 to int32/int64, which can lose precision for values beyond 2^53. Consider checking for truncation or providing an error for out-of-range values.
Consider adding range checks:
case "Int": - return int32(raw["value"].(float64)), nil + val := raw["value"].(float64) + if val != float64(int32(val)) || val > math.MaxInt32 || val < math.MinInt32 { + return nil, fmt.Errorf("value %v outside int32 range or not an integer", val) + } + return int32(val), nil case "Long": - return int64(raw["value"].(float64)), nil + val := raw["value"].(float64) + if val != float64(int64(val)) { + return nil, fmt.Errorf("value %v is not an integer", val) + } + return int64(val), nil
422-426: Same precision loss issue in parseCounterSimilar to the previous comment, implicit casting from float64 to int32/int64 can lose precision.
Apply the same range and integer checks here.
pkg/document/yson/yson_test.go (1)
226-260: Enhance object marshaling tests with special charactersYour object marshaling test is good, but doesn't test special characters that might need proper escaping, like quotes, newlines, or backslashes.
Consider adding test cases with special characters:
obj := yson.Object{ "key1": "value1", + "key-with\"quote": "value with \"quotes\" and newline\ncharacters", + "key-with\\backslash": "value with \\backslashes", "key2": int32(42), // ... }This would verify that the marshaling/unmarshaling correctly handles these edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/document/yson/to_yson.go(1 hunks)pkg/document/yson/yson.go(4 hunks)pkg/document/yson/yson_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/document/yson/to_yson.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: complex-test
- GitHub Check: bench
- GitHub Check: build
🔇 Additional comments (1)
pkg/document/yson/yson_test.go (1)
364-417: Great error handling testsThe error handling test suite is comprehensive and covers various edge cases. This is excellent practice to ensure that errors are properly handled and reported.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
pkg/document/yson/yson.go (4)
448-469:⚠️ Potential issueMissing proper error handling in parseText
The function doesn't properly handle type assertions, which could lead to panics.
func parseText(raw []interface{}) (Text, error) { var text Text for _, node := range raw { - n := node.(map[string]interface{}) + n, ok := node.(map[string]interface{}) + if !ok { + return text, fmt.Errorf("expected map for text node, got %T", node) + } textNode := TextNode{} - if _, ok := n["val"].(string); !ok { + val, ok := n["val"].(string) + if !ok { return text, errors.New("missing val field") } + textNode.Value = val // Missing assignment in the original code if attrs, ok := n["attrs"].(map[string]interface{}); ok { textNode.Attributes = make(map[string]string) for k, v := range attrs { - textNode.Attributes[k] = v.(string) + strVal, ok := v.(string) + if !ok { + return text, fmt.Errorf("attribute value must be string, got %T", v) + } + textNode.Attributes[k] = strVal } } text.Nodes = append(text.Nodes, textNode) } return text, nil }
124-146: 🛠️ Refactor suggestionImprove float64 formatting for deterministic marshaling
The current implementation uses
fmt.Sprintf("%v", v)for float64 values, which can switch between fixed and scientific notation depending on magnitude. This can lead to inconsistent string representations.case float64: - return fmt.Sprintf("%v", v), nil + return strconv.FormatFloat(v, 'f', -1, 64), nil
510-530: 💡 Verification agent🧩 Analysis chain
Naive string replacement can corrupt data in quoted literals
The
preprocessTypeValuesfunction blindly replaces all occurrences of certain strings, which can corrupt data inside quoted literals.For example, if a string value contains
)or a pattern likeInt(, it will be incorrectly replaced. Consider a state-aware approach that only performs replacements outside of string literals.Let's verify if we can create data that would be mangled by the current implementation:
A more robust implementation would be:
func preprocessTypeValues(data string) (string, error) { - replacements := map[string]string{ - `Int(`: `{"type":"Int","value":`, - `Long(`: `{"type":"Long","value":`, - `BinData("`: `{"type":"BinData","value":"`, - `Date("`: `{"type":"Date","value":"`, - `Tree(`: `{"type":"Tree","value":`, - `Text(`: `{"type":"Text","value":`, - `Counter(`: `{"type":"Counter","value":`, - `)`: `}`, - } - - // Replace custom types with JSON-compatible formats - for old, new := range replacements { - data = strings.ReplaceAll(data, old, new) - } + // Use a state-aware approach to only replace tokens outside string literals + var result strings.Builder + inString := false + escaped := false + + for i := 0; i < len(data); i++ { + c := data[i] + + // Handle string state + if c == '"' && !escaped { + inString = !inString + result.WriteByte(c) + continue + } + + if inString { + if c == '\\' && !escaped { + escaped = true + } else { + escaped = false + } + result.WriteByte(c) + continue + } + + // Only process replacements outside of strings + if !inString { + // Check for type wrappers + if i+4 <= len(data) && data[i:i+4] == "Int(" { + result.WriteString(`{"type":"Int","value":`) + i += 3 // Skip "Int(" + continue + } + // Add similar checks for other types + // ... + + // Handle closing parenthesis, but ensure it's a wrapper closing + if c == ')' { + // Check if this is a wrapper closing (would need context tracking) + result.WriteString("}") + continue + } + } + + // Default: copy character as-is + result.WriteByte(c) + } + + return result.String(), nil return data, nil }
🏁 Script executed:
#!/bin/bash # Create a test string with patterns that would be mangled echo 'Creating test string with tokens that would be mangled...' TEST_STRING='["A string with Int(42) inside", "Another with closing parenthesis)"]' echo "Original: $TEST_STRING" # Simulate the replacements done by preprocessTypeValues REPLACED=$(echo "$TEST_STRING" | sed 's/Int(/{"type":"Int","value":/g' | sed 's/)$/}/g') echo "After replacement: $REPLACED" # Check if this would produce valid JSON echo "Validating if result is valid JSON..." echo "$REPLACED" | jq . >/dev/null 2>&1 if [ $? -eq 0 ]; then echo "Result is valid JSON" else echo "Result is INVALID JSON - this confirms the issue" fiLength of output: 1156
Ensure
preprocessTypeValuesonly replaces tokens outside of string literalsThe current implementation uses blind
strings.ReplaceAll, which corrupts data when patterns likeInt(or)appear inside quoted strings. Your test case confirmed that this produces invalid JSON.Locations to fix:
- pkg/document/yson/yson.go, function
preprocessTypeValues(around lines 510–530)Recommended update (illustrative diff):
func preprocessTypeValues(data string) (string, error) { - // naive replacements - for old, new := range replacements { - data = strings.ReplaceAll(data, old, new) - } - return data, nil + var result strings.Builder + inString, escaped := false, false + for i := 0; i < len(data); i++ { + c := data[i] + // Toggle string state + if c == '"' && !escaped { + inString = !inString + result.WriteByte(c) + continue + } + if inString { + // Handle escaped chars inside strings + if c == '\\' && !escaped { + escaped = true + } else { + escaped = false + } + result.WriteByte(c) + continue + } + // Outside strings: perform token replacements + switch { + case i+4 <= len(data) && data[i:i+4] == "Int(": + result.WriteString(`{"type":"Int","value":`) + i += 3 + case c == ')': + result.WriteByte('}') + default: + result.WriteByte(c) + } + } + return result.String(), nilThis state-aware loop only transforms wrapper tokens when
inString == false, preserving any literals containing those patterns.
480-508:⚠️ Potential issueMissing error handling in parseTreeNode
The function uses type assertions without checking for errors, which could lead to panics.
func parseTreeNode(raw map[string]interface{}) (TreeNode, error) { + typeVal, ok := raw["type"].(string) + if !ok { + return TreeNode{}, fmt.Errorf("missing or invalid type field in tree node") + } node := TreeNode{ - Type: raw["type"].(string), + Type: typeVal, } if value, ok := raw["value"].(string); ok { node.Value = value } if attrs, ok := raw["attrs"].(map[string]interface{}); ok { node.Attributes = make(map[string]string) for k, v := range attrs { - node.Attributes[k] = v.(string) + strVal, ok := v.(string) + if !ok { + return TreeNode{}, fmt.Errorf("attribute value must be string, got %T", v) + } + node.Attributes[k] = strVal } } if children, ok := raw["children"].([]interface{}); ok { for _, child := range children { - childNode, err := parseTreeNode(child.(map[string]interface{})) + childMap, ok := child.(map[string]interface{}) + if !ok { + return TreeNode{}, fmt.Errorf("expected map for tree child, got %T", child) + } + childNode, err := parseTreeNode(childMap) if err != nil { return TreeNode{}, err } node.Children = append(node.Children, childNode) } } return node, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/document/yson/to_yson.go(1 hunks)pkg/document/yson/yson.go(5 hunks)pkg/document/yson/yson_test.go(8 hunks)server/packs/packs.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/document/yson/to_yson.go
- server/packs/packs.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: complex-test
- GitHub Check: build
- GitHub Check: bench
🔇 Additional comments (10)
pkg/document/yson/yson_test.go (6)
89-92: LGTM: Updated TreeNode structure correctlyThe TreeNode structure is properly instantiated with the updated fields (Type, Children, Value) matching the new YSON implementation.
112-116: Great use of round-trip validation patternThe new pattern of marshalling, unmarshalling, and comparing against an expected value parsed from YSON syntax is a robust approach for testing serialization.
141-151: LGTM: Properly updated Tree node creationThe TreeNode structure is correctly initialized with the appropriate fields, and the nested structure is maintained.
236-272: Well-designed test for Object marshal/unmarshalThe test properly validates round-trip serialization using the new YSON syntax format. The approach of unmarshalling a string in the test's expected format ensures tests remain clear and maintainable.
274-324: Comprehensive test for Array with mixed typesGood test coverage of all possible element types in arrays, including nested structures, which is crucial for validating the new format.
385-437: Well-structured error handling testsThe table-driven approach to error testing is excellent - it covers various error scenarios and validates the appropriate error messages.
pkg/document/yson/yson.go (4)
144-145: Return formatted error instead of using panicThis is an improvement over previous implementations that would panic for unsupported types.
180-189: LGTM: Counter.Marshal with proper error handlingThe implementation now returns errors instead of panicking for unsupported counter types, which is more robust.
191-207: LGTM: Text.Marshal with proper string escapingThe implementation now correctly uses
strconv.Quotefor proper escaping of string values, addressing previous issues with special characters.
213-234: LGTM: TreeNode.Marshal with proper string escapingThe implementation properly escapes string values with
strconv.Quote, fixing previous issues with special characters.
hackerwins
changed the title
2 participants
What this PR does / why we need it:
Revise YSON string representation
Before:
After:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Documentation