chengxilo commented on PR #3015: URL: https://github.com/apache/iggy/pull/3015#issuecomment-4112250135
> > I think it's better to add a test to prevent regression > > we are adding tests in #2973 In this PR, you better only address the problem of `Permissions` > **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() > 0` checks to match Rust SDK's behavior of skipping empty maps. > 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: > **CreateUser** - Fixed capacity calculation that didn't account for individual length prefix bytes. Was allocating `4 + len(username) + len(password)` but needed `1 + len(username) + 1 + len(password) + 1 + 1` for proper byte accounting. Also removed double-counted permissions flag byte. > > **UpdatePermissions** - Fixed panic when writing has_permissions flag. Buffer was allocated as `len(userIdBytes)` without the required 1-byte flag, causing index-out-of-bounds when accessing `bytes[position]`. Now allocates `len(userIdBytes) + 1`. > > **UpdateUser** - Fixed panic from insufficient buffer allocation (missing flag bytes) and removed input mutation side effect. Method was modifying caller's struct by allocating `new(string)` when Username was nil. Now uses read-only access with proper buffer sizing: `len(userIdBytes) + 2` for both flags. Regarding this part > Note: Does NOT fix https://github.com/apache/iggy/issues/2980, https://github.com/apache/iggy/issues/2981, https://github.com/apache/iggy/issues/2982 (missing 4-byte permissions_len field before permissions data - those are separate issues found during the review of https://github.com/apache/iggy/pull/2973). 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^. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
