fix(go): correct Permissions serialization and add unit tests#3015
fix(go): correct Permissions serialization and add unit tests#3015saie-ch wants to merge 8 commits intoapache:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3015 +/- ##
============================================
+ Coverage 71.88% 71.95% +0.06%
Complexity 930 930
============================================
Files 1118 1120 +2
Lines 92992 93179 +187
Branches 70513 70513
============================================
+ Hits 66851 67043 +192
+ Misses 23582 23578 -4
+ Partials 2559 2558 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@chengxilo could you please check this? |
|
PR description is not good enough. please make it more meaningful: what bug was that, how did it manifest and how was it fixed. prefer on describing why instead of what. |
ryankert01
left a comment
ryankert01
left a comment
There was a problem hiding this comment.
I think it's better to add a test to prevent regression
we are adding tests in #2973 |
|
@saie-ch so why these tests passed? do those commits overlap? |
@hubcio Thanks for the question! Could you clarify which tests you're referring to? |
In this PR, you better only address the problem of
For the following changes related to commands, it would be better to address them in #2973, since they were discovered while you implementing command tests(and more importantly, they are logic of commands):
Regarding this part
I’m still a bit confused why we tend to separate bug fixes and tests into different PRs. In many cases, writing tests alongside the fix makes it easier to validate the implementation immediately ^v^. |
Sorry for making you confused, actually you just need to use a seperate PR to solve the problem of |
Also I think the tests @hubcio refering to is the tests you implemented for command. As those tests passed it bascially means they didn't detect the bug in |
|
@chengxilo I misunderstood the original review feedback and thought the suggestion was to separate ALL bug fixes from the tests. Now: |
chengxilo
left a comment
chengxilo
left a comment
There was a problem hiding this comment.
Ohter part looks good to me
| func TestPermissions_MarshalBinary_WithStreamsAndTopics(t *testing.T) { | ||
| // Test case: Permissions with 2 streams, first stream has 2 topics, second has none | ||
| permissions := &Permissions{ | ||
| Global: GlobalPermissions{ | ||
| ManageServers: true, | ||
| ReadServers: false, | ||
| ManageUsers: true, | ||
| ReadUsers: false, | ||
| ManageStreams: true, | ||
| ReadStreams: false, | ||
| ManageTopics: true, | ||
| ReadTopics: false, | ||
| PollMessages: true, | ||
| SendMessages: false, | ||
| }, | ||
| Streams: map[int]*StreamPermissions{ | ||
| 1: { | ||
| ManageStream: true, | ||
| ReadStream: false, | ||
| ManageTopics: true, | ||
| ReadTopics: false, | ||
| PollMessages: true, | ||
| SendMessages: false, | ||
| Topics: map[int]*TopicPermissions{ | ||
| 10: { | ||
| ManageTopic: true, | ||
| ReadTopic: false, | ||
| PollMessages: true, | ||
| SendMessages: false, | ||
| }, | ||
| 20: { | ||
| ManageTopic: false, | ||
| ReadTopic: true, | ||
| PollMessages: false, | ||
| SendMessages: true, | ||
| }, | ||
| }, | ||
| }, | ||
| 2: { | ||
| ManageStream: false, | ||
| ReadStream: true, | ||
| ManageTopics: false, | ||
| ReadTopics: true, | ||
| PollMessages: false, | ||
| SendMessages: true, | ||
| Topics: nil, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| bytes, err := permissions.MarshalBinary() | ||
| if err != nil { | ||
| t.Fatalf("MarshalBinary failed: %v", err) | ||
| } | ||
|
|
||
| // Verify structure | ||
| position := 0 | ||
|
|
||
| // Global permissions (10 bytes) | ||
| if bytes[position] != 1 { | ||
| t.Errorf("Expected ManageServers=1, got %d", bytes[position]) | ||
| } | ||
| position += 10 | ||
|
|
||
| // Has streams flag | ||
| if bytes[position] != 1 { | ||
| t.Errorf("Expected has_streams=1, got %d", bytes[position]) | ||
| } | ||
| position++ | ||
|
|
||
| // Verify continuation flags are present for streams | ||
| // We should have 2 streams, so we need to find stream continuation flags | ||
| streamsFound := 0 | ||
| for position < len(bytes) { | ||
| // Each stream has: 4 bytes (ID) + 6 bytes (perms) + 1 byte (has_topics) + topics + 1 byte (has_next_stream) | ||
| if position+4 > len(bytes) { | ||
| break | ||
| } | ||
| streamID := binary.LittleEndian.Uint32(bytes[position : position+4]) | ||
| if streamID == 0 { | ||
| break | ||
| } | ||
| position += 4 // stream ID | ||
| position += 6 // stream permissions | ||
| position += 1 // has_topics flag | ||
|
|
||
| // Skip topics if present | ||
| if bytes[position-1] == 1 { | ||
| // Topics exist, need to skip them | ||
| for position+4 <= len(bytes) { | ||
| position += 4 // topic ID | ||
| position += 4 // topic permissions | ||
| if position >= len(bytes) { | ||
| t.Fatalf("Unexpected end of bytes while reading topic continuation flag") | ||
| } | ||
| hasNextTopic := bytes[position] | ||
| position++ | ||
| if hasNextTopic == 0 { | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check stream continuation flag | ||
| if position >= len(bytes) { | ||
| t.Fatalf("Unexpected end of bytes while reading stream continuation flag") | ||
| } | ||
| hasNextStream := bytes[position] | ||
| position++ | ||
| streamsFound++ | ||
|
|
||
| if streamsFound == 1 && hasNextStream != 1 { | ||
| t.Errorf("Expected has_next_stream=1 for first stream, got %d", hasNextStream) | ||
| } | ||
| if streamsFound == 2 && hasNextStream != 0 { | ||
| t.Errorf("Expected has_next_stream=0 for second stream, got %d", hasNextStream) | ||
| } | ||
|
|
||
| if hasNextStream == 0 { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if streamsFound != 2 { | ||
| t.Errorf("Expected 2 streams, found %d", streamsFound) | ||
| } | ||
| } |
There was a problem hiding this comment.
@saie-ch The test should verify that each attribute of Permissions is encoded correctly. Right now, those checks are being skipped. This only validates the basic frame structure (e.g., continuation flags and the size of each part). It would be better to also read back the encoded content and confirm that it matches the values you originally provided.
|
Btw would you mind to update the PR title, maybe |
saie-ch
changed the title
| func TestPermissions_MarshalBinary_WithStreamsAndTopics(t *testing.T) { | ||
| // Test case: Permissions with 2 streams, first stream has 2 topics, second has none | ||
| permissions := &Permissions{ | ||
| Global: GlobalPermissions{ | ||
| ManageServers: true, | ||
| ReadServers: false, | ||
| ManageUsers: true, | ||
| ReadUsers: false, | ||
| ManageStreams: true, | ||
| ReadStreams: false, | ||
| ManageTopics: true, | ||
| ReadTopics: false, | ||
| PollMessages: true, | ||
| SendMessages: false, | ||
| }, |
There was a problem hiding this comment.
I'm sorry that I keep asking you to modify your code.
However, your changes actually doesn't address the problem. If in the future I swaped the position of ManageServers and ManageUsers when encoding, this test won't know I am wrong.
To better desribe the problem, consider you have this struct.
Foo {
A bool,
B bool,
C bool,
}You wrot a function to encode it.
func (f Foo) MarshalBinary() ([]byte, error) {
return []byte{
boolToByte(f.A),
boolToByte(f.B),
boolToByte(f.C),
}
}You wrote a test and here is your test case:
input: Foo {
A: true,
B: false,
C: true
}
want: []byte {1, 0, 1}And you can get a []byte {1, 0, 1}, you are 100% correct.
However, if I write the function like this:
func (f Foo) MarshalBinary() ([]byte, error) {
return []byte{
boolToByte(f.C),
boolToByte(f.B),
boolToByte(f.A),
}
}I will still get []byte {1, 0, 1}, I can still pass the test, but actually it's wrong.
So you probably want to make sure every single field are correctly encoded.
Consider make a bigger test case:
{
input: Foo {
A: true
},
want: []byte{1,0,0}
},
{
input: Foo {
B: true
},
want: []byte{0,1,0}
},
{
input: Foo {
C: true
},
want: []byte{0,0,1}
}There was a problem hiding this comment.
No problem @chengxilo, happy to modify until it's correct. Can you check now?
| 10: { | ||
| ManageTopic: true, | ||
| ReadTopic: false, | ||
| PollMessages: true, | ||
| SendMessages: false, | ||
| }, | ||
| 20: { | ||
| ManageTopic: false, | ||
| ReadTopic: true, | ||
| PollMessages: false, | ||
| SendMessages: true, | ||
| }, | ||
| }, | ||
| }, |
|
@saie-ch please use precommit hooks via |
4 participants
Bugs Fixed:
Permissions.MarshalBinary() - Fixed missing continuation flags between stream/topic entries. The serializer wasn't writing 1-byte flags
(1=has_next, 0=no_next) after each entry, causing the server to incorrectly parse permission data and potentially grant wrong permissions. Also
added
len() > 0checks to match Rust SDK's behavior of skipping empty maps.Note: Does NOT fix #2980, #2981, #2982 (missing 4-byte permissions_len field before permissions data - those are separate issues found during the review of #2973).