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

CapturedMultipartContent doesn't really capture content #580

Closed
bchavez opened this issue Dec 4, 2020 · 3 comments
Closed

CapturedMultipartContent doesn't really capture content #580

bchavez opened this issue Dec 4, 2020 · 3 comments
Labels

Comments

@bchavez
Copy link

bchavez commented Dec 4, 2020

Hey Todd,

Hope you are well & safe. Also, congrats on your 3.0 release! And thank you for all you do.

I think I may have found a small bug in the library:

void Main()
{
   var test = new HttpTest();
   test.RespondWith("OK");
                  
   "https://foo.com/upload".PostMultipartAsync( bc => {
      bc.AddString("key", "value");
   });

   test.ShouldHaveCalled("https://foo.com/upload")
      .With(fc =>
      {
         var content = fc.HttpRequestMessage.Content as CapturedMultipartContent;
         content.Parts.Length.Should().Be(1);
         return true;
      });
}

image

The reason I think CapturedMultipartContent doesn't capture the content is:

  1. The base class MultipartContent is used as a storage container for the items being added:


    public HttpContent[] Parts => this.ToArray();

  2. The MultipartContent in System.Net.Http clears the _nestedContent on .Dispose():
    https://github.com/dotnet/runtime/blob/be9d38c8586af55ac848392fedf0076760e6cccc/src/libraries/System.Net.Http/src/System/Net/Http/MultipartContent.cs#L126-L137

  3. The HttpRequestMessage is disposed of after the request completes in FlurlRequest. And the content is already gone and cleared by the time we make the assertion.

    request.Dispose();

Let me know if you'd like me to create a PR to fix the issue. If performance is a concern, perhaps there could be:

if inTestMode
   keep an additional copy in a local list
else
   use parent container

Best regards,
Brian

@bchavez bchavez added the bug label Dec 4, 2020
@tmenier
Copy link
Owner

tmenier commented Dec 14, 2020

Thanks for reporting. Ick. I agree, "captured" should imply not clearing that collection. I'll take a crack at fixing. Maybe overriding Dispose is the cleanest way, unless you have other ideas.

@tmenier tmenier added this to Backlog in Default Project via automation Dec 14, 2020
@tmenier tmenier moved this from Backlog to In Progress in Default Project Dec 14, 2020
@tmenier
Copy link
Owner

tmenier commented Dec 14, 2020

...Actually, capturing to a different collection is a little cleaner, and more in line with what CapturedStringContent does. I have no overhead concerns - it's a redundant collection but it references the same object instances as the original.

@tmenier tmenier moved this from In Progress to Ready in Default Project Dec 14, 2020
@tmenier
Copy link
Owner

tmenier commented Dec 14, 2020

Fixed & released in 3.0.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Default Project
  
Released
Development

No branches or pull requests

2 participants