fix (#2061): used struct.pack to encode headers length into binary #2287
Conversation
|
|
|
Is it correct way to deprecate JSON format? Or should it use JSON by default and fallback to new format in case of parsing error? |
faststream/redis/parser.py
Outdated
| parsed_data = json_loads(data) | ||
| data = parsed_data["data"].decode() | ||
| headers = parsed_data["headers"] | ||
| warnings.warn( |
There was a problem hiding this comment.
I don't think, that we should show deprecation message here. Users do not select the format they consume.
faststream/redis/parser.py
Outdated
| @@ -90,31 +149,72 @@ def encode( | |||
| reply_to: Optional[str], | |||
| headers: Optional["AnyDict"], | |||
| correlation_id: str, | |||
| to_json: bool = False, | |||
There was a problem hiding this comment.
This is not public API object, should we add json-encoder option here?
I think, we can use just a binary format
docs/docs/en/redis/message_format.md
Outdated
| [Number of headers: 16 bit big-endian int] | ||
| # headers | ||
| [Header key length: 16 bit big-endian int] | ||
| [Header key: UTF-8 string] |
There was a problem hiding this comment.
Look like the format is incorrect. We should use [key:value, key:value] instead of [key, key, value, value].
Also, please use the same words everywhere: 16 bit big-endian int or 16 big-endian bit int
docs/docs/en/redis/message_format.md
Outdated
| @@ -0,0 +1,240 @@ | |||
| # Dealing with message encoded by FastStream | |||
|
|
|||
| To provide such great features as observability and many others **FastStream** needs to add extra data to your message, but suddenly **Redis** doesn't have any option to send it except the message itself. Since that, **FastStream** uses it's own binary format for messages that supports any type of data that you are going to use. | |||
There was a problem hiding this comment.
We should note, that raw messages are OK as well, but they doesn't support headers-based features
| from tests.brokers.base.parser import CustomParserTestcase | ||
|
|
||
|
|
||
| @pytest.mark.redis | ||
| class TestCustomParser(CustomParserTestcase): | ||
| broker_class = RedisBroker | ||
|
|
||
|
|
||
| @pytest.mark.redis |
There was a problem hiding this comment.
This marker should be used only if test require connection to Redis
tests/brokers/redis/test_parser.py
Outdated
|
|
||
|
|
||
| @pytest.mark.redis | ||
| class TestRedisRawMessage: |
There was a problem hiding this comment.
Current test can be just a function as I can see
|
@Lancetnik please review |
Description
Fixes bug of serialization/deserialization of binary data. Logic of working with plain text and json remains the same so as not to break existing API. But when user provides binary data, 4-bytes unsigned int is added to indicate the length of headers needed by FastStream.
May be it's needed to add new type to ContentType enum to make this solution more verbose
Fixes #2061
Type of change
Please delete options that are not relevant.
Checklist
scripts/lint.shshows no errors)scripts/test-cov.shscripts/static-analysis.sh