fix(nuget): add nuget dependency extract #3121

hellhound-abu · 2022-10-06T06:57:26Z

Fixes #0000

Tasks

Reviewed contribution guidelines
PR is descriptively titled 📑 and links the original issue above 🔗
Tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR
Run tests locally to check for errors.
Commits are in uniquely-named feature branch and has no merge conflicts 📁

AyanSinhaMahapatra · 2022-10-10T09:43:54Z

Thanks! Could you fix the test failures at tests/packagedcode/test_nuget.py with:

pytest -vvs tests/packagedcode/test_nuget.py and then
SCANCODE_REGEN_TEST_FIXTURES=yes pytest -vvs tests/packagedcode/test_nuget.py --lf

Also please add your signoff. 🙇

pombredanne · 2022-10-28T15:37:57Z

@hellhound-abu gentle ping.. we would like to merge this... but we cannot unless you update your PR at least with a DCO signoff.

pombredanne · 2022-10-28T15:41:48Z

@6mile ping you seem to be from the same group, may be you can help there?

6mile · 2022-10-28T20:46:31Z

@6mile ping you seem to be from the same group, may be you can help there?

I've let him know. Cheers.

jaitaiwan · 2022-10-29T00:57:46Z

Hey folks, Dan from SecureStack here. Abu is on holidays so won’t be able to contribute till 2 weeks from now. I can fix the failing tests if that’s helpful but that won’t help get a DCO from him.

Do you have a way around this or content to wait?

pombredanne · 2022-10-29T08:55:33Z

@jaitaiwan Thank you ++ for your reply!
No rush, we can wait alright!
We are just making sure we do not have too many PRs that could go stale from lack of TLC especially like this one which is valuable addition.

Whatever you guys are doing on SecureStack looks great BTW! 👍

pombredanne · 2022-11-14T16:58:34Z

@hellhound-abu gentle ping ... you may be back now?

Signed-off-by: Abedin Poonawala <[email protected]>

1. update code to look for dependencies in a group as well 2. update nuget test data with dependency data Signed-off-by: Abedin Poonawala <[email protected]>

Signed-off-by: Abedin Poonawala <[email protected]>

hellhound-abu · 2022-11-22T03:51:18Z

Updated the branch with necessary changes. If we can merge it whenever possible.

Thanks for waiting.

pombredanne

@hellhound-abu Thank you ++ for the updates! I posted a few review comments inline for your consideration. There is also one failing test.

pombredanne · 2022-11-22T09:28:47Z

src/packagedcode/nuget.py

 """
 Handle NuGet packages and their manifests.
 """
-# TODO: add dependencies
+
+# TODO: Add section to handle dependencies of dependencies


For this you may want to create a separate issue... TODO comments (that I tend to abuse) eventually get lost.
Note also that for a full, dynamic dependency resolution of nugets. there is this new project at https://github.com/nexB/nuget-inspector

I say the same thing about TODO's internally 😂 That nuget-inspector proejct looks promising.

pombredanne · 2022-11-22T09:29:50Z

src/packagedcode/nuget.py

+                            type='nuget',
+                            namespace=None,
+                            name=dependency.get("@id"),
+                            version=dependency.get("@version"),


You can only put a version here if this is not a range so IMHO you would have to test for this.

pombredanne · 2022-11-22T09:30:20Z

src/packagedcode/nuget.py

+                            dep_pack = models.DependentPackage(
+                            purl=str(dpurl),
+                            extracted_requirement=dependency.get("@version"),
+                            scope="dependency",


Would not each dependency group become a scope here?

pombredanne · 2022-11-22T09:31:18Z

src/packagedcode/nuget.py

+                            scope="dependency",
+                            is_runtime=False,
+                            is_optional=False,
+                            is_resolved=True,


A dependency is resolved if an only if we have a concrete version and not a range. Here the version is optional AFAIK and could be a range?

pombredanne · 2022-11-22T09:34:18Z

src/packagedcode/nuget.py

+
+                            dependencies.append(dep_pack)
+
+            if "dependency" in nuspec.get("dependencies"):


I would prefer something like this, and then reuse the nuget_dependency var below:

Suggested change

if "dependency" in nuspec.get("dependencies"):

nuget_dependencies = nuspec.get("dependencies") or {}

nuget_dependency = nuget_dependencies.get("dependency")

if nuget_dependency:

Out of curiosity, any reason for or {} over get("...", {})?

@jaitaiwan re:

Out of curiosity, any reason for or {} over get("...", {})?

This is an idiom I like to use to make sure that I am getting a proper empty dict in all cases even if the get returns None or some emptyish value.
Getting an empty dict when you expect a dict enable chaining the next call without requiring an extra check for empties.

>>> repr({1: None, 2: "", 3: {"bar": "baz"}}.get(1, {})) 'None' >>> repr({1: None, 2: "", 3: {"bar": "baz"}}.get(1, {}) or {}) '{}' >>> repr({1: None, 2: "", 3: {"bar": "baz"}}.get(1) or {}) '{}'

Fair call. thanks!

pombredanne · 2022-11-22T09:35:02Z

src/packagedcode/nuget.py

+                    dep_pack = models.DependentPackage(
+                        purl=str(dpurl),
+                        extracted_requirement=nuspec.get("dependencies").get(
+                            "dependency").get("@version"),


Factor version here and above in a variable.

pombredanne · 2022-11-22T09:35:35Z

src/packagedcode/nuget.py

+                        scope="dependency",
+                        is_runtime=False,
+                        is_optional=False,
+                        is_resolved=True,


Same comment as above... resolve is True IFF we have a concrete pinned, non-empty version

pombredanne · 2022-11-22T10:25:06Z

src/packagedcode/nuget.py

+        return dependencies
+
+    except Exception as e:
+        print(e)


Please do not print exceptions. In general your should avoid to wrap you code in a try/except altogether as this can never be debugged this way. Let the exception bubble up and we will catch it where needed, and worse case at the top level.

pombredanne · 2022-11-22T10:25:47Z

src/packagedcode/nuget.py

@@ -62,14 +144,15 @@ def parse(cls, location):

        # Summary: A short description of the package for UI display. If omitted, a
        # truncated version of description is used.
-        description = build_description(nuspec.get('summary') , nuspec.get('description'))
+        description = build_description(


Why wrapping this on tow lines? I am not sure this helps with readability.

pombredanne · 2022-11-22T10:27:11Z

tests/packagedcode/data/nuget/bootstrap.nuspec.json.expected

-    "datasource_id": "nuget_nupsec",
-    "purl": "pkg:nuget/[email protected]"
-  }
+          "type": "nuget",


Please avoid changing the spacing if possible as this makes reviewing the diff difficult

pombredanne · 2023-01-04T09:58:57Z

@hellhound-abu Happy new year! gentle ping

pombredanne · 2023-01-09T15:46:01Z

@AyanSinhaMahapatra since there is no reply for now, let's move this to the next milestone

AyanSinhaMahapatra · 2023-01-09T15:48:35Z

@pombredanne moved this to v32.1

pombredanne · 2023-01-18T00:14:33Z

I have applied the fixes in #3206 on top of your commits. So I am losing this and will merge this other branch!
Thanks!

hellhound-abu force-pushed the add-nuget-dependencies branch from dfb4de1 to 6049c36 Compare November 18, 2022 03:32

bloodhoundGit added 2 commits November 18, 2022 15:46

fix(nuget): add nuget dependency extract

a05670b

Signed-off-by: Abedin Poonawala <[email protected]>

feat(nuget): add dependency check for inside group tags

a228078

1. update code to look for dependencies in a group as well 2. update nuget test data with dependency data Signed-off-by: Abedin Poonawala <[email protected]>

hellhound-abu force-pushed the add-nuget-dependencies branch from 6049c36 to a228078 Compare November 18, 2022 05:46

bloodhoundGit added 3 commits November 18, 2022 16:10

fix(nuget): match updated develop code

f370581

Signed-off-by: Abedin Poonawala <[email protected]>

fix(nuget): update nuget test

fc2d36a

Signed-off-by: Abedin Poonawala <[email protected]>

fix(nuget): add dependency section in bootstrap test

60a14ec

Signed-off-by: Abedin Poonawala <[email protected]>

hellhound-abu force-pushed the add-nuget-dependencies branch from c8708e9 to 60a14ec Compare November 22, 2022 01:00

pombredanne requested changes Nov 22, 2022

View reviewed changes

This was referenced Jan 4, 2023

Add dependencies to NuGet #648

Open

Dependencies not collected from NuGet #2308

Open

pombredanne added this to the v32.0 milestone Jan 6, 2023

pombredanne requested review from jaitaiwan and AyanSinhaMahapatra and removed request for jaitaiwan January 9, 2023 15:45

AyanSinhaMahapatra modified the milestones: v32.0, v32.1 Jan 9, 2023

pombredanne mentioned this pull request Jan 18, 2023

Add nuget nuspec dependencies #3206

Merged

4 tasks

pombredanne closed this Jan 18, 2023

pombredanne modified the milestones: v32.1, v32.0 Jan 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(nuget): add nuget dependency extract #3121

fix(nuget): add nuget dependency extract #3121

hellhound-abu commented Oct 6, 2022 •

edited

Loading

AyanSinhaMahapatra commented Oct 10, 2022

pombredanne commented Oct 28, 2022

pombredanne commented Oct 28, 2022

6mile commented Oct 28, 2022

jaitaiwan commented Oct 29, 2022

pombredanne commented Oct 29, 2022

pombredanne commented Nov 14, 2022

hellhound-abu commented Nov 22, 2022

pombredanne left a comment

pombredanne Nov 22, 2022

jaitaiwan Nov 22, 2022

pombredanne Nov 22, 2022

pombredanne Nov 22, 2022

pombredanne Nov 22, 2022

pombredanne Nov 22, 2022

jaitaiwan Nov 22, 2022

pombredanne Nov 22, 2022

jaitaiwan Nov 22, 2022

pombredanne Nov 22, 2022

pombredanne Nov 22, 2022

pombredanne Nov 22, 2022 •

edited

Loading

pombredanne Nov 22, 2022

pombredanne Nov 22, 2022

pombredanne commented Jan 4, 2023

pombredanne commented Jan 9, 2023

AyanSinhaMahapatra commented Jan 9, 2023

pombredanne commented Jan 18, 2023 •

edited

Loading


		dependencies.append(dep_pack)

		if "dependency" in nuspec.get("dependencies"):

-            if "dependency" in nuspec.get("dependencies"):
+            nuget_dependencies = nuspec.get("dependencies") or {}
+            nuget_dependency = nuget_dependencies.get("dependency")
+            if nuget_dependency:

fix(nuget): add nuget dependency extract #3121

fix(nuget): add nuget dependency extract #3121

Conversation

hellhound-abu commented Oct 6, 2022 • edited Loading

Tasks

AyanSinhaMahapatra commented Oct 10, 2022

pombredanne commented Oct 28, 2022

pombredanne commented Oct 28, 2022

6mile commented Oct 28, 2022

jaitaiwan commented Oct 29, 2022

pombredanne commented Oct 29, 2022

pombredanne commented Nov 14, 2022

hellhound-abu commented Nov 22, 2022

pombredanne left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pombredanne Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pombredanne commented Jan 4, 2023

pombredanne commented Jan 9, 2023

AyanSinhaMahapatra commented Jan 9, 2023

pombredanne commented Jan 18, 2023 • edited Loading

hellhound-abu commented Oct 6, 2022 •

edited

Loading

pombredanne Nov 22, 2022 •

edited

Loading

pombredanne commented Jan 18, 2023 •

edited

Loading