Update MinioClientSettings to UseSsl parsing #943

josemalm32 · 2025-11-01T21:56:46Z

Changes made:

Updated ParseConnectionString to no longer infer UseSsl from https/http URI schemes
Added strict TryParseBool that only accepts boolean values or "true"/"false" strings
Added support for UseSsl key variants (UseSsl, UseSSL, Ssl)
Added 20+ comprehensive tests covering all edge cases
Addressed code review comments: translated comment and improved null handling

Co-authored-by: josemalm32 <23101537+josemalm32@users.noreply.github.com>

…ling Co-authored-by: josemalm32 <23101537+josemalm32@users.noreply.github.com>

josemalm32 · 2025-11-01T21:57:53Z

@dotnet-policy-service agree

Copilot

Pull Request Overview

This PR refactors the MinioClientSettings connection string parsing to introduce explicit handling of the UseSsl property and prevents automatic inference of SSL usage from URI schemes. The key change is that UseSsl now defaults to false and must be explicitly set in connection strings or configuration.

Key changes:

Added explicit UseSsl property handling with support for multiple key variants (UseSsl, UseSSL, Ssl)
Introduced strict boolean parsing that only accepts "true"/"false" strings (case-insensitive), rejecting numeric and yes/no values
Modified connection string parsing to never infer UseSsl from URI schemes (https:// or http://)

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`src/CommunityToolkit.Aspire.Minio.Client/MinioClientSettings.cs`	Refactored `ParseConnectionString` to handle `UseSsl` explicitly with strict boolean parsing; added early return for null/whitespace; added `TryParseBool` helper method
`tests/CommunityToolkit.Aspire.Minio.Client.Tests/ConfigurationTests.cs`	Added comprehensive test coverage for `UseSsl` behavior including default value, explicit true/false, key variants, rejected formats, and non-inference from URI schemes

Comments suppressed due to low confidence (1)

src/CommunityToolkit.Aspire.Minio.Client/MinioClientSettings.cs:100

The method lacks XML documentation comments. All public and internal methods should have XML doc comments per project conventions. Add a summary explaining the strict parsing behavior and document the parameters and return value.

    // Strict boolean parsing: only accepts bool or the strings "true"/"false" (case-insensitive).
    private static bool TryParseBool(object? value, out bool result)

Copilot · 2025-11-01T22:04:37Z

src/CommunityToolkit.Aspire.Minio.Client/MinioClientSettings.cs

    private const string ConnectionStringEndpoint = "Endpoint";
    private const string AccessKey = "AccessKey";
    private const string SecretKey = "SecretKey";
+    private static readonly string[] UseSslKeys = new[] { "UseSsl", "UseSSL", "Ssl" };


Use collection expression syntax instead of array initializer for consistency with project coding style. Change to: private static readonly string[] UseSslKeys = [\"UseSsl\", \"UseSSL\", \"Ssl\"];

Copilot · 2025-11-01T22:04:38Z

src/CommunityToolkit.Aspire.Minio.Client/MinioClientSettings.cs

+        foreach (var key in UseSslKeys)
+        {
+            if (connectionBuilder.TryGetValue(key, out var useSslObj))
            {
-                Credentials = new MinioCredentials
+                if (TryParseBool(useSslObj, out var parsed))
                {
-                    AccessKey = accessKey.ToString()!, SecretKey = secretKey.ToString()!
-                };
+                    UseSsl = parsed;
+                    break;
+                }
            }
        }


This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.

Copilot · 2025-11-01T22:04:38Z

src/CommunityToolkit.Aspire.Minio.Client/MinioClientSettings.cs

+            if (connectionBuilder.TryGetValue(key, out var useSslObj))
            {
-                Credentials = new MinioCredentials
+                if (TryParseBool(useSslObj, out var parsed))
                {
-                    AccessKey = accessKey.ToString()!, SecretKey = secretKey.ToString()!
-                };
+                    UseSsl = parsed;
+                    break;
+                }
            }


These 'if' statements can be combined.

Harold-Morgan · 2025-11-02T20:22:33Z

tests/CommunityToolkit.Aspire.Minio.Client.Tests/ConfigurationTests.cs

+        Assert.Null(settings.Endpoint);
+
+        settings = new MinioClientSettings();
+        settings.ParseConnectionString("");


nit: string.Empty could be more convenient here

src/CommunityToolkit.Aspire.Minio.Client/MinioClientSettings.cs

Harold-Morgan · 2025-11-02T22:23:02Z

PR itself looks good!

I'm a little bit concerned about turning off security features by default, though, is it more convenient?

josemalm32 · 2025-11-02T22:39:56Z

Hi @Harold-Morgan

Thanks for review 💯

If you use Minio locally, it is simpler to have SSL set to false. There is no need to configure self-signed certificates. And I suppose that if it is activated, the Hosting.MinIO package would have to be modified. The idea of adding SSL to the connection string is for production environments. In my case, I tried to connect but never succeeded because I always ended up calling port 80 (non-SSL) instead of 443 (SSL).

Thanks

aaronpowell · 2025-11-03T05:04:28Z

src/CommunityToolkit.Aspire.Minio.Client/MinioClientSettings.cs

    private const string ConnectionStringEndpoint = "Endpoint";
    private const string AccessKey = "AccessKey";
    private const string SecretKey = "SecretKey";
+    private static readonly string[] UseSslKeys = new[] { "UseSsl", "UseSSL", "Ssl" };


The DbConnectionStringBuilder is case insensitive when using TryGetValue: https://lab.razor.fyi/#bc69SgQxFAVgdrVwUy0-QZhqBiQws2VI4ywoYjGw_oCVSTYOkZl7l5tkfxALe99A8H0sfB9LWVRcxPKcjwOHvQwZawhb0r2w4fB5mIKHls82Ibpest0kpjpqUWPfI0jGlpq4DYYrDm7Fp6ZGAGejR5hF8tAeJ9_NHeUFf2Cjv6h4doohKm2sbJCiKqtqIi-DI6U1IchGh7BCmitt1uUP3eNvv8BUVpOMPUrGaoSAnRPX5KM79-ByG4y4oM2Ji1e6Sy7PFt-77Ihjinx7frmVopD_zL9Inu1RgpvRwcfr-9Pb3Xj_drAefAI

We should only care about a single key, UseSsl.

aaronpowell · 2025-11-03T05:05:56Z

src/CommunityToolkit.Aspire.Minio.Client/MinioClientSettings.cs

+        }
+    }
+
+    // Strict boolean parsing: only accepts bool or the strings "true"/"false" (case-insensitive).


Why? bool.TryParse is case insensitive so do we really care? This seems like code for codes sake.

Refactor SSL key handling and remove unused boolean parsing method.

Removed multiple tests for SSL connection string parsing.

aaronpowell · 2025-11-03T23:20:11Z

src/CommunityToolkit.Aspire.Minio.Client/MinioClientSettings.cs

+            {
+            Credentials = new MinioCredentials
+            {
+                AccessKey = accessKey.ToString()!, SecretKey = secretKey.ToString()!


You can simplify this logic by using some pattern matching to avoid having to use ? and !:

https://lab.razor.fyi/#hY5BSsRAEEUZ4kJ7JZ6gDDIkIH2ACdkko6KIDowouLI7dkJD0iXVHTWEHMO93sC1K7feSpIhEBRxVVT9x3_F3j3GVoQFiYpndu_Nq602Bawb61QVsenGl8IJnmJVoYkYexQEmZUQg1FPsJQpGqMyp9GsHWlTJLUu7xUFIbTwM4MY_GPEWESJoFj60EWM6RyCzEp-Rc2JcteirFXQU_4hYO2g9-WIIczn_QRtwW7KcsSB7pP9zY2f2ou6LC_pqHpwTTASIQOAHvvlSQRNPFLQ4JGCJh4p6B_PSIQhtIMqRWOxVPyGtFPn2qjgwM8RF9COL3V-GP2JSkELaMfWAe3OPKrN7c72x8vr51e-u3U3e559Aw

josemalm32 · 2025-11-04T08:09:36Z

Adjusted with the required changes.

Thanks 💯

Harold-Morgan · 2025-11-04T09:33:44Z

Some tests fail and need fixing :(

Apart from that looks great!

Harold-Morgan · 2025-11-04T09:40:33Z

src/CommunityToolkit.Aspire.Minio.Client/MinioClientSettings.cs

+            if (connectionBuilder.TryGetValue(UseSslKey, out var useSslObj))
            {
-                Endpoint = serviceUri;
+                if (bool.TryParse(useSslObj, out var parsed))


It seems that you need to explicitly cast useSslObj to string over here

Fix compilation error and improve readability of SSL parsing.

josemalm32 · 2025-11-04T21:06:48Z

Fix compilation error and improve readability of SSL parsing.

Thanks to both of you😄

aaronpowell

Looks good and we're almost ready to merge

aaronpowell · 2025-11-05T00:08:34Z

tests/CommunityToolkit.Aspire.Minio.Client.Tests/ConfigurationTests.cs

+    public void ParseConnectionString_HttpsUri_DoesNotInferUseSsl()
+    {
+        var settings = new MinioClientSettings();
+        settings.ParseConnectionString("https://minio.example.com:9000");


Since the ParseConnectionString method is internal you'll need to expose it to the test project with <InternalsVisibleTo Include="CommunityToolkit.Aspire.Minio.Client.Tests" />

You'll find a bunch of examples of other projects using InternalsVisibleTo in the repo

aaronpowell · 2025-11-10T23:16:43Z

@josemalm32 can you get this actioned in the next day so it can make the release this week with aspire 13

josemalm32 · 2025-11-11T20:10:04Z

Hi @aaronpowell Sorry for the delay. With the latest change, the tests are working fine. I've tried running it locally and it works.

Thanks 😄

Copilot AI and others added 3 commits November 1, 2025 21:39

Initial plan

1232c27

Make UseSsl parsing strict and avoid inferring from URI scheme

5087708

Co-authored-by: josemalm32 <23101537+josemalm32@users.noreply.github.com>

Address code review feedback: translate comment and improve null hand…

0535933

…ling Co-authored-by: josemalm32 <23101537+josemalm32@users.noreply.github.com>

Copilot AI review requested due to automatic review settings November 1, 2025 21:56

josemalm32 requested a review from Harold-Morgan as a code owner November 1, 2025 21:56

Copilot AI reviewed Nov 1, 2025

View reviewed changes

Harold-Morgan reviewed Nov 2, 2025

View reviewed changes

src/CommunityToolkit.Aspire.Minio.Client/MinioClientSettings.cs Outdated Show resolved Hide resolved

aaronpowell requested changes Nov 3, 2025

View reviewed changes

josemalm32 added 3 commits November 3, 2025 14:06

Refactor MinioClientSettings for SSL key usage

0cef174

Refactor SSL key handling and remove unused boolean parsing method.

Remove redundant SSL parsing tests

637d11a

Removed multiple tests for SSL connection string parsing.

Merge branch 'main' into copilot/fixminio-usesl-strict

20de448

aaronpowell requested changes Nov 3, 2025

View reviewed changes

josemalm32 added 2 commits November 4, 2025 09:07

Refactor access key and secret key retrieval logic

f20210d

Merge branch 'main' into copilot/fixminio-usesl-strict

b204313

Harold-Morgan reviewed Nov 4, 2025

View reviewed changes

Refactor SSL parsing logic in MinioClientSettings

f80d221

Fix compilation error and improve readability of SSL parsing.

aaronpowell requested changes Nov 5, 2025

View reviewed changes

github-actions bot added the Stale label Nov 10, 2025

aaronpowell removed the Stale label Nov 10, 2025

aaronpowell added this to the 13 milestone Nov 11, 2025

Using InternalsVisibleTo to fix test for Minio Client.

b759195

Merge branch 'main' into copilot/fixminio-usesl-strict

bc67f69

aaronpowell approved these changes Nov 12, 2025

View reviewed changes

aaronpowell merged commit fb042c4 into CommunityToolkit:main Nov 12, 2025
108 checks passed

Update MinioClientSettings to UseSsl parsing #943

Update MinioClientSettings to UseSsl parsing #943

Conversation

josemalm32 commented Nov 1, 2025

Uh oh!

josemalm32 commented Nov 1, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Harold-Morgan Nov 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Harold-Morgan commented Nov 2, 2025

Uh oh!

josemalm32 commented Nov 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronpowell Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

aaronpowell Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

aaronpowell Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

josemalm32 commented Nov 4, 2025

Uh oh!

Harold-Morgan commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Harold-Morgan Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

josemalm32 commented Nov 4, 2025

Uh oh!

aaronpowell left a comment

Choose a reason for hiding this comment

Uh oh!

aaronpowell Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

aaronpowell commented Nov 10, 2025

Uh oh!

josemalm32 commented Nov 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

josemalm32 commented Nov 2, 2025 •

edited

Loading

Harold-Morgan commented Nov 4, 2025 •

edited

Loading