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

Use System.Text.Json to deserialize health report response #975

Merged
merged 1 commit into from
Mar 9, 2022

Conversation

mipa87
Copy link
Contributor

@mipa87 mipa87 commented Mar 1, 2022

What this PR does / why we need it:
Use System.Text.Json to deserialize health report response in HealthCheckReportCollector.cs.

See https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/HttpContentJsonExtensions.cs

Which issue(s) this PR fixes:
Better handling of bad responses

  • when State of response contains bad value, e.g. 404
  • when response body is empty

Please reference the issue this PR will close:
#[862]

Special notes for your reviewer:
Or it can be like this, but there is check for EnsureSuccessStatusCode() which fails for unhealthy status (returns 503), so I think this is not the right solution:

return await _httpClient.GetFromJsonAsync<UIHealthReport>(absoluteUri, new JsonSerializerOptions(JsonSerializerDefaults.Web)
{
    Converters = {
        new JsonStringEnumConverter(null, false)
    }
});

See https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/HttpClientJsonExtensions.Get.cs

Does this PR introduce a user-facing change?:
No

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Unit tests passing
  • End-to-end tests passing
  • Extended the documentation
  • Provided sample for the feature

@mipa87 mipa87 force-pushed the feature/DeserializeResponse branch from 4c18681 to d6040bb Compare March 1, 2022 18:59
@sungam3r
Copy link
Collaborator

sungam3r commented Mar 1, 2022

Is it really necessary to change all those launchsettings ?

@sungam3r
Copy link
Collaborator

sungam3r commented Mar 1, 2022

Please reference the issue this PR will close:
#[862]

#862 was fixed in #911

@sungam3r
Copy link
Collaborator

sungam3r commented Mar 1, 2022

Better handling of bad responses

when State of response contains bad value, e.g. 404
when response body is empty

What do you mean better ?

@mipa87
Copy link
Contributor Author

mipa87 commented Mar 1, 2022

Is it really necessary to change all those launchsettings ?

It's not necessary, I updated it when I tested it.

@mipa87
Copy link
Contributor Author

mipa87 commented Mar 1, 2022

Better handling of bad responses

when State of response contains bad value, e.g. 404
when response body is empty

What do you mean better ?

When Status in response is 404, using Newtonsoft Json this value is propagated to the UII, where it crashes in some places.

@sungam3r
Copy link
Collaborator

sungam3r commented Mar 1, 2022

When Status in response is 404, using Newtonsoft Json this value is propagated to the UII, where it crashes in some places.

So now the error will occur earlier?

@mipa87 mipa87 force-pushed the feature/DeserializeResponse branch from 2a1b004 to 4316864 Compare March 2, 2022 08:54
@mipa87
Copy link
Contributor Author

mipa87 commented Mar 2, 2022

When Status in response is 404, using Newtonsoft Json this value is propagated to the UII, where it crashes in some places.

So now the error will occur earlier?

No, this behavior occurs only with Newtonsoft.Json. Now with System.Text.Json the string values like "0", "2" , "Healthy", "Healthy " are handled correctly, when value is e.g. 404 it throws exception.

@sungam3r
Copy link
Collaborator

sungam3r commented Mar 2, 2022

the string values like "0", "2" , "Healthy", "Healthy " are handled correctly

Are you sure? You use allowIntegerValues: false so integer enum values should throw I think. I'm OK for such behavior by the way.

@sungam3r
Copy link
Collaborator

sungam3r commented Mar 2, 2022

@carlosrecuero Are you OK to change all those launchsettings files?

@mipa87
Copy link
Contributor Author

mipa87 commented Mar 2, 2022

the string values like "0", "2" , "Healthy", "Healthy " are handled correctly

Are you sure? You use allowIntegerValues: false so integer enum values should throw I think. I'm OK for such behavior by the way.

Integer in a string (like "0") will be fine and an integer as a number (like 0) will fail if allowIntegerValues is false.

@mipa87
Copy link
Contributor Author

mipa87 commented Mar 2, 2022

@carlosrecuero Are you OK to change all those launchsettings files?

I know this has nothing to do with the proposed changes, but they are samples and I think it's good that they work in one click. Or I can submit another pull request for these changes.

@sungam3r
Copy link
Collaborator

sungam3r commented Mar 2, 2022

Integer in a string (like "0") will be fine and an integer as a number (like 0) will fail if allowIntegerValues is false.

🤔 I didn't know. OK. Though "0"/"1" seems a bit weird and maybe I would prefer to forbid such "enum values".

I'm fine to merge as is if @carlosrecuero approve.

@sungam3r2022
Copy link
Contributor

@carlosrecuero Merge as you see fit. I have no write access with this new account.

@sungam3r
Copy link
Collaborator

sungam3r commented Mar 9, 2022

So.... I merge it as is because I see no harm to have all those launchsettings files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants