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.

Sign up for GitHub

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

Jump to bottom

Do not expose HttpResponseMessage.Content == null to user code #386

Closed
dougbu opened this issue Feb 13, 2023 · 2 comments · Fixed by #387
Closed

Do not expose HttpResponseMessage.Content == null to user code #386

dougbu opened this issue Feb 13, 2023 · 2 comments · Fixed by #387
Assignees
@dougbu
Labels
bug

Comments

@dougbu
Copy link
Member

dougbu commented Feb 13, 2023

In recent (5.0 and newer) .NET runtimes, Content==null is impossible for user code to see. Even in older runtimes, HttpClient would never leave Content==null either.

Public code in this repo both exposes Content==null (inconsistently) and expects (mostly in tests) to see that state. Bad enough we expose that state but it being inconsistent (depending on the runtime version) makes things worse.

The action here is to change our code to both remove the inconsistencies i.e., have Content!=null everywhere regardless of the runtime version and update our expectations to handle Content!=null. This includes reverting tests removed on net6.0 in #384.
@dougbu dougbu added the bug label Feb 13, 2023
@dougbu dougbu self-assigned this Feb 13, 2023
@dougbu
Copy link
Member Author

dougbu commented Feb 13, 2023

Anything to add @Tratcher or @stephentoub❔

dougbu added a commit to dougbu/AspNetWebStack that referenced this issue Feb 13, 2023
@dougbu
!fixup! Reference aspnet#386
a74ea59 
- fix comming **Real Soon Now:tm:** :grin:
@dougbu dougbu mentioned this issue Feb 13, 2023
Merged
@stephentoub
Copy link

Sounds fine.

dougbu added a commit that referenced this issue Feb 13, 2023
@dougbu
Test Formatting assemblies w/ net6.0 (#384)
0f4c624 
- expand our text matrix to include a modern (and LTS) TFM
- change how Formatting test assemblies are found
  - `netcoreapp` is no longer the only relevant folder prefix
- use the latest .NET 6 SDK
  - install the 2.1.x runtime in the pipeline
- don't require .NET in VS
  - will use binplaced `msbuild` instead
  - have `git` ignore the new .msbuild/ folder
- react to new `Exception.Message`s in `netcoreapp3.1`
  - handle different formatting of argument info in `ArgumentException.Message`s
  - handle slightly greater `decimal` precision in a `JsonReaderException.Message`
- react to new `Exception.Message`s and other changes in `net6.0`
  - handle different `Message` in `InvalidOperationException`s about invalid request URIs
- react to other changes in `net6.0`
  - handle inability to mock a `Stream` if a writer passes a `ReadOnlySpan<byte>` in `net6.0`
    - see devlooped/moq#829, devlooped/moq#979, and dotnet/runtime#45152 about the issue
  - skip tests failing due to `HttpResponseMessage` changes
    - see #386 and dotnet/runtime@b48900f (which introduced this)
    - fix coming **Real Soon Now:tm:** :grin:
- nits:
  - simplify define use now that `NETCOREAPP3_1_OR_GREATER` and so on are available
  - clean up .gitignore
  - clean up a few comments and tighten scripting up
dougbu added a commit to dougbu/AspNetWebStack that referenced this issue Feb 17, 2023
@dougbu
Add Content to all HttpResponseMessages
ecadf28 
- fix aspnet#386
  - reenable `net6.0` tests disabled in aspnet#384 for this issue
- also, check `Stream.CanSeek` less when calculating `HttpMessageContent.ContentLength` value
  - use inner `HttpContent.Headers.ContentLength` if available
  - this changes behaviour slightly for both requests and responses
    - observable as `HttpMessageContent.Headers.ContentLength!=null` for empty messages
  - for responses, avoids `ContentLength==null` versus `ContentLength==0` inconsistencies (depending on platform)
  - `ContentLength==null` may still occur in some corner cases (which existed before)
    - e.g. when inner `HttpContent` doesn't know its length and `ReadAsStreamAsync()` hasn't completed
- main change means `HttpResponseMessage`s we expose to user code are consistent across platforms
  - note: user code won't see `EmptyContent` in `HttpResponseMessage`s we create
@dougbu dougbu mentioned this issue Feb 17, 2023
Merged
dougbu added a commit that referenced this issue Feb 17, 2023
@dougbu
Add Content to all HttpResponseMessages (#387)
979e6ac 
- fix #386
  - reenable `net6.0` tests disabled in #384 for this issue
- also, check `Stream.CanSeek` less when calculating `HttpMessageContent.ContentLength` value
  - use inner `HttpContent.Headers.ContentLength` if available
  - this changes behaviour slightly for both requests and responses
    - observable as `HttpMessageContent.Headers.ContentLength!=null` for empty messages
  - for responses, avoids `ContentLength==null` versus `ContentLength==0` inconsistencies (depending on platform)
  - `ContentLength==null` may still occur in some corner cases (which existed before)
    - e.g. when inner `HttpContent` doesn't know its length and `ReadAsStreamAsync()` hasn't completed
- main change means `HttpResponseMessage`s we expose to user code are consistent across platforms
  - note: user code won't see `EmptyContent` in `HttpResponseMessage`s we create
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dougbu dougbu

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add Content to all HttpResponseMessages dougbu/AspNetWebStack
2 participants
@stephentoub @dougbu