[MDEP-799] - improve dependency:tree to add optional JSON output of the results #207

kezhenxu94 · 2022-03-13T04:26:33Z

Following this checklist to help us incorporate your
contribution quickly and easily:

Make sure there is a JIRA issue filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [MDEP-XXX] - Fixes bug in ApproximateQuantiles,
where you replace MDEP-XXX with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean verify to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
You have run the integration tests successfully (mvn -Prun-its clean verify).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

(I'm Apache Member and I've signed ICLA)

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

src/main/java/org/apache/maven/plugins/dependency/tree/JSONDependencyNodeVisitor.java

slawekjaranowski

why json is created manually? Maybe use FasterXML
missing documentation, I propose separate page with output example and description

slawekjaranowski · 2022-03-25T12:36:35Z

src/main/java/org/apache/maven/plugins/dependency/tree/TreeMojo.java

+        else if ( "json".equals( outputType ) )
+        {
+            return new JSONDependencyNodeVisitor( writer );
+        }


can be replaced by switch statement

slawekjaranowski · 2022-03-25T12:38:08Z

src/test/java/org/apache/maven/plugins/dependency/tree/TestTreeMojo.java

+     *
+     * @throws Exception in case of an error.
+     */
+    public void _testTreeJSONSerializing()


Please write dedicated test for new class JSONDependencyNodeVisitor

kezhenxu94 · 2022-05-19T04:16:17Z

why json is created manually? Maybe use FasterXML

Is it OK to add some dependencies? I thought you might want to keep the dependencies minimal.

missing documentation, I propose separate page with output example and description

OK, will add doc

slawekjaranowski · 2022-08-27T10:24:03Z

why json is created manually? Maybe use FasterXML

Is it OK to add some dependencies? I thought you might want to keep the dependencies minimal.

It is ok if meets: https://maven.apache.org/developers/dependency-policies.html

missing documentation, I propose separate page with output example and description

OK, will add doc

elharo · 2023-03-17T21:39:23Z

src/main/java/org/apache/maven/plugins/dependency/tree/JSONDependencyNodeVisitor.java

+    /**
+     * Constructor.
+     *
+     * @param writer the writer to write to.


nit: no period

elharo · 2023-03-17T21:40:43Z

src/main/java/org/apache/maven/plugins/dependency/tree/JSONDependencyNodeVisitor.java

+    {
+        Artifact artifact = node.getArtifact();
+
+        writer.println( indentations( indent ) + "{" );


We shouldn't be using a PrintWriter here. They swallow IOExceptions. Even if the superclass already does this, let's not make the same mistake again.

elharo · 2023-03-17T21:43:08Z

src/main/java/org/apache/maven/plugins/dependency/tree/JSONDependencyNodeVisitor.java

+
+        writer.println( indentations( indent ) + "{" );
+        indent++;
+        String groupId = indentations( indent ) + "\"groupId\": \"" + artifact.getGroupId() + "\"";


I think manually constructing the JSON is preferable to adding another library. (Some JSON libraries due have security issues.)

elharo · 2023-03-17T21:43:40Z

src/main/java/org/apache/maven/plugins/dependency/tree/JSONDependencyNodeVisitor.java

+        String[] elements = root ? new String[] { groupId, artifactId, version, type }
+                                 : new String[] { groupId, artifactId, version, type, scope };
+
+        writer.print( StringUtils.join( "," + System.lineSeparator(), elements ) );


Please don't system dependent output like System.lineSeparator(). Specify the character being output.

Why, a PrintWriter will do the same?!

Yes, and that's another reason we shouldn't be using PrintWriter

monperrus · 2023-05-22T13:55:59Z

super cool feature, hope we can get it merged!

MartinWitt · 2023-05-22T17:24:10Z

Hey, as this PR seems stale, and I want this feature, I would implement this in a new PR. Do you prefer to use a JSON library, which one do you like, or shall I write a JSON printer myself?

elharo · 2023-05-22T18:14:24Z

I strongly prefer no extra dependencies for this. JSON libraries in particular are a world of security bugs, unmaintained code, violations of Java conventions, and overall poor design. For JSON output, writing strings is straight-forward. A library shouldn't be necessary to implement this.

Possibly tests (and only tests) might want to parse the JSON, and for that a library would be helpful. Do not use Jackson. javax.json or perhaps GSON might be OK.

michael-o · 2023-05-22T18:17:18Z

I strongly prefer no extra dependencies for this. JSON libraries in particular are a world of security bugs, unmaintained code, violations of Java conventions, and overall poor design. For JSON output, writing strings is straight-forward. A library shouldn't be necessary to implement this.

Possibly tests (and only tests) might want to parse the JSON, and for that a library would be helpful. Do not use Jackson. javax.json or perhaps GSON might be OK.

GSON is superseded already.

elharo · 2023-05-22T19:57:03Z

I strongly prefer no extra dependencies for this. JSON libraries in particular are a world of security bugs, unmaintained code, violations of Java conventions, and overall poor design. For JSON output, writing strings is straight-forward. A library shouldn't be necessary to implement this.
Possibly tests (and only tests) might want to parse the JSON, and for that a library would be helpful. Do not use Jackson. javax.json or perhaps GSON might be OK.

GSON is superseded already.

By what?

cstamas · 2023-05-23T17:37:01Z

What makes you think that "gson is superseded"?

michael-o · 2023-05-23T17:39:17Z

What makes you think that "gson is superseded"?

There is a while post about it.

cstamas · 2023-05-23T17:40:35Z

Where?

michael-o · 2023-05-23T18:28:24Z

Where?

Searching, it was from the original authors, maybe even Kevin Bourillion.

cstamas · 2023-05-23T18:50:24Z

AFAIK, GSON was "just" moved out from Google umbrella, and is in maintenance mode, as it is feature complete (or something, dunno, guessing), but the project is quite lively and have regular releases and fixes.

elharo · 2023-05-23T19:27:16Z

While I was at Google, I did successfully lobby to get GSON adopted and officially supported by the Java core libraries team. This was part of my then day job managing dependencies for the Google Cloud Java client libraries, which depended quite heavily on GSON. That was a couple of years ago. Things might have changed since, but anything you heard about the project pre-2021 is certainly out of date.

elharo · 2024-05-23T16:44:47Z

feature complete under another PR

michael-o reviewed Mar 13, 2022

View reviewed changes

src/main/java/org/apache/maven/plugins/dependency/tree/JSONDependencyNodeVisitor.java Outdated Show resolved Hide resolved

[MDEP-799] tree: add optional output type json

11ba0c9

kezhenxu94 force-pushed the MDEP-799 branch from a6bf220 to 11ba0c9 Compare March 13, 2022 11:10

michael-o self-requested a review March 13, 2022 11:16

slawekjaranowski requested changes Mar 25, 2022

View reviewed changes

michael-o removed their request for review April 12, 2022 11:37

elharo requested changes Mar 17, 2023

View reviewed changes

MartinWitt mentioned this pull request May 22, 2023

[MDEP-799] tree: add optional output type json #325

Closed

8 tasks

elharo closed this May 23, 2024

kezhenxu94 deleted the MDEP-799 branch May 24, 2024 06:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MDEP-799] - improve dependency:tree to add optional JSON output of the results #207

[MDEP-799] - improve dependency:tree to add optional JSON output of the results #207

kezhenxu94 commented Mar 13, 2022

slawekjaranowski left a comment

slawekjaranowski Mar 25, 2022

slawekjaranowski Mar 25, 2022

kezhenxu94 commented May 19, 2022

slawekjaranowski commented Aug 27, 2022

elharo Mar 17, 2023

elharo Mar 17, 2023

elharo Mar 17, 2023

elharo Mar 17, 2023

michael-o Mar 17, 2023

elharo Mar 17, 2023

monperrus commented May 22, 2023

MartinWitt commented May 22, 2023

elharo commented May 22, 2023

michael-o commented May 22, 2023

elharo commented May 22, 2023

cstamas commented May 23, 2023

michael-o commented May 23, 2023

cstamas commented May 23, 2023

michael-o commented May 23, 2023

cstamas commented May 23, 2023

elharo commented May 23, 2023 •

edited

elharo commented May 23, 2024

[MDEP-799] - improve dependency:tree to add optional JSON output of the results #207

[MDEP-799] - improve dependency:tree to add optional JSON output of the results #207

Conversation

kezhenxu94 commented Mar 13, 2022

slawekjaranowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kezhenxu94 commented May 19, 2022

slawekjaranowski commented Aug 27, 2022

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

monperrus commented May 22, 2023

MartinWitt commented May 22, 2023

elharo commented May 22, 2023

michael-o commented May 22, 2023

elharo commented May 22, 2023

cstamas commented May 23, 2023

michael-o commented May 23, 2023

cstamas commented May 23, 2023

michael-o commented May 23, 2023

cstamas commented May 23, 2023

elharo commented May 23, 2023 • edited

elharo commented May 23, 2024

elharo commented May 23, 2023 •

edited