Skip to content

Check for empty list instead of null value. - #630

Merged
gathogojr merged 4 commits into
OData:mainfrom
VirusQuartirus:issue628
Jul 13, 2022
Merged

Check for empty list instead of null value.#630
gathogojr merged 4 commits into
OData:mainfrom
VirusQuartirus:issue628

Conversation

@VirusQuartirus

Copy link
Copy Markdown
Contributor

No description provided.

RequestHeaders requestHeader = request.GetTypedHeaders();
if (requestHeader != null && requestHeader.AcceptCharset != null)
// Starting from ASP .NET Core 3.0 AcceptCharset returns an empty collection instead of null.
if (requestHeader?.AcceptCharset.Count > 0)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (requestHeader?.AcceptCharset.Count > 0)
if (requestHeader?.AcceptCharset?.Count > 0)

Let's be compatible with both versions

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIR this repo is for ASP .NET Core 3+. So it can't be ran with ASP .NET Core 2.x. This change make sense for OData/WebApi repo. Correct me if I'm wrong.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also lean in favor of the AcceptCharset?.Count check

@habbes habbes Jul 12, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the standard lib explicitly marks the properties that can be null as nullable in newer .NET versions e.g.: https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.headers.requestheaders.cachecontrol?view=aspnetcore-6.0

I don't think that was the case for .NET Core 3.1 so it's hard to tell from just looking at the property whether it's safe to assume it can't be null.

MediaTypeHeaderValue currentContentType = GetContentType(response.Headers[HeaderNames.ContentType].FirstOrDefault());
RequestHeaders requestHeader = request.GetTypedHeaders();
if (requestHeader != null && requestHeader.AcceptCharset != null)
// Starting from ASP .NET Core 3.0 AcceptCharset returns an empty collection instead of null.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a test for this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

@ghost

ghost commented Jul 6, 2022

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

Comment thread test/Microsoft.AspNetCore.OData.E2E.Tests/Enums/EnumsTest.cs Outdated
Comment thread test/Microsoft.AspNetCore.OData.E2E.Tests/Enums/EnumsTest.cs Outdated
@gathogojr
gathogojr self-requested a review July 8, 2022 04:36
VirusQuartirus and others added 2 commits July 8, 2022 21:23

@gathogojr gathogojr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UTF-8 BOM is added to raw value response when running on ASP .NET Core 3+

5 participants