#940 upgrade papercut and changed portnumbers

[anoordover]

Closes #940

PR Checklist

• Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
• Based off latest main branch of toolkit
• PR doesn't include merge commits (always rebase on top of our main, if needed)
• New integration
  • Docs are written
  • Added description of major feature to project description for NuGet package (4000 total character limit, so don't push entire description over that)
• Tests for the changes have been added (for bug fixes / features) (if applicable)
• Contains NO breaking changes
• Every new API (including internal ones) has full XML docs
• Code follows all style conventions

Other information

@mguinness thanks for the pointer to the changed ports.
I don't know if this is a breaking change for this component.

[mguinness]

LGTM, I did some testing and revising the ports worked as expected.

The only problem I found was that I had to replace httpPort with (int)HttpStatusCode.OK in the following:

Aspire/src/CommunityToolkit.Aspire.Hosting.PapercutSmtp/PapercutSmtpHostingExtension.cs

Line 38 in 4fc8f80

.WithHttpHealthCheck("/health", httpPort, PapercutSmtpContainerResource.HttpEndpointName);

Also do you think it's worth adding some example code for the connection string to README?

var connStr = config.GetConnectionString("papercut");
var uri = new Uri(connStr[9..]);
var sc = new SmtpClient(uri.Host, uri.Port);

[anoordover]

@mguinness thanks for pointing out this bug which might have been present from the start of this component.
I remove the status parameter from the call because the default value is 200.
@mguinness how did you notice this bug? Can I add a unittest to test this?
Do you have any pointers for me?

[anoordover]

@mguinness about your second note: In my example project I used the DbConnectionStringBuilder and added this code to the readme.md.

[mguinness]

I noticed the bug when I changed AddPapercutSmtp to specify ports.

var papercut = builder.AddPapercutSmtp("papercut", 8080, 2525)

Then health check then failed, but I'm not sure why it didn't occur when httpPort variable was 80.

The message in UriHealthCheck should be returned when status code 80 is incorrectly expected. @sungam3r Any ideas?

[sungam3r]

The message in UriHealthCheck should be returned when status code 80 is incorrectly expected.

What do you mean?

[anoordover]

Ah, now I see why my test doesn't fail. When you don't override the httpPort this parameter will be null on the line you mentioned @mguinness and the default httpStatus 200 will be used. Should I create two papercut resources in my example project and use them both in my unittest so as to make sure that setting the optional parameters won't break the healthcheck?
What do you think @mguinness ?

[mguinness]

What do you mean?

Apologies, I wasn't very clear. I suppose the question should be whether bounds checking occur in ExpectedHttpCodes to ensure valid status code >= 100 and <=599 in UriHealthCheckOptions before httpClient.SendAsync is called?

The check could also be done in ResourceBuilderExtensions and throw a ArgumentOutOfRangeException if statusCode is out of range. Or more simply change the statusCode parameter from int to HttpStatusCode.

Should I create two papercut resources in my example project and use them both in my unittest so as to make sure that setting the optional parameters won't break the healthcheck?

I don't think that is necessary since you have now removed the statusCode parameter from WithHttpHealthCheck.

[anoordover]

@mguinness I have a local change ready to commit that I can push if we want to test whether setting the portnumbers on the papercut host will effect the healthcheck.

var papercut = builder.AddPapercutSmtp(PapercutConstants.ResourceNameDefaultParameters);
var papercut2 = builder.AddPapercutSmtp(PapercutConstants.ResourceNameOverrideParameters, httpPort: 8080, smtpPort: 2525);

Now I can use the two resource-names in my unittest in a Theory. My previous code would fail on the second papercut-resource.
Shall I commit this?

Changes will be:

• Added a static class PapercutConstants containing the names of the two papercut resources;
• Added a second papercut resource in my example project;
• Changed my AppHostTests to run the test ResourceStartsAndMailShouldBeReceived as a Theory with the two resourcenames.

[mguinness]

@aaronpowell might be better placed to answer. I suppose you could test that a port number outside the range 1-65535 fails.

[aaronpowell]

not sure I'm properly following - is the issue that if you set a port number outside a certain range things break unexpectedly?

[mguinness]

I just tested and AllocatedEndpoint throws a ArgumentOutOfRangeException.

Arno, I think what you already have in the PR is sufficient for merging if it can be assigned for review.

[aaronpowell]

Port range management should delegate to Aspire, unless a resource has very specific requirements