Initial subscription decompression functionality #1408
Conversation
| if (contentEncoding is not null) | ||
| { | ||
| var decompressor = _decompressionRegistry.GetDecompressor(contentEncoding.StringValue); | ||
| // TODO What to do when decompressor not found? |
There was a problem hiding this comment.
This created some interesting discussion, as my instinct is to throw here, if we see there is a Content-Encoding mentioning a compression we weren't expecting, however @hwoodiwiss pointed out that this could be a header someone is already legitimately using for other purposes, and we just broke them.
We could have some sort of opt-in behaviour, and if the default "no compression" configuration is detected then skip this block entirely. I was thinking we just had it on for subscription by default, but we might want to make that a major or minor version bump.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1408 +/- ##
==========================================
+ Coverage 79.09% 79.16% +0.06%
==========================================
Files 138 141 +3
Lines 3234 3264 +30
Branches 448 449 +1
==========================================
+ Hits 2558 2584 +26
- Misses 445 449 +4
Partials 231 231
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
|
||
| namespace JustSaying.AwsTools.MessageHandling.Compression; | ||
|
|
||
| class GzipMessageBodyCompressor : IMessageBodyCompressor |
There was a problem hiding this comment.
Unless you intended to make it public:
| class GzipMessageBodyCompressor : IMessageBodyCompressor | |
| internal sealed class GzipMessageBodyCompressor : IMessageBodyCompressor |
src/JustSaying/AwsTools/MessageHandling/Compression/GzipMessageBodyCompressor.cs
Show resolved
Hide resolved
|
|
||
| namespace JustSaying.AwsTools.MessageHandling.Compression; | ||
|
|
||
| class GzipMessageBodyDecompressor : IMessageBodyDecompressor |
There was a problem hiding this comment.
| class GzipMessageBodyDecompressor : IMessageBodyDecompressor | |
| internal sealed class GzipMessageBodyDecompressor : IMessageBodyDecompressor |
|
|
||
| class GzipMessageBodyCompressor : IMessageBodyCompressor | ||
| { | ||
| public string ContentEncoding { get; } = "gzip,base64"; |
There was a problem hiding this comment.
| public string ContentEncoding { get; } = "gzip,base64"; | |
| public string ContentEncoding { get; } = "gzip,base64"; | |
| internal interface IMessageBodyCompressor | ||
| { | ||
| string ContentEncoding { get; } | ||
| string Compress(string messageBody); |
There was a problem hiding this comment.
Can we make this async, or do we not have access to the required async methods the implementations use in all our TFMs?
There was a problem hiding this comment.
Fair point, I was thinking it should be sync because we are writing to and from in-memory streams. No strong opinion here from me.
There was a problem hiding this comment.
True, but if it's pluggable and we could make it async, it would enable async if for some reason it was being backed by a file or some other IO.
| @@ -2,6 +2,7 @@ | |||
| <PropertyGroup> | |||
| <IsShipping>true</IsShipping> | |||
| <TargetFrameworks>netstandard2.0;net461;net8.0</TargetFrameworks> | |||
| <NoWarn>$(NoWarn);RS0016;RS0017</NoWarn> | |||
There was a problem hiding this comment.
I assume this is being deleted post-draft?
...stSaying.UnitTests/AwsTools/MessageHandling/MessageDispatcherTests/WhenDispatchingMessage.cs
Outdated
Show resolved
Hide resolved
...JustSaying.UnitTests/Messaging/Channels/SubscriptionGroupTests/BaseSubscriptionGroupTests.cs
Outdated
Show resolved
Hide resolved
| var memoryStream = new MemoryStream(); | ||
| await using (var gzipStream = new GZipStream(memoryStream, CompressionMode.Compress)) | ||
| { | ||
| gzipStream.Write(Encoding.UTF8.GetBytes(payload)); | ||
| } |
There was a problem hiding this comment.
We should add some unit tests for the (de)compression implementations.
…ompressionRegistry.cs Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
…patcherTests/WhenDispatchingMessage.cs Co-authored-by: Martin Costello <martin@martincostello.com>
…pTests/BaseSubscriptionGroupTests.cs Co-authored-by: Martin Costello <martin@martincostello.com>
slang25
changed the title
No description provided.