Refactor response handling and update project structure #131
Conversation
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
Refactored response handling to improve clarity and consistency: - Renamed `DefaultMimeType` to `DefaultContentType`. - Removed `CustomResponseBuilder` parameter from `RequestData` constructor. - Introduced `HttpRequestInvoker` class and refactored constructors. - Updated `RequestCoreAsync` to use `contentType` instead of `mimeType`. - Added `ResponseFactory` property to `IRequestInvoker` interface. - Refactored `DefaultResponseBuilder` to implement `IResponseBuilder`. - Simplified response deserialization logic. Updated project structure and configurations: - Added new project `Elastic.Transport.Tests.Shared`. - Updated solution file and project references. - Enabled nullable reference types in several files. - Removed `StreamResponseTests.cs` and `ResponseBuilderDisposeTests.cs`. - Added new test classes for `ResponseFactory`, `DynamicResponseBuilder`, and `BytesResponseBuilder`. Enhanced error handling and documentation: - Removed `EmptyError` class. - Updated `ApiCallDetails` to use nullable reference types. - Improved method signatures and internal logic for consistency. - Added license information to several files.
A new property `ResponseContentType` has been added to the `ApiCallDetails` class. This reintroduces the original `ResponseMimeType` property marked as obsolete and serves as a temporary alias for the existing `ResponseContentType` property.
Mpdreamz
requested changes
Nov 7, 2024
tests/Elastic.Transport.IntegrationTests/Responses/SpecialisedResponseTests.cs
Outdated
Show resolved
Hide resolved
flobernd
reviewed
Nov 7, 2024
| @@ -115,16 +112,19 @@ public TransportConfiguration( | |||
| /// Expert usage: Create a new transport configuration based of a previously configured instance | |||
| public TransportConfiguration(ITransportConfiguration config) | |||
| { | |||
| #if NET8_0_OR_GREATER | |||
There was a problem hiding this comment.
nit: Let's implement a helper function for argument validation like suggested in #120.
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static void NotNull([NotNull] object? obj, [CallerArgumentExpression(nameof(obj))] string? paramName = null)
{
#if NET
ArgumentNullException.ThrowIfNull(obj, paramName);
#else
if (obj is null)
{
throw new ArgumentNullException(paramName);
}
#endif
}There was a problem hiding this comment.
Yep, that'll be an excellent follow-up.
| /// </remarks> | ||
| internal sealed class DefaultResponseFactory(ITransportConfiguration transportConfiguration) : ResponseFactory | ||
| { | ||
| private readonly ConcurrentDictionary<Type, IResponseBuilder> _resolvedBuilders = new() |
There was a problem hiding this comment.
The builder instances in the dictionary could be static I think - they don't seem to maintain a state (maybe the DefaultResponseFactory itself is a static singleton - in this case just ignore my comment; I did not check that).
There was a problem hiding this comment.
Yeah, I guess we could have a static Default instance of them. However, we create them once, and the factory itself is only realistically expected to be created once by whichever IRequestInvoker is used for the transport.
Mpdreamz
approved these changes
Nov 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactored response handling to improve clarity and consistency:
DefaultMimeTypetoDefaultContentType.CustomResponseBuilderparameter fromRequestDataconstructor.RequestCoreAsyncto usecontentTypeinstead ofmimeType.ResponseFactoryproperty toIRequestInvokerinterface.DefaultResponseBuilderto implementIResponseBuilder.Updated project structure and configurations:
Elastic.Transport.Tests.Shared.StreamResponseTests.csandResponseBuilderDisposeTests.cs.ResponseFactory,DynamicResponseBuilder, andBytesResponseBuilder.Enhanced error handling and documentation:
EmptyErrorclass.ApiCallDetailsto use nullable reference types.