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

MartinWitt · 2023-05-22T20:37:44Z

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.

I will squash and update the commit in the end. There is still open discussion.

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).
I hereby declare this contribution to be licensed under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

I sent the mail for this a few minutes ago so it should be signed soon.

I tried to understand the way you write unit/integration tests but it looks a bit complicated. What is the correct way to add a testcase? Best case, I can add a pom and check if the model written as json -> read by any JSON parser is equal to the model before.

The JSON files currently exist for you to see the current printed format. It will be deleted afterward.

This PR adds JSON output type to the maven-depdenceny-plugin with the goal tree.

Supersedes #207

MartinWitt · 2023-05-23T17:30:32Z

CLA is now signed and accepted.

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

MartinWitt · 2023-05-29T11:28:00Z

As I am a first-time contributor, someone has to approve the workflow run before we see CI results.

MartinWitt · 2023-06-14T22:31:40Z

@elharo could you approve the worflow run?

elharo · 2023-06-14T23:17:33Z

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

+    }
+
+    @Override
+    public boolean visit(DependencyNode node) {


Is it possible for this to et stuck in an infinite recursion with a maliciously hand-crafted dependency node? Or for that matter, with a non-maliciious but buggy circular dependency tree (which does happen)?

@elharo the cure may be to keep track of already visited nodes and either fail or keep trucking?

add a test for this case, see what works

elharo · 2023-06-14T23:18:01Z

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

+ */
+public class JsonDependencyNodeVisitor extends AbstractSerializingVisitor implements DependencyNodeVisitor {
+
+    private static final String LINE_SEPARATOR = "\n";


inline this, a simple "\n" is clearer

elharo · 2023-06-14T23:18:13Z

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

+        }
+    }
+    /**
+     * Appends the node and it's children to the string builder.


elharo · 2023-06-14T23:19:00Z

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

+     * @param key  the key used as json key
+     * @param value  the value used as json value
+     */
+    private void appendKey(StringBuilder sb, int indent, String key, String value) {


appendKeyValue

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

elharo · 2023-06-14T23:21:43Z

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

+     */
+    public void _testTreeJsonSerialzing() throws Exception {
+        List<String> contents = runTreeMojo("tree1.json", "json");
+        assertTrue(findString(contents, "\"testGroupId\": \"project\""));


not sure why findString is used here. contains is simple and more obvious

I tried to keep the test case in the same style as the rest of the class. But I can change it.

contains is clearer

I want to double check: do you mean contents.contains("...")?
Due to indentation the string might not be in the list of strings, but a substring of an element, which the findString-method seems to be created for in the same file.

That makes sense.

pombredanne · 2024-02-09T11:06:05Z

@MartinWitt what's left to do to get this through?
FWIW, there is a cottage industry of smalls tools and scripts that are parsing this plugin output .... it would be awesome to have a structured JSON output!
Anything to help you need move this forward?

elharo · 2024-02-09T12:32:52Z

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

+/**
+ * A dependency node visitor that serializes visited nodes to a writer using the JSON format.
+ */
+public class JsonDependencyNodeVisitor extends AbstractSerializingVisitor implements DependencyNodeVisitor {


Does this need to be public? It's easier to evolve and iterate on if it's not.

elharo · 2024-02-09T12:37:12Z

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

+        try {
+            writer.write(sb.toString());
+        } catch (IOException e) {
+            throw new RuntimeException("Error while writing json output", e);


needs a more specific exception. Looking at this now I notice that the superclass and interface are badly designed. They use a PrintWriter which swallows exceptions. That might need to be fixed.

What about just not catch an exception here and leaving this business to the callers.... fixing the PrintWriter could then be done in another patch and not here.

Fixing PrintWriter should be a different PR but that might be a prerequisite for this PR. I'm not sure. It seems fine to let this method throw IOException and handle it further up the stack. RuntimeException is not OK.

pombredanne · 2024-02-12T09:09:57Z

Looking at the details of this PR, it feels to me that crafting the JSON by hand as done here feels like problems in the making with encoding or else. Should not this use a proper JSON library of sorts? Is not there one bundled in the standard Maven or Java and this plugin may therefore always have have one on hand?

(Sorry for these likely dumb questions: my Java skills need to brushing off)

yahavi · 2024-03-01T08:57:57Z

If it's useful to anyone, we've developed a Maven dependency tree plugin that generates a JSON dependency tree. Check it out at: https://github.com/jfrog/maven-dep-tree.

monperrus · 2024-05-06T05:28:17Z

@pombredanne @elharo if we are able to drive to merge, we can put more effort on this PR.

elharo · 2024-05-06T11:09:01Z

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

+        try {
+            writer.write(sb.toString());
+        } catch (IOException e) {
+            throw new RuntimeException("Error while writing json output", e);


Fixing PrintWriter should be a different PR but that might be a prerequisite for this PR. I'm not sure. It seems fine to let this method throw IOException and handle it further up the stack. RuntimeException is not OK.

elharo · 2024-05-06T11:10:49Z

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

+     */
+    public void _testTreeJsonSerialzing() throws Exception {
+        List<String> contents = runTreeMojo("tree1.json", "json");
+        assertTrue(findString(contents, "\"testGroupId\": \"project\""));


contains is clearer

elharo · 2024-05-06T11:12:03Z

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

+    }
+
+    @Override
+    public boolean visit(DependencyNode node) {


add a test for this case, see what works

LogFlames · 2024-05-16T14:26:50Z

Hi,
I'm working on getting this merged and incorporating the requested changes. Unfortunately I don't have access to Martin's fork and will need to create a new PR. Will build on Martin's branch and try to update all changes beforehand to make the transition as smooth as possible.

elharo · 2024-05-23T16:45:03Z

feature complete under another PR

MartinWitt added 2 commits May 22, 2023 22:24

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

b9ee5ee

add testcase

b0d2421

MartinWitt changed the title ~~WIP: [MDEP-799] tree: add optional output type json~~ [MDEP-799] tree: add optional output type json May 22, 2023

replace System.lineSeparator with constant

69df906

MartinWitt commented May 29, 2023

View reviewed changes

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

elharo reviewed Jun 14, 2023

View reviewed changes

change according to feedback

e6d7d0c

elharo reviewed Feb 9, 2024

View reviewed changes

elharo requested changes May 6, 2024

View reviewed changes

LogFlames mentioned this pull request May 16, 2024

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

Merged

8 tasks

elharo closed this May 23, 2024

pombredanne mentioned this pull request May 23, 2024

Create Maven "lockfile" using built-in from maven.... It supports a dot, graphml and tgf output format. (And soon JSON!) nexB/dependency-inspector#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MartinWitt commented May 22, 2023 •

edited

MartinWitt commented May 23, 2023

MartinWitt commented May 29, 2023

MartinWitt commented Jun 14, 2023

elharo Jun 14, 2023

pombredanne Feb 12, 2024

elharo May 6, 2024

elharo Jun 14, 2023

elharo Jun 14, 2023

elharo Jun 14, 2023

elharo Jun 14, 2023

MartinWitt Jun 18, 2023

elharo May 6, 2024

LogFlames May 16, 2024

elharo May 16, 2024

pombredanne commented Feb 9, 2024 •

edited

elharo Feb 9, 2024

elharo Feb 9, 2024

pombredanne Feb 12, 2024

elharo May 6, 2024

pombredanne commented Feb 12, 2024

yahavi commented Mar 1, 2024

monperrus commented May 6, 2024

elharo May 6, 2024

elharo May 6, 2024

elharo May 6, 2024

LogFlames commented May 16, 2024

elharo commented May 23, 2024

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

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

Conversation

MartinWitt commented May 22, 2023 • edited

MartinWitt commented May 23, 2023

MartinWitt commented May 29, 2023

MartinWitt commented Jun 14, 2023

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

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

pombredanne commented Feb 9, 2024 • edited

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

pombredanne commented Feb 12, 2024

yahavi commented Mar 1, 2024

monperrus commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LogFlames commented May 16, 2024

elharo commented May 23, 2024

MartinWitt commented May 22, 2023 •

edited

pombredanne commented Feb 9, 2024 •

edited