spec: Don't expect integers to round-trip through msg.extras #435
Conversation
WalkthroughThe recent changes involve a modification to the test data structure in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- spec/acceptance/realtime/message_spec.rb (1 hunks)
Additional comments not posted (1)
spec/acceptance/realtime/message_spec.rb (1)
176-176: Verify the impact of changing integer to float in test data.Changing the integer
55to a float55.1in the test data aligns with the PR objective to handle numeric values as floats. Ensure that this change is reflected in all relevant tests and does not cause unintended side effects.Run the following script to verify the impact of this change on the test behavior:
Verification successful
Change from integer to float is adequately tested and verified.
The modification of the test data to include a float instead of an integer is verified by the test case, which checks for consistent encoding and decoding. This ensures that the change aligns with the PR's objective and does not introduce unintended side effects.
- The test
it 'is encoded and decoded to the same Array'confirms the correct handling of the float value.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the change from integer to float in test data does not cause unintended side effects. # Test: Search for the usage of the modified data structure. Expect: Consistent handling of numeric values as floats. rg --type ruby -A 5 $'let(:data) { { \'push\' => { \'data\' => { \'key\' => [ true, false, 55.1, nil, \'string\', { \'Hash\' => true }, [\'array\'] ] } } } }'Length of output: 402
Script:
#!/bin/bash # Description: Verify that the change from integer to float in test data does not cause unintended side effects. # Corrected Test: Search for the usage of the modified data structure. Expect: Consistent handling of numeric values as floats. rg --type ruby -A 5 "let\(:data\) \{ \{ 'push' => \{ 'data' => \{ 'key' => \[ true, false, 55\.1, nil, 'string', \{ 'Hash' => true \}, \['array'\] \] \} \} \} \}"Length of output: 698
There was a problem hiding this comment.
I'm not entirely sure about this, probably not a big deal for frontdoor websockets but will this affect extras when publishing push notifications through frontdoors? for example we have live docs which specify an int value in extras here, is it safe to assume this will all still work correctly if we stop enforcing that ints survive protobuf encoding?
|
@owenpearson so this only affects msgpack encoding, JSON doesn't have different number types so whether we store an integer as a float or int internally, in JSON it will encode the same (and specifically, this means it won't affect push notifications since they will be encoded as JSON when we send them to the push platform, similarly for integrations), so I don't think this will impact anything other than clients which subscribe using msgpack always getting numbers as floats in msgpack-encoded extras. |
Internally, we store msg.extras in a protobuf.Struct which stores a JSON number in a double field: https://github.com/protocolbuffers/protobuf/blob/5cbf13bc9ab0f883bcb664241f6be8d962915569/src/google/protobuf/struct.proto#L56-L78 This means that when a connection is using binary encoding (i.e. msgpack), any numbers in msg.extras are encoded as floats even if the inbound message encoded the number as an int type, so the inbound and outbound representations are not guaranteed to be the same, although the semantic numeric value will be the same. This commit handles this nuance by not expecting an integer to come back as an integer in msg.extras, but expecting a float to instead. Signed-off-by: Lewis Marshall <[email protected]>
lmars
force-pushed
the
msg-extras-float
branch
from
7840544
to
ca52a1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- spec/acceptance/realtime/message_spec.rb (1 hunks)
Additional comments not posted (1)
spec/acceptance/realtime/message_spec.rb (1)
176-176: The code change looks good.Changing the type of a value in the test data from integer to float should not impact the behavior of the test. The test is only concerned with encoding and decoding the data, not the specific types of the array elements.
Internally, we store msg.extras in a protobuf.Struct which stores a JSON number in a double field:
https://github.com/protocolbuffers/protobuf/blob/5cbf13bc9ab0f883bcb664241f6be8d962915569/src/google/protobuf/struct.proto#L56-L78
This means that when a connection is using binary encoding (i.e. msgpack), any numbers in msg.extras are stored as floats even if the inbound message encoded the number as an int type, so the inbound and outbound representations are not guaranteed to be the same, although the semantic numeric value will be the same.
This commit handles this nuance by not expecting an integer to come back as an integer in msg.extras, but expecting a float to instead.
Summary by CodeRabbit
Bug Fixes
Tests