Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WithCallback circumvent the rest of the builder #569

Closed
rlf opened this issue Jan 27, 2021 · 5 comments
Closed

WithCallback circumvent the rest of the builder #569

rlf opened this issue Jan 27, 2021 · 5 comments
Assignees
Labels

Comments

@rlf
Copy link

rlf commented Jan 27, 2021

Describe the bug

The code-line listed here:

Effectively disables part of the builder - which IMO ruins the builder-pattern for the response builder.

Expected behavior:

I would expect the following code:

Response.Create()
  .WithCallback(req => new ResponseMessage())
  .WithStatusCode(HttpStatusCode.Created)
  .WithHeader("X-UserId", "NA")

to generate a responsemessage with what-ever the callback provides, but with the status-code Created and the additional header X-UserId.

Test to reproduce

Create any Given().RespondWith() chain that mixes both WithCallback and other builder-inputs.
Verify the Callback short-circuits anything else you put into the Builder-pattern.

I do get, how-ever, that if the callback changes the default value of say the Status - that status should prevail - but not if you specifically add a non-default status to your response, using the builder.

Other related info

None that I can think of.

@rlf rlf added the bug label Jan 27, 2021
@StefH
Copy link
Collaborator

StefH commented Feb 1, 2021

@rlf I understand your issue.

I cannot remember anymore why I decided that WithCallback does behave like this.
(Maybe I thought that you want to do all logic in the callback, so also defining the status-codes and headers)

However, I see that in that case the fluentbuilder should not return a chainable thing so that it seems that you can add more fluent methods.

I'll check if it's easy to change that all additional .WIth*** are used.

@StefH
Copy link
Collaborator

StefH commented Feb 18, 2021

Hello @rlf,

Can you please try preview NuGet WireMock.Net.1.4.6-ci-14668 from MyGet (https://github.com/WireMock-Net/WireMock.Net/wiki/MyGet-preview-versions)? This should fix your issue.

@StefH
Copy link
Collaborator

StefH commented Feb 22, 2021

Dear @rlf ; did you have time to test this?

@rlf
Copy link
Author

rlf commented Feb 26, 2021

Verified. At least the tests I wrote when discovering this bug is now green.
Thanks!
Awesome work man!

@StefH
Copy link
Collaborator

StefH commented Feb 26, 2021

@rlf Thank you for testing.

I'll release a new official version today.

@StefH StefH closed this as completed Feb 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants