Add More Peek API Variants for ByteBuffer #3174
Conversation
Existing get APIs require passing an explicit index and can be misused, leading to verbose and error-prone code. Adding peek variants that automatically use the current readerIndex improves safety and clarity. This aims to address issue apple#2034 and issue apple#2736, and is a continuation of PR apple#3157 Modifications: Introduced peekSlice(), peekLengthPrefixedSlice(), peekData(), peekUUIDBytes() and peekWebSocketErrorCode(). Added tests for each peek API covering normal, empty and repeated peek scenarios. Result: Developers can now use nonmutating peek APIs to inspect ByteBuffer contents without altering the reader index.
|
Just adding some more peek*s for some more get*s I found, I'm not sure if any more are remaining apart from multiple integers. Weirdly, I was unable to get the peekWebSocketErrorCode() Api tests to run, and then I realised there are no tests written for any of WebSocketErrorCode APIs? I could be wrong, but because of that I have skipped adding tests for that. |
RaghavRoy145
force-pushed
the
peek-remaining
branch
from
576df99 to
061352f
Compare
There was a problem hiding this comment.
Thanks @RaghavRoy145! Left one comment.
About the WebSocketErrorCode yeah, that's a strange oversight. We should get that fixed up. You could try adding tests just for the peek method if you want, but otherwise we can tackle adding tests for all the related APIs separately.
| guard let lengthPrefix = self.getInteger(at: self.readerIndex, endianness: endianness, as: Integer.self), | ||
| let messageLength = Int(exactly: lengthPrefix), | ||
| let messageBuffer = self.getSlice( | ||
| at: self.readerIndex + MemoryLayout<Integer>.size, | ||
| length: messageLength | ||
| ) | ||
| else { | ||
| return nil | ||
| } | ||
| return messageBuffer |
There was a problem hiding this comment.
Can't we just call getLengthPrefixedSlice here?
There was a problem hiding this comment.
Yeah I thought about that, but I went with this approach just because of how getLengthPrefixedSlice is implemented, it's doing multiple things. It reads an integer prefix, then reads a slice relative to that prefix, fails on some inputs (negative or out-of-bounds), etc. So it didnt seem as safe as say implementing a peek variant of getUUID which is predictable and doesn't have side-effects, so I could just create a wrapper for it.
If however it's more reasonable to simply call getLengthPrefixedSlice, I can change it to that. I can see how the current version is just duplicate code that needs to be separately maintained (unless ofcourse get*s will eventually be phased out anyway!)
There was a problem hiding this comment.
I think this should probably just call getLengthPrefixedSlice.
There was a problem hiding this comment.
Cory beat me to it but yes I think it's better to just call getLengthPrefixedSlice. Copying the code doesn't really make e.g. the input validation any more obvious; and it makes maintainability potentially harder as we now have the same code in multiple places as you rightly pointed out.
There was a problem hiding this comment.
Awesome, I'll add this in
Got it, however, when I try to write tests for the peek API, it's unable to find the definition of my implementation, and I'm guessing that's because of how imports are handled in this file. I didn't want to add any additional dependencies here so I skipped it seeing how none of the other WebSocketErrorCode APIs are tested, can you help me out with this if its needed? swift-nio/Tests/NIOCoreTests/ByteBufferTest.swift:3868:27: error: value of type 'ByteBuffer' has no member 'peekWebSocketErrorCode' |
|
I recommend putting those tests into NIOWebSocketTests, as you need the import of NIOWebSocket. |
Sure, since there doesn't exist a file meant for ByteBuffer tests for WebSocketAPIs, I'll create one in the directory. This can then be used to add the tests for the other APIs as well that don't exist yet |
Thanks for the review! I think these should be all of the peek*s left to implement, but if there's more get*s lurking around that need wrapping up, happy to help |
|
Thanks a bunch @RaghavRoy145! |
The write(webSocketErrorCode:), readWebSocketErrorCode(), and getWebSocketErrorCode(at:) methods in ByteBuffer were not covered by unit tests apple#3174 (review). Since a peekWebSocketErrorCode() method was recently added, I figured I should verify both the new peek API and the existing WebSocket error code APIs behave correctly. Added tests for: write(webSocketErrorCode:) getWebSocketErrorCode(at:) readWebSocketErrorCode() Tests verify: Correct encoding and decoding of WebSocket error codes Reader index behavior (no movement for get, movement for read) Handling of insufficient bytes Consistency between peek-before-read and post-read These tests ensure the WebSocket error code APIs in ByteBuffer are correct, and safe to use.
The write(webSocketErrorCode:), readWebSocketErrorCode(), and getWebSocketErrorCode(at:) methods in ByteBuffer were not covered by unit tests apple#3174 (review). Since a peekWebSocketErrorCode() method was recently added, I figured I should verify both the new peek API and the existing WebSocket error code APIs behave correctly. Added tests for: write(webSocketErrorCode:) getWebSocketErrorCode(at:) readWebSocketErrorCode() Tests verify: Correct encoding and decoding of WebSocket error codes Reader index behavior (no movement for get, movement for read) Handling of insufficient bytes Consistency between peek-before-read and post-read These tests ensure the WebSocket error code APIs in ByteBuffer are correct, and safe to use.
…er (#3198) ### Motivation The `write(webSocketErrorCode:)`, `readWebSocketErrorCode()`, and `getWebSocketErrorCode(at:)` methods in ByteBuffer were not covered by unit tests #3174 (review). Since a peekWebSocketErrorCode() method was recently added, I figured I should verify both the new peek API and the existing WebSocket error code APIs behave correctly. ### Modifications Added tests for: write(webSocketErrorCode:) getWebSocketErrorCode(at:) readWebSocketErrorCode() Tests verify: Correct encoding and decoding of WebSocket error codes Reader index behavior (no movement for get, movement for read) Handling of insufficient bytes Consistency between peek-before-read and post-read ### Result These tests ensure the WebSocket error code APIs in ByteBuffer are correct, and safe to use. Co-authored-by: Cory Benfield <[email protected]>
Motivation:
Existing get APIs require passing an explicit index and can be misused, leading to verbose and error-prone code. Adding peek variants that automatically use the current readerIndex improves safety and clarity. This aims to address issue #2034 and issue #2736, and is a continuation of PR #3157
Modifications:
Result:
Developers can now use nonmutating peek APIs to inspect ByteBuffer contents without altering the reader index.