WebServiceTarget support for Posting JSON, XML + Injecting JSON serializer into NLog #1740

tetrodoxin · 2016-11-02T23:04:59Z

Added support for POST of JSON and XML documents and added respective tests. I tried to keep it simple, comprehensible and not to break with existing functionality.

I'd have to change existing tests in WebServiceTargetTests to extend their usability to the new types but I wasn't sure about altering existing tests of other authors, so I added new parts.

Have a look and tell me what you think about the direction it's heading.

This change is

fixes #700

…rget added corresponding tests to WebServiceTargetTests

304NotModified · 2016-11-02T23:09:33Z

Cool!

Could you undo the moved methods? It would be easier to review and otherwise we have a hard time merging this also in the .NET Core branch. Thanks!

tetrodoxin · 2016-11-02T23:17:45Z

Oh I see, that auto sorting and formatting of VS has created a diff-hell.
I think I'll deal with this tomorrow, just adding the new stuff at the bottom and try to prevent VS from exchanging TAB<=>spaces

Good night.

tetrodoxin · 2016-11-02T23:18:43Z

How do you want JSONSerializer to be 'injected' ?

For XmlSerializer too?

304NotModified · 2016-11-02T23:19:25Z

For XmlSerializer too?

Maybe, but that's already in non system.web libraries so less a problem IMO

tetrodoxin · 2016-11-02T23:21:24Z

So JSONSerializer injecting by defining an interface and adding a property to the target?

304NotModified · 2016-11-02T23:26:17Z

So JSONSerializer injecting by defining an interface and adding a property to the target?

Yes, and it would be nice if the property is at higher level than this target. I thinkConfigurationItemFactory or a new class.

304NotModified · 2016-11-02T23:26:21Z

good night!

tetrodoxin · 2016-11-03T17:10:09Z

So, tried a completed reformatting as good as possible.
Is it feasible this way?

I would then go and deal with JsonFormatter injection.

codecov-io · 2016-11-03T17:20:30Z

Current coverage is 82% (diff: 83%)

Merging #1740 into master will increase coverage by 1%

@@             master      #1740   diff @@
==========================================
  Files           276        274     -2   
  Lines         17711      18224   +513   
  Methods        2768       2833    +65   
  Messages          0          0          
  Branches       2020       2085    +65   
==========================================
+ Hits          14271      14854   +583   
+ Misses         2987       2908    -79   
- Partials        453        462     +9

Powered by Codecov. Last update 00e58af...f88a1d5

tetrodoxin · 2016-11-03T18:25:00Z

Two possible ways spontaneously cross my mind:

Just add an public IJsonSerializer' JsonSerializer {get; set;} to ConfigurationItemFactory which in turn sets an property to every WebServiceTarget instance it creates
introduce a IServiceResolver interface and an InjectedAttribute that marks properties of objects created by ConfigurationItemFactory as injectable and so is done, if that resolver 'knows' the type of that property

The first option would be more simple and quickly done, the second option indeed slightly more complex but more extendible and may be used for further scenarios. On the other hand, if no one can think of any additional use cases, that would be a perfect example of overkill.

tetrodoxin · 2016-11-03T18:27:13Z

Interface so far (inspired by JsonSerializer by NewtonSoft):

public interface IJsonSerializer
{
    void Serialize(TextWriter writer, object value);
    void Serialize(TextWriter writer, object value, Type objectType);
    object Deserialize(TextReader reader, Type objectType);
    T Deserialize<T>(TextReader reader);
}

304NotModified · 2016-11-03T19:10:25Z

I prefer the first one. More in line with the current options in NLog. :)

tetrodoxin · 2016-11-03T21:55:24Z

After reflecting about it...
Which 'object' shall this serializer convert to JSON, actually? Because by now WebServiceTarget writes parameters that are named in config, one by one. There is no complex object to serialize.

304NotModified · 2016-11-03T22:03:11Z

Well then the simple json serializer will work.

Except that is an issue with the current implementation when there is a quote in the value AFAIK.

304NotModified · 2016-11-03T22:03:57Z

src/NLog/Targets/WebServiceTarget.cs

+            private static string getJsonValueString(object value)
+            {
+                if (value == null) return "null";
+                else if (value is string) return string.Format("\"{0}\"", value);


we need to escape the quote inside the string?

tetrodoxin · 2016-11-03T22:04:21Z

Yup, now as you say it, forgot to escape strings.

tetrodoxin · 2016-11-03T22:05:39Z

I'm on it by tomorrow.

thus a custom JSON serialize can be set in and injected via ConfigurationItemFactory into WebServiceTarget

tetrodoxin · 2016-11-06T14:17:06Z

So, since the named parameters in config are just objects, I decided to add a slim serilizer interface, namely ICompactJsonSerializer, containing only a single method to serialize a value. In addition there is a default implementation, doing very rudimentary serialization and string escaping.
Tests for serializer are included.
Extended ConfigurationItemFactory so, that it provides a virtual method InterceptCreation, which is invoked right after a new object has been created via the instantiator in CreateInstance.
The base implementation of InterceptCreation realizes the injection of the JSON serializer into WebServiceTarget, if one was set by ConfigurationItemFactory.SetJsonSerializer()

Let me know, what you think... uhm, and I just see, I have to deal with Travis complaints :-/

tetrodoxin · 2016-11-06T19:44:45Z

Don't understand this: AppVeyor is complaining about things, that aren't contained in that files ?!?

fixed Silverlight specific constructors

tetrodoxin · 2016-11-19T15:10:16Z

So I'm tempted to say, we keep the serializer rudimentary and leave out serialization of collection types, because there are ready, mature serializers out there one can use, which have been the reason for this interface.

304NotModified · 2016-11-20T12:55:35Z

src/NLog/Targets/DefaultJsonSerializer.cs

+    /// </summary>
+    public class DefaultJsonSerializer : IJsonSerializer
+    {
+        private static List<Type> NumericTypes = new List<Type>


please create use a hashset (because of the contains futher)

304NotModified · 2016-11-20T12:57:11Z

Maybe have a level counter (default starting at 2) of how many levels down the object-graph that it will explore.

The easier and better way is to store the reference the a hashset? (and compare if it's already used)

304NotModified · 2016-11-20T12:57:39Z

So I'm tempted to say, we keep the serializer rudimentary and leave out serialization of collection types,

Good point, doubting as I think we are almost there.

tetrodoxin · 2016-11-21T14:21:19Z

So:
switch do hashset for numeric types
and
use a hashset to track visited references to prevent circular paths.

Allright?

304NotModified · 2016-11-21T17:16:43Z

Yes that would be nice!

tetrodoxin · 2016-11-21T18:19:09Z

I'm on it

tetrodoxin · 2016-11-21T18:30:57Z

Although the question may be: how to handle reference loops? JSON.NET for example per default breaks with an exception. You can alter this behavior to 2 alternatives.
We could end with an exception or skip serialization of objects, when they're child of themselves. In both cases we track every 'path' of object serialization.

snakefoot · 2016-11-21T20:01:47Z

Consider extracting the method JsonLayout.IsNumeric() into a helper-class, then you can skip the HashSet-Lookup.

304NotModified · 2016-11-21T20:26:01Z

I prefer skip instead of exception

tetrodoxin · 2016-11-21T23:28:34Z

Now it uses Hashset (and Contains() ) for checking current serialization path for loop. In the (unlikely) case of an infinite loop because of special implementations of IEnumerable for example, there is also an maximum recursion depth of 10, just protecting application from a stack overflow or out of memory.

304NotModified · 2016-11-21T23:36:20Z

👍
cool. Maybe unlikely, but stackoverflows are one the worst issue you could have in an application. So thanks for fixing it!

snakefoot · 2016-11-22T22:47:00Z

src/NLog/Targets/WebServiceTarget.cs

+                string valueString = string.Empty;
+                if (!string.IsNullOrEmpty(parameterValue))
+                {
+                    var sb = new StringBuilder(Math.Max(parameterValue.Length + 20, Target.Parameters.Count > 1 ? 256 : 0));


The Target.Parameters.Count-check only makes sense when using the same StringBuilder for streaming all parameters.

Unless you optimize the (re-)usage of the StringBuilder, then it should just be:

var sb = new StringBuilder(parameterValue.Length + 20);

snakefoot · 2016-11-22T22:53:44Z

src/NLog/Targets/WebServiceTarget.cs

+                    UrlHelper.EscapeDataEncode(parameterValue, sb, encodingFlags);
+                    valueString = sb.ToString();
+                }
+
                return string.Format("{0}={1}",


Could it not be changed it into:

if (string.IsNullOrEmpty(parameterValue))
{
return string.Concat(parameter.Name, "=");
}
else
{
var sb = new StringBuilder(parameter.Name.Length + parameterValue.Length + 20);
sb.Append(parameter.Name);
sb.Append("=");
UrlHelper.EscapeDataEncode(parameterValue, sb, encodingFlags);
return sb.ToString();
}

(Could probably be optimized even more if re-using the StringBuilder)

snakefoot · 2016-11-22T22:58:48Z

src/NLog/Targets/WebServiceTarget.cs

+
+            protected override string GetFormattedContent(string parametersContent)
+            {
+                return string.Format("{{{0}}}", parametersContent);


Consider changing into this:

return string.Concat("{{", parametersContent, "}}");

snakefoot · 2016-11-22T23:00:20Z

src/NLog/Targets/WebServiceTarget.cs

+                }
+                else
+                {
+                    soapAction = Target.Namespace + "/" + Target.MethodName;


Consider changing into this:

soapAction = string.Concat(Target.Namespace, "/", Target.MethodName);

(Maybe also use string.Concat for the one without slash for consistency)

304NotModified · 2016-11-24T19:17:34Z

This is ready to merge right?

snakefoot · 2016-11-24T21:52:17Z

src/NLog/Targets/WebServiceTarget.cs

+                }
+                else
+                {
+                    var sb = new StringBuilder(parameterValue.Length + 20);


Consider changing it into:

var sb = new StringBuilder(parameter.Name.Length + parameterValue.Length + 20);

304NotModified · 2016-11-24T23:08:12Z

Reviewed 1 of 17 files at r10, 1 of 4 files at r11, 21 of 22 files at r13, 1 of 1 files at r14.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

Comments from Reviewable

304NotModified · 2016-11-24T23:10:15Z

@tetrodoxin I have one final comment: please don't create variables with just 1 char! l, c, d etc. Please do use meaningful names. Only in math single chars are allowed ;)

When those names are fixed, I will merge it.

@snakefoot it's OK for me to merge it and tweaks some last things with a new PR.

added support for HTTP POST of XML and JSON documents to WebServiceTa…

8985188

…rget added corresponding tests to WebServiceTargetTests

304NotModified added this to the 4.4 milestone Nov 2, 2016

304NotModified added the feature label Nov 2, 2016

fixed using directives

f711a8c

304NotModified mentioned this pull request Nov 2, 2016

Make it possible to inject JSONSerializer #1741

Closed

reformatted

d7325e2

304NotModified reviewed Nov 3, 2016

View reviewed changes

introduced ICompactJsonSerializer and DefaultJsonSerializer, plus tests

845e738

thus a custom JSON serialize can be set in and injected via ConfigurationItemFactory into WebServiceTarget

tetrodoxin added 2 commits November 6, 2016 15:20

resynced projects files

a959cb4

fixed using statements

94b72d4

tetrodoxin added 2 commits November 6, 2016 20:48

fix for CI

ed93740

corrected mistakenly added TargetsContructorsTests.cs

0ce2c43

fixed Silverlight specific constructors

304NotModified reviewed Nov 20, 2016

View reviewed changes

tetrodoxin added 2 commits November 21, 2016 23:42

altered DefaultJsonSerializer to prevent reference loops

f20bbac

merged master

558ad31

snakefoot reviewed Nov 22, 2016

View reviewed changes

altered string concatenation

f88a1d5

snakefoot reviewed Nov 24, 2016

View reviewed changes

304NotModified merged commit ecc64a4 into NLog:master Nov 25, 2016

304NotModified added needs documentation on wiki and removed almost ready labels Dec 9, 2016

304NotModified changed the title ~~WebServiceTarget support for JSON & Injecting JSON serializer into NLog~~ WebServiceTarget support for JSON, XML + Injecting JSON serializer into NLog Dec 9, 2016

304NotModified changed the title ~~WebServiceTarget support for JSON, XML + Injecting JSON serializer into NLog~~ WebServiceTarget support for Posting JSON, XML + Injecting JSON serializer into NLog Dec 9, 2016

304NotModified added the documentation done all docs done (wiki, api docs, lists on nlog-project.org, xmldocs) label Jan 14, 2018

snakefoot added the webservice-target label Dec 30, 2023

WebServiceTarget support for Posting JSON, XML + Injecting JSON serializer into NLog #1740

WebServiceTarget support for Posting JSON, XML + Injecting JSON serializer into NLog #1740

Conversation

tetrodoxin commented Nov 2, 2016 • edited by 304NotModified Loading

304NotModified commented Nov 2, 2016

tetrodoxin commented Nov 2, 2016

tetrodoxin commented Nov 2, 2016 • edited Loading

304NotModified commented Nov 2, 2016

tetrodoxin commented Nov 2, 2016

304NotModified commented Nov 2, 2016

304NotModified commented Nov 2, 2016 • edited Loading

tetrodoxin commented Nov 3, 2016

codecov-io commented Nov 3, 2016 • edited Loading

Current coverage is 82% (diff: 83%)

tetrodoxin commented Nov 3, 2016

tetrodoxin commented Nov 3, 2016 • edited Loading

304NotModified commented Nov 3, 2016

tetrodoxin commented Nov 3, 2016

304NotModified commented Nov 3, 2016 • edited Loading

304NotModified Nov 3, 2016

Choose a reason for hiding this comment

tetrodoxin commented Nov 3, 2016

tetrodoxin commented Nov 3, 2016

tetrodoxin commented Nov 6, 2016

tetrodoxin commented Nov 6, 2016

tetrodoxin commented Nov 19, 2016

304NotModified Nov 20, 2016

Choose a reason for hiding this comment

304NotModified commented Nov 20, 2016

304NotModified commented Nov 20, 2016

tetrodoxin commented Nov 21, 2016

304NotModified commented Nov 21, 2016

tetrodoxin commented Nov 21, 2016

tetrodoxin commented Nov 21, 2016

snakefoot commented Nov 21, 2016

304NotModified commented Nov 21, 2016

tetrodoxin commented Nov 21, 2016

304NotModified commented Nov 21, 2016

snakefoot Nov 22, 2016

Choose a reason for hiding this comment

snakefoot Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

snakefoot Nov 22, 2016

Choose a reason for hiding this comment

snakefoot Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

304NotModified commented Nov 24, 2016

snakefoot Nov 24, 2016

Choose a reason for hiding this comment

304NotModified commented Nov 24, 2016

304NotModified commented Nov 24, 2016 • edited Loading

tetrodoxin commented Nov 2, 2016 •

edited by 304NotModified

Loading

tetrodoxin commented Nov 2, 2016 •

edited

Loading

304NotModified commented Nov 2, 2016 •

edited

Loading

codecov-io commented Nov 3, 2016 •

edited

Loading

tetrodoxin commented Nov 3, 2016 •

edited

Loading

304NotModified commented Nov 3, 2016 •

edited

Loading

snakefoot Nov 22, 2016 •

edited

Loading

snakefoot Nov 22, 2016 •

edited

Loading

304NotModified commented Nov 24, 2016 •

edited

Loading