[Prototype] Use JsonDeserializer in Microsoft.Framework.Runtime #1779

troydai · 2015-05-05T00:04:57Z

Address issue: #30

JsonDeserializer is entirely internal to Microsoft.Framework.Runtime.

Logic of writing lock file is moved to Microsoft.Framework.PackageManager, where Newtonsoft.Json will be referenced.

All exceptions are updated.

Unit tests are added to covert JsonDeserializer

Maxwe11 · 2015-05-05T09:26:36Z

src/Microsoft.Framework.Runtime.Hosting/Json/JsonObject.cs

+        {
+            return ValueAs(key, value =>
+            {
+                if (value != null && value is JsonBoolean)


condition could be simplified:
a) as cast and check on null
or
b) remove null check because if value is null is will return false

Eilon · 2015-05-05T16:37:51Z

src/Microsoft.Framework.Runtime.Hosting/Json/JsonContent.cs

+        /// <summary>
+        /// Current line number in zero-based index.
+        /// </summary>
+        public int CurrentLine { get; private set; } = 0;


0 is the default value.

Eilon · 2015-05-05T17:08:53Z

BTW several other style issues (multiple types per file, var, etc.) but I won't comment on them further because this is a prototype.

victorhurdugaci · 2015-05-05T17:21:43Z

So this json deserializer doesn't support comments? That's a breaking change

troydai · 2015-05-05T17:36:47Z

Two features are required not to break current scenario:

Comments support;
Redundant trailing comma.

troydai · 2015-05-05T17:38:58Z

@davidfowl @Eilon @victorhurdugaci
There are quite a few implementations I want to improve, but not until we have a evaluation on the prototype and decide whether we want to move forward with additional features.

Eilon · 2015-05-05T17:55:23Z

@victorhurdugaci BTW it's not a "breaking change" in the traditional change because this is all new... but it would be breaking from previous previews, of course. But I agree with @troydai that in the meantime we'll do things one at a time in terms of honing the prototype into production quality.

victorhurdugaci · 2015-05-05T17:56:57Z

Then we need to update some project.json files like this one: https://github.com/aspnet/MusicStore/blob/dev/src/MusicStore/project.json

Eilon · 2015-05-05T17:56:59Z

@troydai BTW how close is this code to the stuff I sent you? Is the basic parsing logic the same, except that you added the line number support etc.? Are there any other significant differences? I ask because the old parsing code was fairly robust; it was just missing some desirable features (line numbers, comment support, etc.).

troydai · 2015-05-05T18:00:43Z

I make sure the the original parsing logic is kept the same, what I added is the line information support. But adding line information support introduces extract logic to MoveNext and MovePrev, which open to potential bugs. Besides that the deserializing logic itself is intact.

Eilon · 2015-05-05T18:06:21Z

@victorhurdugaci I think before we take this change we'd have to support comments (assuming we want to continue to support comments, which I hope we do).

Eilon · 2015-05-05T18:06:33Z

@troydai great!

davidfowl · 2015-05-05T18:07:53Z

Feels like the parser needs to be trimmed down for things we don't need to care about:

Utf16StringValidator.cs (do we need it)
Json* types (why do we need them?), Dictionary<string, object>, string, int, primitive types should be enough right?
Depth checking (why bother)? Do we need this safety?

troydai · 2015-05-05T18:16:55Z

Types are for line information. By combining the line formation and data in one class, it helps to consume the line information easily.

BrennanConroy · 2015-05-05T20:35:17Z

src/Microsoft.Framework.PackageManager/Restore/LockFileWriter.cs

+        private JToken WritePackageReferenceSet(PackageReferenceSet item)
+        {
+            var json = new JObject();
+            json["targetFramework"] = item.TargetFramework?.ToString();


Is possibly assigning null here okay?

Fixing LockFileWriter is not in the scope of this PR. This is merely copied from LockFileFormat to separate read and write.

victorhurdugaci · 2015-05-09T04:56:27Z

src/Microsoft.Framework.Runtime.Hosting/Json/JsonDeserializerResource.cs

+            return string.Format("Invalid Unicode [{0}]", unicode);
+        }
+
+        internal static string Format_UnfinishedJSON(string nextTokenValue)


What is the diff between this and JSON_InvalidEnd ?

Invalid end is the file finishes to early. This is file finishes to late, there are unprocessed content for example:

{} {}

troydai · 2015-05-11T16:43:52Z

@Eilon @victorhurdugaci ⌚

troydai · 2015-05-11T17:55:38Z

Rebased and ready for reviews.

Eilon · 2015-05-11T21:31:33Z

for me - I don't think that me looking at it anymore will yield anything useful 😄

troydai · 2015-05-11T21:41:01Z

Yeah. I collected one squirrel now.

victorhurdugaci · 2015-05-11T21:44:25Z

src/Microsoft.Framework.Runtime.Hosting/FileFormatException.cs

+            if (exception is JsonDeserializerException)
+            {
+                return new FileFormatException(exception.Message, exception)
+                   .WithFilePath(filePath)


I like this fluent api :)

victorhurdugaci · 2015-05-11T21:47:05Z

🚢 it

troydai · 2015-05-11T22:15:10Z

All right, I got a squirrel sitting on a ship now. All what I need is @davidfowl screaming at me one more time....

JamesNK · 2015-05-12T08:16:30Z

If you want ideas for crazy JSON to throw at your reader, there are a ton of examples here: https://github.com/JamesNK/Newtonsoft.Json/blob/master/Src/Newtonsoft.Json.Tests/JsonTextReaderTest.cs

Lots of stuff I read is not technically JSON however. Multi-line strings, date constructors, octal numbers, etc.

davidfowl · 2015-05-12T08:19:28Z

@JamesNK This parser is really for 2 things:

Don't use JSON.NET before the app's dependency graph loads
Performance loading 3 known file formats, project.lock.json, project.json, global.json.

We're going to use JSON.NET for everything else.

JamesNK · 2015-05-12T08:28:54Z

I know. Just letting you know that you either need to handle those examples or fail well. They're what I've built up over the years.

davidfowl · 2015-05-12T08:29:41Z

@JamesNK thanks for the input. Hopefully this will be used on those known quantities and we won't have to handle anything like that.

davidfowl · 2015-05-12T08:33:10Z

src/Microsoft.Framework.PackageManager/Restore/LockFileWriter.cs

@@ -0,0 +1,247 @@
+// Copyright (c) .NET Foundation. All rights reserved.


I think you should move the old lock file reader writer logic as a whole to the PackageManager and create a copy of the writer (unshared) in the runtime itself. The PackageManager needs to read the lock file as well but it can use the normal JSON.NET stuff.

All right. I'll do that.

davidfowl · 2015-05-12T08:44:58Z

A few minor comments but it looks great!

I have some things to follow up on after this initial checkin to do with performance 😄 !

1. Implement an internal JSON parser in runtime; 2. Remove dependency on Newtonsoft.Json from Microsoft.Framework.Runtime; 3. Move the LockFileFormat to Microsoft.Framework.PackageManager. Leave a copy of which contains only the reading logic. Rename it to LockFileReader; 4. Make LockFileReader using the internal JSON parser; 5. Updat tests.

ghost added the cla-already-signed label May 5, 2015

Maxwe11 reviewed May 5, 2015
View reviewed changes

Eilon mentioned this pull request May 5, 2015

Decouple Microsoft.Framework.Runtime from Newtonsoft.Json #1778

Closed

Eilon reviewed May 5, 2015
View reviewed changes

BrennanConroy reviewed May 5, 2015
View reviewed changes

victorhurdugaci reviewed May 9, 2015
View reviewed changes

troydai force-pushed the trdai/json branch from c8e8861 to e0c2629 Compare May 11, 2015 17:45

victorhurdugaci reviewed May 11, 2015
View reviewed changes

davidfowl reviewed May 12, 2015
View reviewed changes

troydai force-pushed the trdai/json branch 2 times, most recently from 38faef4 to e36657d Compare May 12, 2015 18:15

This was referenced May 12, 2015

Remove comments from project.json aspnet/Security#252

Merged

Remove coments from project.json aspnet/Entropy#61

Closed

troydai force-pushed the trdai/json branch from e36657d to f0a852a Compare May 12, 2015 18:49

troydai merged commit f0a852a into dev May 12, 2015

troydai deleted the trdai/json branch May 12, 2015 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Prototype] Use JsonDeserializer in Microsoft.Framework.Runtime #1779

[Prototype] Use JsonDeserializer in Microsoft.Framework.Runtime #1779

troydai commented May 5, 2015

Maxwe11 May 5, 2015

Eilon May 5, 2015

Eilon commented May 5, 2015

victorhurdugaci commented May 5, 2015

troydai commented May 5, 2015

troydai commented May 5, 2015

Eilon commented May 5, 2015

victorhurdugaci commented May 5, 2015

Eilon commented May 5, 2015

troydai commented May 5, 2015

Eilon commented May 5, 2015

Eilon commented May 5, 2015

davidfowl commented May 5, 2015

troydai commented May 5, 2015

BrennanConroy May 5, 2015

troydai May 7, 2015

victorhurdugaci May 9, 2015

troydai May 9, 2015

troydai commented May 11, 2015

troydai commented May 11, 2015

Eilon commented May 11, 2015

troydai commented May 11, 2015

victorhurdugaci May 11, 2015

victorhurdugaci commented May 11, 2015

troydai commented May 11, 2015

JamesNK commented May 12, 2015

davidfowl commented May 12, 2015

JamesNK commented May 12, 2015

davidfowl commented May 12, 2015

davidfowl May 12, 2015

troydai May 12, 2015

davidfowl commented May 12, 2015

		@@ -0,0 +1,247 @@
		// Copyright (c) .NET Foundation. All rights reserved.

[Prototype] Use JsonDeserializer in Microsoft.Framework.Runtime #1779

[Prototype] Use JsonDeserializer in Microsoft.Framework.Runtime #1779

Conversation

troydai commented May 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eilon commented May 5, 2015

victorhurdugaci commented May 5, 2015

troydai commented May 5, 2015

troydai commented May 5, 2015

Eilon commented May 5, 2015

victorhurdugaci commented May 5, 2015

Eilon commented May 5, 2015

troydai commented May 5, 2015

Eilon commented May 5, 2015

Eilon commented May 5, 2015

davidfowl commented May 5, 2015

troydai commented May 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troydai commented May 11, 2015

troydai commented May 11, 2015

Eilon commented May 11, 2015

troydai commented May 11, 2015

Choose a reason for hiding this comment

victorhurdugaci commented May 11, 2015

troydai commented May 11, 2015

JamesNK commented May 12, 2015

davidfowl commented May 12, 2015

JamesNK commented May 12, 2015

davidfowl commented May 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented May 12, 2015