-
Notifications
You must be signed in to change notification settings - Fork 132
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
## Summary of changes - The `IApiResponse.ContentEncoding` property was confusing and incorrectly implemented - Refactored as `GetCharsetEncoding()` - Added `GetContentEncodingType` ## Reason for change The `ContentEncoding` property was meant to return [the `charset` associated with a `Content-type`](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Type#charset) i.e. the `charset=utf-8` part of `text/plain;charset=utf-8`, converted into a .NET `Encoding` object. However, there's also [a `ContentEncoding` _header_](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Encoding) which defines whether the content is encoded with a compression algorithm, e.g. gzip/brotli etc. This PR aims to fix the bug, clear up the confusion by renaming, add some unit tests, and make the behaviour consistent across our various `IApiResponse` implementations. ## Implementation details - Expose `ContentTypeHeader` and `ContentEncodingHeader` as values on the `IApiResponse`. - This is necessary because you _can't_ necessarily get these values directly from `GetHeaders` in some cases (e.g. `HttpClient`) - Rename `ContentEncoding` to `GetCharsetEncoding()` - Made it a method so we don't bother processing the header until we need it. In most cases we never use it (and use it at most once) - Re-used the optimized implementation we were using in `HttpMessage` where possible. Tweaked it slightly to always return UTF-8 by default (as our calling code wasn't resistant to it anyway and would have thrown) - Made it fallback to "any" charset, as was previously _always_ happening for `HttpClientResponse` - Add `GetContentEncodingType()` which returns an enum of compression values - We don't currently use this, but I could do with it in #5747 ## Test coverage Added unit tests for the parsing logic used for decoding the `charset` and the content-encoding ## Other details I initially based this on #5658 but ultimately changed a lot (and we still don't need that mime-type handling yet) <!--⚠️ Note: where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. -->
- Loading branch information
1 parent
fab003e
commit dae1d15
Showing
13 changed files
with
405 additions
and
69 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
// <copyright file="ContentEncodingType.cs" company="Datadog"> | ||
// Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License. | ||
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc. | ||
// </copyright> | ||
|
||
#nullable enable | ||
|
||
namespace Datadog.Trace.Agent; | ||
|
||
/// <summary> | ||
/// The encoding used on the content body. | ||
/// <see href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Encoding">Mozilla</see> for the content-encoding header | ||
/// </summary> | ||
internal enum ContentEncodingType | ||
{ | ||
/// <summary> | ||
/// The Content-Encoding header is not present or was empty | ||
/// </summary> | ||
None, | ||
|
||
/// <summary> | ||
/// The Content-Encoding header specified 'gzip' | ||
/// </summary> | ||
GZip, | ||
|
||
/// <summary> | ||
/// The Content-Encoding header specified 'deflate' | ||
/// </summary> | ||
Deflate, | ||
|
||
/// <summary> | ||
/// The Content-Encoding header specified 'compress' | ||
/// </summary> | ||
Compress, | ||
|
||
/// <summary> | ||
/// The Content-Encoding header specified 'br' | ||
/// </summary> | ||
Brotli, | ||
|
||
/// <summary> | ||
/// The Content-Encoding header was not recognized | ||
/// </summary> | ||
Other, | ||
|
||
/// <summary> | ||
/// The Content-Encoding header indicated multiple headers were specified | ||
/// </summary> | ||
Multiple, | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.