Initial "Dotnet list package json output" implementation #4743

erdembayar · 2022-08-02T00:04:07Z

Bug

Regression? Last working version:

Description

Implement Dotnet List Package Machine Readable Json Output spec (spec PR), e.g. dotnet list package --format json will give you json output.
If there any error with json generation then application'll return non-zero value.
This is draft implementation just to check if my technical implementation design going correct direction, if anyone has concern then better to address concern going too far into wrong direction, since it's 1st json output for dotnet so always room for learn and improve. Ideally, we should be MVC style pattern but there's tight coupling between business logic and console output from previous implementation, it's very hard to make MVC style at this moment.
Please just quickly glance at PR and let me know if there are any serious concern, I know implementation is not complete there is some discrepancy what you see here and spec, I'll address them later.

dotnet list package

Project 'MyProjectA' has the following package references
   [netcoreapp3.1]:
   Top-level Package                      Requested             Resolved
   > DotNetNuke.Core                      6.0.0                 6.0.0
   > DotNetZip                            1.0.0                 1.0.0
   > EntityFramework.MappingAPI           *                     6.2.1
   > Microsoft.Extensions.Primitives      [1.0.0, 5.0.0]        1.0.0
   > NuGet.Core                           2.13.0                2.13.0
   > System.CommandLine                   0.3.0-alpha.20070.2   0.3.0-alpha.20070.2
   > Text2Xml.Lib                         [1.1.2, 2.0.0)        1.1.2

Project 'MyProjectB' has the following package references
   [netcoreapp3.1]:
   Top-level Package      Requested             Resolved
   > DotNetNuke.Core      6.0.0                 6.0.0
   > NuGet.Commands       4.8.0-preview3.5278   4.8.0-preview3.5278
   > NuGet.Core           2.13.0                2.13.0
   > Text2Xml.Lib         1.1.2                 1.1.2

   [net5.0]:
   Top-level Package      Requested             Resolved
   > DotNetNuke.Core      6.0.0                 6.0.0
   > NuGet.Commands       4.8.0-preview3.5278   4.8.0-preview3.5278
   > NuGet.Core           2.13.0                2.13.0
   > Text2Xml.Lib         1.1.2                 1.1.2

Here is current basic output looks like for dotnet list package --format json:

{
  "version": 1,
  "parameters": "",
  "projects": [
    {
      "path": "MyProjectA",
      "frameworks": [
        {
          "framework": "netcoreapp3.1",
          "topLevelPackages": [
            {
              "id": "DotNetNuke.Core",
              "requestedVersion": "6.0.0",
              "resolvedVersion": "6.0.0"
            },
            {
              "id": "DotNetZip",
              "requestedVersion": "1.0.0",
              "resolvedVersion": "1.0.0"
            },
            {
              "id": "EntityFramework.MappingAPI",
              "requestedVersion": "*",
              "resolvedVersion": "6.2.1"
            },
            {
              "id": "Microsoft.Extensions.Primitives",
              "requestedVersion": "[1.0.0, 5.0.0]",
              "resolvedVersion": "1.0.0"
            },
            {
              "id": "NuGet.Core",
              "requestedVersion": "2.13.0",
              "resolvedVersion": "2.13.0"
            },
            {
              "id": "System.CommandLine",
              "requestedVersion": "0.3.0-alpha.20070.2",
              "resolvedVersion": "0.3.0-alpha.20070.2"
            },
            {
              "id": "Text2Xml.Lib",
              "requestedVersion": "[1.1.2, 2.0.0)",
              "resolvedVersion": "1.1.2"
            }
          ]
        }
      ]
    },
    {
      "path": "MyProjectB",
      "frameworks": [
        {
          "framework": "netcoreapp3.1",
          "topLevelPackages": [
            {
              "id": "DotNetNuke.Core",
              "requestedVersion": "6.0.0",
              "resolvedVersion": "6.0.0"
            },
            {
              "id": "NuGet.Commands",
              "requestedVersion": "4.8.0-preview3.5278",
              "resolvedVersion": "4.8.0-preview3.5278"
            },
            {
              "id": "NuGet.Core",
              "requestedVersion": "2.13.0",
              "resolvedVersion": "2.13.0"
            },
            {
              "id": "Text2Xml.Lib",
              "requestedVersion": "1.1.2",
              "resolvedVersion": "1.1.2"
            }
          ]
        },
        {
          "framework": "net5.0",
          "topLevelPackages": [
            {
              "id": "DotNetNuke.Core",
              "requestedVersion": "6.0.0",
              "resolvedVersion": "6.0.0"
            },
            {
              "id": "NuGet.Commands",
              "requestedVersion": "4.8.0-preview3.5278",
              "resolvedVersion": "4.8.0-preview3.5278"
            },
            {
              "id": "NuGet.Core",
              "requestedVersion": "2.13.0",
              "resolvedVersion": "2.13.0"
            },
            {
              "id": "Text2Xml.Lib",
              "requestedVersion": "1.1.2",
              "resolvedVersion": "1.1.2"
            }
          ]
        }
      ]
    }
  ]
}

PR Checklist

PR has a meaningful title
PR has a linked issue.
Described changes
Tests
- Automated tests added, I'll add tests after getting confirmation on my current implementation is good enough.
- OR
- Test exception
- OR
- N/A
Documentation
- Documentation PR or issue filled
- OR
- N/A

erdembayar · 2022-08-02T00:05:30Z

@NuGet/nuget-client
Hi team.
Could take quick look at PR and check if my implementation is reasonable? If it looks good, then I'll continue the implementation.

erdembayar · 2022-08-02T00:10:01Z

....CommandLine.XPlat/Commands/PackageReferenceCommands/ListPackage/ListPackageCommandRunner.cs

-                Console.WriteLine(Strings.ListPkg_AutoReferenceDescription);
+                reportRenderer.WriteLine(Strings.ListPkg_AutoReferenceDescription);
+
+                // Should log into json output ?


autoReferenceFound is for the package referenced from sdk.
It's something we didn't consider for spec, so I far I haven't seen actual example of this.

Test a project that targets netstandard2.0. It should have an implicit package reference.

zivkan · 2022-08-02T01:47:01Z

....CommandLine.XPlat/Commands/PackageReferenceCommands/ListPackage/ListPackageCommandRunner.cs

+                reportRenderer.WriteLine();
+                reportRenderer.WriteLine(Strings.ListPkg_SourcesUsedDescription);
+                ProjectPackagesPrintUtility.PrintSources(listPackageArgs);
+                reportRenderer.WriteLine();


I think that this, coupled with the fact that JsonRenderer has a bunch of methods that are no-ops, the text renderer also has some methods that are no-ops, and the fact that there's a if (render is JsonRenderer) below, that there isn't a good match in abstractions.

Effectively, this ListPackageCommandRunner is written in a way that orders its work based on the console output, and then you're trying to bolt a json renderer on top of this.

Instead, I propose something similar to a web MVC architecture. A "controller" which gets the information for a single project (the model), and then the view (renderer) knows how to render one of these models. Something like:

renderer.Start(); // no-op for command line, for json it writes `{` and version property foreach (var project in allProjects) { // args is set up before the loop, from the command line arguments. It tells the controller // what properties of the model to populate, which is how it will know to be "offline only" // for certain text output scenarios. var model = await controller.GetPackagesAsync(project, args); renderer.Write(project); } renderer.End(); // json writes errors, other info?. does console output any "wrapping up" info?

Honestly, the "model" could actually be the model for JSON output, and then the renderer is little more than a JsonSerializer.Serialize(console.out, model), and the only place "logic" is needed is the text renderer.

This way IReportRenderer doesn't need a bunch of methods that are only implemented in one of the two renderers.

I think that this, coupled with the fact that JsonRenderer has a bunch of methods that are no-ops, the text renderer also has some methods that are no-ops, and the fact that there's a if (render is JsonRenderer) below, that there isn't a good match in abstractions.

Removed if (render is JsonRenderer) logic, it can be abstract away.

Effectively, this ListPackageCommandRunner is written in a way that orders its work based on the console output, and then you're trying to bolt a json renderer on top of this.

Instead, I propose something similar to a web MVC architecture. A "controller" which gets the information for a single project (the model), and then the view (renderer) knows how to render one of these models. Something like:

renderer.Start(); // no-op for command line, for json it writes `{` and version property foreach (var project in allProjects) { // args is set up before the loop, from the command line arguments. It tells the controller // what properties of the model to populate, which is how it will know to be "offline only" // for certain text output scenarios. var model = await controller.GetPackagesAsync(project, args); renderer.Write(project); } renderer.End(); // json writes errors, other info?. does console output any "wrapping up" info?

Honestly, the "model" could actually be the model for JSON output, and then the renderer is little more than a JsonSerializer.Serialize(console.out, model), and the only place "logic" is needed is the text renderer.

This way IReportRenderer doesn't need a bunch of methods that are only implemented in one of the two renderers.

Thank you for feedback.

With this approach we couldn't print all the problems on top of json report, instead it would be written to the bottom of report. Is it ok?
Current json template is

{ "version": 1, "parameters": "", "problems": [ { "project": "src/lib/MyProjectB.csproj", "message": "The project `C:/Users/userA/repos/MainApp/src/lib/MyProjectB.csproj`` uses package.config for NuGet packages, while the command works only with package reference projects." } ], "projects": [ { ... } ] }

Instead, we would do:

{ "version": 1, "parameters": "", "projects": [ { ... } ], "problems": [ { "project": "src/lib/MyProjectB.csproj", "message": "The project `C:/Users/userA/repos/MainApp/src/lib/MyProjectB.csproj`` uses package.config for NuGet packages, while the command works only with package reference projects." } ] }

Also making it MVC style means I need to completely rewrite the current console output logic too. I'm not sure unless I divide into phases not sure how much additional effort it would take.

With this approach we couldn't print all the problems on top of json report, instead it would be written to the bottom of report. Is it ok?

Two things:

Section 1 of the JSON specification says:

An object is an unordered collection of zero or more name/value
pairs, where a name is a string and a value is a string, number,
boolean, null, object, or array.

An array is an ordered sequence of zero or more values.

In other words, it is "invalid" to depend on specific order of properties in an object. Having said that, we really, really need version to be the very first property so that tools can implement polymorphic deserialization without buffering the entire json (for when a version 2 output eventually exists).

The JSON renderer's implementation of void Write(ProjectInfo) could be just to add the object into a list, and it doesn't actually output anything until the void End() at the end of the loop.

The big picture I was trying to raise awareness over is overall readability and "software architecture". Let's avoid leaky abstractions. Don't get bogged down in the example comments that were designed to help understand the API design. How different renderers implement the APIs are implementation specific.

Section 1 of the JSON specification says:

An object is an unordered collection of zero or more name/value
pairs, where a name is a string and a value is a string, number,
boolean, null, object, or array.
An array is an ordered sequence of zero or more values.

In other words, it is "invalid" to depend on specific order of properties in an object. Having said that, we really, really need version to be the very first property so that tools can implement polymorphic deserialization without buffering the entire json (for when a version 2 output eventually exists).

Of course, it doesn't make any difference for machine which order is objects are in json, but someone looking at the report have to scroll down all the way to find out what was the issue (for example admin looking at non-zero exit code from json output, in case there're hundreds of projects and thousands of dependencies then finding what went wrong requires writing own parse). Current structure allows to find out if there was issue at 1st glance and it's consistent what npm -ls json doing it, I feel having errors/problems at top is more user friendly.

The JSON renderer's implementation of void Write(ProjectInfo) could be just to add the object into a list, and it doesn't actually output anything until the void End() at the end of the loop.

I added void Write(ReportProject reportProject) method into IRenderer interface and Void End() is doing the action rendering, maybe this could be not enough to address your concern, let me know if I need to make more drastic change.

but someone looking at the report have to scroll down all the way to find out what was the issue

The feature is called "machine readable". You're right that this doesn't mean we should make it difficult for humans to read. But customers are expected to use it in scripts or pipe to other commands, not to output to the console and look at manually. Those scripts they create can reply the errors to their CI logs, to the console, to whereever. Alternatively, if the customer runs the command without --format json, we expect them to see the same errors (but in a more human friendly text output).

I feel having errors/problems at top is more user friendly.

Only if the output is piped to something like more or less, right? If you just run dotnet list pacakges --format json and there are pages and pages of output, when the terminal stops scrolling, you're only going to see what's at the end, not at the beginning.

I added void Write(ReportProject reportProject) method into IRenderer interface and Void End() is doing the action rendering, maybe this could be not enough to address your concern, let me know if I need to make more drastic change.

I feel that since both the text renderer and json renderer have interface methods which they ignore, it's a code smell that it's not well designed. I get that it's quicker because you don't need to de-tangle business logic from presentation. But think about it from the point of view of someone who hasn't joined the NuGet team yet (or who doesn't review this PR). In the future they'll get some work item to work on dotnet list package, they'll be reading the code for the first time, and trying to understand why it's calling renderer.WriteLine(someString) followed by renderer.Write(ProjectInfo). Why doesn't renderer.Write(ProjectInfo generate someString itself? Another also, I know this isn't a performance hot path, but all those times where the business logic does renderer.WriteLine(string.Format(...)), the runtime will need to parse the format string, create a string builder, create the string, return to caller, which then gets passed to the json renderer, just to ignore it. Not a performance hot path, but every code path is an opportunity to practise best practices.

I feel that since both the text renderer and json renderer have interface methods which they ignore, it's a code smell that it's not well designed.

I agree with this.

The abstraction is there in order to facilitate fewer code changes which isn't good design.

Part of the reason why we've always had trouble with making changes to dotnet list package has been the lack of proper abstractions. I feel like this exacerbates that.

In other words, there's an interface with 10 methods, but only 5 of those methods are actually common.

src/NuGet.Core/NuGet.CommandLine.XPlat/ReportRenderers/JsonRenderer/JsonOutputContent.cs

…her dotnet command like 'dotnet nuget list source' besides current 'dotnet list package'

dominoFire · 2022-08-03T00:22:49Z

src/NuGet.Core/NuGet.CommandLine.XPlat/ReportRenderers/IReportRenderer.cs

+
+namespace NuGet.CommandLine.XPlat
+{
+    internal interface IReportRenderer


Suggestion: Make the IReportRendered independent of exit code

classDiagram class IReportRenderer~T~:::abstract { TextWriter writer WriteOutput(T reportContent) } IReportRenderer~T~ <|-- ConsoleWriter : implements IReportRenderer~T~ <|-- JsonPackageRendered : implements

Loading

Maybe rename ConsoleWriter, JsonPackageRendered types to reflect that they implement IReportRenderer

I think exit code is independent of text output.

I left the same feedback as @dominoFire in a separate comment.

nkolev92

Big issue why you're having a tough time writing easy to follow code is the lack of good abstractions in this implementation.

I feel like this exacerbates that.
dotnet list package is a command that's had a lot of changes/additions. I think we should take the time to get it done correctly so that we don't have these issues in the future.

Fortunately, not of these types are public APIs, so we can do better.

nkolev92 · 2022-08-03T00:38:07Z

.../NuGet.CommandLine.XPlat/Commands/PackageReferenceCommands/ListPackage/ListPackageCommand.cs

+            IReportRenderer jsonReportRenderer;
+
+            var currentlySupportedReportVersions = new List<string> { "1" };
+            // If customer pass unsupported version then default to latest available version and warn about unsupported version.


The comment here is warn, but we seem to log jsonReportRenderer.WriteErrorLine.

Shouldn't this be a 1 exit code, invalid input?
Seems better than ignoring a bad parameter.

nkolev92 · 2022-08-03T00:40:52Z

....CommandLine.XPlat/Commands/PackageReferenceCommands/ListPackage/ListPackageCommandRunner.cs

+                reportRenderer.WriteLine();
+                reportRenderer.WriteLine(Strings.ListPkg_SourcesUsedDescription);
+                ProjectPackagesPrintUtility.PrintSources(listPackageArgs);
+                reportRenderer.WriteLine();


I feel that since both the text renderer and json renderer have interface methods which they ignore, it's a code smell that it's not well designed.

I agree with this.

The abstraction is there in order to facilitate fewer code changes which isn't good design.

Part of the reason why we've always had trouble with making changes to dotnet list package has been the lack of proper abstractions. I feel like this exacerbates that.

In other words, there's an interface with 10 methods, but only 5 of those methods are actually common.

nkolev92 · 2022-08-03T00:42:04Z

.../NuGet.CommandLine.XPlat/Commands/PackageReferenceCommands/ListPackage/ListPackageCommand.cs

+            {
+                try
+                {
+                    outputFormat = EnumExtensions.GetValueFromName<ReportOutputFormat>(outputFormatOption);


why not use enum parse?

As you can see enum parsing is case sensitive. Another way of dealing with this one is make upper case the outputFormatOption and also create enum all Upper case too.

nkolev92 · 2022-08-03T00:42:28Z

.../NuGet.CommandLine.XPlat/Commands/PackageReferenceCommands/ListPackage/ListPackageCommand.cs

+                }
+                catch (ArgumentException)
+                {
+                    string currentlySupportedFormat = "console, json";


This should be generated by looking at all the values of the enum, not by hardcoding it.

This one is temporary; I want feedback on main design.

nkolev92 · 2022-08-03T00:44:18Z

....CommandLine.XPlat/Commands/PackageReferenceCommands/ListPackage/ListPackageCommandRunner.cs

            }
+
+            listPackageArgs.Renderer.End();
+            return listPackageArgs.Renderer.ExitCode();


The renderer having the exit code feels weird.

Roughly, the design should be something that returns a model and then something else that renders that.

The model should contain the failure information.

nkolev92 · 2022-08-03T00:45:04Z

src/NuGet.Core/NuGet.CommandLine.XPlat/ReportRenderers/IReportRenderer.cs

+
+namespace NuGet.CommandLine.XPlat
+{
+    internal interface IReportRenderer


I left the same feedback as @dominoFire in a separate comment.

ghost · 2022-08-16T19:01:25Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

erdembayar added 16 commits July 25, 2022 14:18

Add output format and output version

d7535fc

Add JsonRenderer

1dabdd3

Add v1 of JsonRenderer

df91c6b

Add v1 of JsonRenderer

f821e72

Replace console.write with generic IRenderer.Write

e6fdae1

Start implementing JsonRenderer

962fde3

Introduce non-zero return value from dotnet list package

eb7024d

Replace all Console instance with Renderer

286174c

Start implementing JsonRenderer

3c6862c

Introduce column naming enum

70795ba

Separate Vulnurability info

4090a84

Fix vulnerability data column after refactoring

31b187b

Pass data for JsonRendering

4457630

Fix test

88bad21

Basic json generation

7a56481

Fix basic json output

a45bbd0

erdembayar marked this pull request as ready for review August 2, 2022 00:05

erdembayar requested a review from a team as a code owner August 2, 2022 00:05

erdembayar commented Aug 2, 2022

View reviewed changes

Clean up

a68b183

erdembayar force-pushed the dev-eryondon-dotnetlistpackage-jsonoutput branch from 0fbca57 to a68b183 Compare August 2, 2022 00:12

zivkan reviewed Aug 2, 2022

View reviewed changes

erdembayar force-pushed the dev-eryondon-dotnetlistpackage-jsonoutput branch from 4af16f5 to a68b183 Compare August 2, 2022 03:57

Remove check if JsonRenderer

0f29d38

dominoFire reviewed Aug 2, 2022

View reviewed changes

src/NuGet.Core/NuGet.CommandLine.XPlat/ReportRenderers/JsonRenderer/JsonOutputContent.cs Outdated Show resolved Hide resolved

erdembayar force-pushed the dev-eryondon-dotnetlistpackage-jsonoutput branch from 5571189 to 3b6d90c Compare August 2, 2022 17:09

Abstract write project data

c371ad0

erdembayar force-pushed the dev-eryondon-dotnetlistpackage-jsonoutput branch from 3b6d90c to c371ad0 Compare August 2, 2022 17:51

Clean up

8139fe1

erdembayar force-pushed the dev-eryondon-dotnetlistpackage-jsonoutput branch from a980649 to 8139fe1 Compare August 2, 2022 21:02

Make JsonRenderer more abstract for possible feature extension for ot…

13d3c84

…her dotnet command like 'dotnet nuget list source' besides current 'dotnet list package'

erdembayar force-pushed the dev-eryondon-dotnetlistpackage-jsonoutput branch from 2422c73 to 13d3c84 Compare August 2, 2022 21:28

dominoFire reviewed Aug 3, 2022

View reviewed changes

nkolev92 reviewed Aug 3, 2022

View reviewed changes

Error on bad version.

2ea08a8

erdembayar marked this pull request as draft August 9, 2022 17:15

ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Aug 16, 2022

ToneyAlexander mentioned this pull request Aug 22, 2022

Add errors handling scenario for json output spec NuGet/Home#11983

Merged

ghost closed this Aug 23, 2022

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial "Dotnet list package json output" implementation #4743

Initial "Dotnet list package json output" implementation #4743

erdembayar commented Aug 2, 2022 •

edited

Loading

erdembayar commented Aug 2, 2022

erdembayar Aug 2, 2022 •

edited

Loading

zivkan Aug 2, 2022

zivkan Aug 2, 2022 •

edited

Loading

erdembayar Aug 2, 2022 •

edited

Loading

zivkan Aug 2, 2022

erdembayar Aug 2, 2022 •

edited

Loading

zivkan Aug 2, 2022

nkolev92 Aug 3, 2022 •

edited

Loading

dominoFire Aug 3, 2022 •

edited

Loading

nkolev92 Aug 3, 2022

nkolev92 left a comment

nkolev92 Aug 3, 2022

nkolev92 Aug 3, 2022 •

edited

Loading

nkolev92 Aug 3, 2022

erdembayar Aug 3, 2022

nkolev92 Aug 3, 2022

erdembayar Aug 3, 2022

nkolev92 Aug 3, 2022

nkolev92 Aug 3, 2022

ghost commented Aug 16, 2022

Initial "Dotnet list package json output" implementation #4743

Initial "Dotnet list package json output" implementation #4743

Conversation

erdembayar commented Aug 2, 2022 • edited Loading

Bug

Description

PR Checklist

erdembayar commented Aug 2, 2022

erdembayar Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zivkan Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

erdembayar Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkolev92 Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

dominoFire Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkolev92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkolev92 Aug 3, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 16, 2022

erdembayar commented Aug 2, 2022 •

edited

Loading

erdembayar Aug 2, 2022 •

edited

Loading

zivkan Aug 2, 2022 •

edited

Loading

erdembayar Aug 2, 2022 •

edited

Loading

erdembayar Aug 2, 2022 •

edited

Loading

nkolev92 Aug 3, 2022 •

edited

Loading

dominoFire Aug 3, 2022 •

edited

Loading

nkolev92 Aug 3, 2022 •

edited

Loading