Removed binaryformatter replaced with json serializer #2264

jchannon · 2016-02-09T15:48:50Z

address #2220

khellang · 2016-02-09T16:03:41Z

src/Nancy/DefaultObjectSerializer.cs

            {
-                formatter.Serialize(outputStream, sourceObject);
+                var jsonSerializer = new JsonSerializer(new JavaScriptSerializer());


Should we really new this up every time?

Also, why haven't we #ifdef'ed this to use BinaryFormatter where available?

Because JS will work for both I guess

khellang · 2016-02-15T10:44:41Z

src/Nancy.MSBuild/Nancy.csproj

@@ -1531,6 +1537,9 @@
      <Link>Diagnostics\Views\login.sshtml</Link>
    </EmbeddedResource>
  </ItemGroup>
+  <ItemGroup>
+    <None Include="packages.config" />


I can't see this anywhere?

thecodejunkie · 2016-02-16T07:23:53Z

src/Nancy/Extensions/ObjectExtensions.cs

+            IDictionary<string, object> expando = new ExpandoObject();
+
+            foreach (PropertyDescriptor property in TypeDescriptor.GetProperties(value.GetType()))
+                expando.Add(property.Name, property.GetValue(value));


Missing { }

thecodejunkie · 2016-02-16T07:34:09Z

So the one thing here is that this does not seem to handle object graphs. The ObjectExtensions.ToDynamic does not seem to handle if there are complex types on one of the properties? With the BinaryFormatter is not a problem, but here we only seem to handle 1-level of properties and they have to be primitive types

phillip-haydon · 2016-02-16T07:40:59Z

test/Nancy.Tests/Unit/Security/CsrfFixture.cs

@@ -34,8 +35,10 @@ public CsrfFixture()
            this.pipelines = new MockPipelines();

            this.cryptographyConfiguration = CryptographyConfiguration.Default;
-
-            this.objectSerializer = new DefaultObjectSerializer();
+            var fakeAssCatalog = A.Fake<IAssemblyCatalog>();


People might get offended by the word 'Ass'

😄 This is the same as the adq variable, why shorten it? fakeAssemblyCatalog is waaaaay more descriptive. 👍 for changing this, even if it's not for the same reason @phillip-haydon pointed out ;)

jchannon · 2016-02-16T11:28:00Z

@thecodejunkie seems to be ok or are you thinking more complex https://dotnetfiddle.net/0RDcN1

thecodejunkie · 2016-02-18T10:06:30Z

I stand corrected :D

thecodejunkie · 2016-02-18T10:12:16Z

Looking at AppVeyor, we should get #2274 in first and then rebase this?

thecodejunkie · 2016-02-18T10:13:10Z

samples/Nancy.Demo.Hosting.Aspnet/DemoBootstrapper.cs

@@ -66,7 +66,7 @@ protected override void ApplicationStartup(TinyIoCContainer container, IPipeline

            this.Conventions.StaticContentsConventions.Add(StaticContentConventionBuilder.AddDirectory("moo", "Content"));

-            CookieBasedSessions.Enable(pipelines);
+            CookieBasedSessions.Enable(pipelines, container.Resolve<IAssemblyCatalog>());


this.AssemblyCatalog should be all that is needed here, no?

Removed binaryformatter replaced with json serializer

thecodejunkie · 2016-02-23T15:29:12Z

src/Nancy/DefaultObjectSerializer.cs

-            using (var outputStream = new MemoryStream())
+        private dynamic AddTypeInformation(object sourceObject)
+        {
+            var sourceType = this.assemblyCatalog.GetAssemblies().Select(assembly => assembly.GetType(sourceObject.GetType().FullName)).FirstOrDefault(type => type != null);


@jchannon this can be replaced by sourceObject.GetType().AssemblyQualifiedName can't it?

we already do that a few lines below, this is finding the type in the list of assemblies to get the assembly qualified name as the object being serialized might be in a project just with pocos for example

this is finding the type in the list of assemblies to get the assembly qualified name

But why do you need to find it in a list of assemblies when you got the type right there? It's even used to get the FullName of the type...

I think maybe AssemblyQualifiedName is empty if its a class in a separate poco lib or something.

You think? 😝

I think thats some sarcasm I'm not getting 😄 We need to find the type in the assemblies to get the AQN

You can't think, you need to know.

The only way AssemblyQualifiedName returns null is if it's a generic type parameter. From MSDN:

The assembly-qualified name of the Type, which includes the name of the assembly from which the Type was loaded, or null if the current instance represents a generic type parameter.

Well if you hadn't insisted on one commit message per PR then I might have
something that jogs my memory why I added it.

Remove it, run the tests and if all is ok then consider me told

On Tuesday, 23 February 2016, Kristian Hellang [email protected]
wrote:

In src/Nancy/DefaultObjectSerializer.cs
#2264 (comment):

using (var outputStream = new MemoryStream())

private dynamic AddTypeInformation(object sourceObject)

{

var sourceType = this.assemblyCatalog.GetAssemblies().Select(assembly => assembly.GetType(sourceObject.GetType().FullName)).FirstOrDefault(type => type != null);

You can't think, you need to know.

The only way AssemblyQualifiedName returns null is if it's a generic type
parameter. From MSDN:

The assembly-qualified name of the Type, which includes the name of the
assembly from which the Type was loaded, or null if the current instance
represents a generic type parameter.

[image: image]
https://cloud.githubusercontent.com/assets/582487/13261618/49c94f20-da61-11e5-8eb1-937c659111c6.png

—
Reply to this email directly or view it on GitHub
https://github.com/NancyFx/Nancy/pull/2264/files#r53822433.

jchannon added this to the coreclr milestone Feb 9, 2016

Yantrio mentioned this pull request Feb 9, 2016

Get Nancy compiling on dotnet54/dnxcore50 #2220

Closed

12 tasks

jchannon force-pushed the binaryformatterremoval branch from f40a54c to 4a37969 Compare February 9, 2016 15:55

khellang reviewed Feb 9, 2016
View reviewed changes

jchannon mentioned this pull request Feb 10, 2016

JsonDeserializer does not have Deserialise<T> on it #2273

Closed

jchannon force-pushed the coreclr branch 2 times, most recently from e1fe850 to 6714147 Compare February 10, 2016 21:04

jchannon force-pushed the binaryformatterremoval branch from 81a5de9 to 86b71c5 Compare February 13, 2016 12:31

khellang reviewed Feb 15, 2016
View reviewed changes

jchannon force-pushed the binaryformatterremoval branch from 8fa36e7 to 19afc01 Compare February 15, 2016 11:03

jchannon added the Breaking Change label Feb 15, 2016

thecodejunkie reviewed Feb 16, 2016
View reviewed changes

phillip-haydon reviewed Feb 16, 2016
View reviewed changes

jchannon force-pushed the binaryformatterremoval branch from 19afc01 to bf5bba8 Compare February 16, 2016 11:21

jchannon force-pushed the binaryformatterremoval branch from bf5bba8 to 2592da4 Compare February 16, 2016 11:33

jchannon force-pushed the binaryformatterremoval branch 2 times, most recently from ff26612 to 68216cc Compare February 16, 2016 12:08

thecodejunkie force-pushed the coreclr branch from 648c517 to 89f2f34 Compare February 17, 2016 08:57

jchannon force-pushed the binaryformatterremoval branch 3 times, most recently from d158036 to 55391d6 Compare February 17, 2016 17:06

thecodejunkie reviewed Feb 18, 2016
View reviewed changes

jchannon force-pushed the binaryformatterremoval branch 2 times, most recently from fef04ee to 98ff683 Compare February 18, 2016 11:00

replaced binaryformatter with simplejson

14ac689

jchannon force-pushed the binaryformatterremoval branch from 98ff683 to 14ac689 Compare February 18, 2016 11:02

thecodejunkie added a commit that referenced this pull request Feb 18, 2016

Merge pull request #2264 from jchannon/binaryformatterremoval

a8616d8

Removed binaryformatter replaced with json serializer

thecodejunkie merged commit a8616d8 into NancyFx:coreclr Feb 18, 2016

thecodejunkie reviewed Feb 23, 2016
View reviewed changes

Removed binaryformatter replaced with json serializer #2264

Removed binaryformatter replaced with json serializer #2264

Uh oh!

Conversation

jchannon commented Feb 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thecodejunkie commented Feb 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jchannon commented Feb 16, 2016

Uh oh!

thecodejunkie commented Feb 18, 2016

Uh oh!

thecodejunkie commented Feb 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!