-
Notifications
You must be signed in to change notification settings - Fork 12
Add a converter to serialize properly DBNull value #477
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
Conversation
…he suggestion in dotnet/runtime#418 Move duplicate code in GxRedis.cs and GxMemcached.cs to GxClasses.csproj Adding the package reference System.Text.Json to GxClasses.csproj gets a conflict with System.Net.Http. Because of that, HttpClient used in MapFunctions.cs is replaced by HttpWebRequest for .netframework similar to the use in GXGeolocation.
Cherry pick to beta success |
| #if !NETCORE | ||
| HttpWebRequest req = (HttpWebRequest)WebRequest.Create(new Uri(urlString)); | ||
| req.Method = "GET"; | ||
| HttpWebResponse resp = (HttpWebResponse)req.GetResponse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HttpWebResponse should be released. Maybe include "using" ?
https://docs.microsoft.com/en-us/dotnet/api/system.net.httpwebresponse.getresponsestream?view=net-5.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Moreover, I removed the unused code.
| { | ||
| string info = readStream.ReadToEnd(); | ||
| } | ||
| rStream.Close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use "using" instead of .Close() ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely. The unnecessary code was removed.
| { | ||
| using (HttpContent content = response.Content) | ||
| { | ||
| string info = content.ReadAsStringAsync().Result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is not part of this commit, sorry!. But look what weird it is this.
The Web Response "info" string is read from the Stream, but not used anywhere?
Also, if the HttpRequest fails (crashes), Exception would ocurr. Maybe we can add a try-catch and log the error? What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No doubt about it. I removed that unnecessary code.
…abs/DotNetClasses into memcached-serialization
Cherry pick to beta success |
* Add a converter to serialize properly the DBNull value according to the suggestion in dotnet/runtime#418 Move duplicate code in GxRedis.cs and GxMemcached.cs to GxClasses.csproj Adding the package reference System.Text.Json to GxClasses.csproj gets a conflict with System.Net.Http. Because of that, HttpClient used in MapFunctions.cs is replaced by HttpWebRequest for .netframework similar to the use in GXGeolocation. * Remove unused code. (cherry picked from commit fbfe50c)
According to the suggestion at dotnet/runtime#418
Issue:91913
Move duplicate code in GxRedis.cs and GxMemcached.cs to GxClasses.csproj
Adding the package reference System.Text.Json to GxClasses.csproj gets a conflict with System.Net.Http. Because of that, HttpClient used in MapFunctions.cs is replaced by HttpWebRequest for .netframework similar to the use in GXGeolocation.