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

WebServiceTarget - Fix boolean parameter conversion for Xml and Json (lowercase) & merged the JSON serializer code into DefaultJsonSerializer #2017

Merged
merged 1 commit into from Mar 22, 2017

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Mar 17, 2017

And also merged the JSON serializer code into DefaultJsonSerializer (Trying to fix #2016)

This also fixes the handling of boolean for the DefaultJsonSerializer, so it is now lowercase without being json-encoded (Fixed the existing test to be case-sensitive). This also fixes the handling of all non-string-objects in DefaultJsonSerializer (Ex DateTime or Guid), so when converted to string, then they are actually json-encoded as strings (Added new tests to verify this).

This also reduces the number of string-allocations in DefaultJsonSerializer, when serializing standard strings (better performance). The original EscapeString (would allocate StringBuilder and call ToString for every value) has been replaced by JsonStringEscape (only allocates StringBuilder when needed).

Modified the use of TypeCode, so it should also work on NET Core (Now using System.Convert.GetTypeCode instead of System.Type.GetTypeCode)

Will probably gives lots of conflicts in the NLog 5.0 Core-Branch (as you have already made change to fix the previous use of System.Type.GetTypeCode). But since I have moved the old code to XmlHelper.cs, then it should be easy to fix.

@codecov-io
Copy link

codecov-io commented Mar 17, 2017

Codecov Report

Merging #2017 into master will decrease coverage by <1%.
The diff coverage is 81%.

@@           Coverage Diff           @@
##           master   #2017    +/-   ##
=======================================
- Coverage      82%     82%   -<1%     
=======================================
  Files         286     286            
  Lines       19665   19663     -2     
  Branches     2297    2309    +12     
=======================================
- Hits        16072   16028    -44     
- Misses       3037    3069    +32     
- Partials      556     566    +10

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73f317b...0e833ea. Read the comment docs.

@snakefoot
Copy link
Contributor Author

snakefoot commented Mar 17, 2017

Have now created XmlHelper.XmlConvertToString(object value) that uses XmlConvert.ToString() as much as possible. And changed Log4JXmlEventLayoutRenderer to also use XmlConvertToString for writing properties.

Have also added Xml-string-validation to WebServiceTarget.HttpPostXmlFormatterBase, so it will automatically strip invalid-xml-characters for the xml-paramters. Extended the existing unit-test to show that it also works.

Also added handling for DateTime-formatting, but it seems to be a dangerous minefield (Many different formatting options, and we only have one). But since one can always specify parameter as string, and provide a custom format string, then it will not break anything. The handling of DateTime-formatting is only applied when having specified the parameter as System.DateTime (Just like the handling of Boolean-formatting is only applied when having the parameter as System.Boolean).

Maybe someone can answer why the existing unit-test enforced the Microsoft JSON-datacontract-formatter, instead of the newer (and default) JSON-NET-formatter?

Formatters.JsonFormatter.UseDataContractJsonSerializer = true

I had to change it to the default JSON-NET-formatter for the DateTime-parameter to work (Instead of using microsoft-wcf-format)

@snakefoot snakefoot force-pushed the WebServiceTargetAllowBoolean branch 23 times, most recently from 60576a1 to 8468e44 Compare March 19, 2017 15:53
@snakefoot snakefoot changed the title WebServiceTarget - Improved conversion of boolean parameters (lowercase) WebServiceTarget - Fix boolean parameter conversion for Xml and Json (lowercase) Mar 19, 2017
@snakefoot snakefoot force-pushed the WebServiceTargetAllowBoolean branch 2 times, most recently from 31a3a6f to df87a3a Compare March 19, 2017 19:01
Copy link
Member

@304NotModified 304NotModified left a comment

Choose a reason for hiding this comment

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

Thanks, have to check more in-depth, will try to do this week

@@ -98,7 +98,13 @@ public MethodCallParameter(string name, Layout layout, Type type)
/// </summary>
/// <docgen category='Parameter Options' order='10' />
[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1721:PropertyNamesShouldNotMatchGetMethods", Justification = "Backwards compatibility")]
public Type Type { get; set; }
public Type Type { get { return ParameterType; } set { ParameterType = value; } }
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should tell this is the same as ParameterType (with <see)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

}

objTypeCode = Convert.GetTypeCode(value);
switch (objTypeCode)
Copy link
Member

Choose a reason for hiding this comment

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

is working with typecode better than type? Any idea of the .NET core support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think switch-statement is faster than dictionary-lookup. System.Convert.GetTypeCode should also work in NET Core (It is System.Type.GetTypeCode that has been removed).

@304NotModified 304NotModified added this to the 4.4.5 milestone Mar 20, 2017
@snakefoot snakefoot force-pushed the WebServiceTargetAllowBoolean branch 2 times, most recently from 42ade37 to 28445c3 Compare March 20, 2017 20:12
…owercase), and merged the JSON serializer code (Fixed DateTime)
@snakefoot snakefoot force-pushed the WebServiceTargetAllowBoolean branch from 28445c3 to 0e833ea Compare March 20, 2017 20:19
@304NotModified 304NotModified added the bug Bug report / Bug fix label Mar 22, 2017
@304NotModified 304NotModified changed the title WebServiceTarget - Fix boolean parameter conversion for Xml and Json (lowercase) WebServiceTarget - Fix boolean parameter conversion for Xml and Json (lowercase) & merged the JSON serializer code into DefaultJsonSerializer Mar 22, 2017
@304NotModified 304NotModified added enhancement Improvement on existing feature has unittests labels Mar 22, 2017
@304NotModified
Copy link
Member

thanks!

@304NotModified 304NotModified merged commit ff7c8e3 into NLog:master Mar 22, 2017
@snakefoot snakefoot deleted the WebServiceTargetAllowBoolean branch October 10, 2017 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug report / Bug fix enhancement Improvement on existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants