Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: type hint for get_component_by_purl is incorrect #310

Merged
merged 2 commits into from
Nov 28, 2022

Conversation

gruebel
Copy link
Contributor

@gruebel gruebel commented Oct 8, 2022

Signed-off-by: gruebel anton.gruebel@gmail.com

  • adjusted the type hint for get_component_by_purl and added a test for it 馃檪
  • changed the list/filter expression to use a list comprehension, which is almost 2x faster in this case 馃殌

Signed-off-by: gruebel <anton.gruebel@gmail.com>
@gruebel gruebel requested a review from a team as a code owner October 8, 2022 09:02
Copy link
Member

@jkowalleck jkowalleck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this PR needs to be discussed - see #310 (comment)


Thanks for the bug report, @gruebel, but to me it appears that
you bring up breaking (arch/design - not code) changes that alter the original method(API) in its meaning.

Here is a fix that does not introduce breaking changes, but actually fixes a bug

diff --git a/cyclonedx/model/bom.py b/cyclonedx/model/bom.py
--- a/cyclonedx/model/bom.py	(revision 6c0c1742d2ea19dfc0284785cf9597b43ef05979)
+++ b/cyclonedx/model/bom.py	(date 1665226315792)
@@ -300,7 +300,7 @@
             `Component` or `None`
         """
         if purl:
-            found = list(filter(lambda x: x.purl == purl, self.components))
+            found = list(filter(lambda x: str(x.purl) == purl, self.components))
             if len(found) == 1:
                 return found[0]

PS: turned out that the actual bugfix is not inteded, but a documentation fix - see #310 (comment)

tests/test_component.py Outdated Show resolved Hide resolved
tests/test_component.py Outdated Show resolved Hide resolved
cyclonedx/model/bom.py Show resolved Hide resolved
cyclonedx/model/bom.py Show resolved Hide resolved
@jkowalleck
Copy link
Member

jkowalleck commented Oct 8, 2022

@madpah we need to triage this one.
was the typing and the method description wrong, or did we forget the string cast?

DO we have downstream users we can ask how they use this functionality right now?

How do you want to have this method implemented in the upcoming #262 ?

@gruebel
Copy link
Contributor Author

gruebel commented Oct 8, 2022

yeap, we are using it in checkov
https://github.com/bridgecrewio/checkov/blob/2fce14ee65f4e6143c2a510e3219b9957ee5a50f/checkov/common/output/cyclonedx.py#L106-L110
and actually your suggested change would break our implementation. So, I think I did the correct thing by adjusting the type hint instead of changing the code to be aligned with the current type hint.

@jkowalleck
Copy link
Member

yeap, we are using it in checkov https://github.com/bridgecrewio/checkov/blob/2fce14ee65f4e6143c2a510e3219b9957ee5a50f/checkov/common/output/cyclonedx.py#L106-L110 and actually your suggested change would break our implementation. So, I think I did the correct thing by adjusting the type hint instead of changing the code to be aligned with the current type hint.

In python the typing is optional and has no meaning for the runtime.
Fixing the typing and docs, while leaving the implementation untouched - this should be the least code-breaking change (downstream).

Even though I would argue that it is a breaking change from architectural/design perspective.
But in the end the usage counts for python.

@jkowalleck jkowalleck self-requested a review October 8, 2022 12:03
Signed-off-by: gruebel <anton.gruebel@gmail.com>
@gruebel
Copy link
Contributor Author

gruebel commented Oct 8, 2022

In python the typing is optional and has no meaning for the runtime. Fixing the typing and docs, while leaving the implementation untouched - this should be the least code-breaking change (downstream).

Even though I would argue that it is a breaking change from architectural/design perspective. But in the end the usage counts for python.

that's correct. if it was differently intended, then you should probably put it into v4 release branch 馃檪

@gruebel
Copy link
Contributor Author

gruebel commented Nov 25, 2022

hey @jkowalleck any plan on merging my change 馃檪

@jkowalleck
Copy link
Member

re: #310 (comment)
@madpah asked for a dev-stop until #262 was done. it seams like this takes longer than expected.

we will see how to get this very PR integrated.

@madpah madpah self-assigned this Nov 28, 2022
@madpah madpah added the bug Something isn't working label Nov 28, 2022
@madpah
Copy link
Collaborator

madpah commented Nov 28, 2022

I'm happy to merge this - from my checks, get_component_by_purl doesn't appear to be used internally by this library, or the cyclonedx-python app - and this is clearly a typing error.

@madpah madpah merged commit 06037b9 into CycloneDX:main Nov 28, 2022
@madpah
Copy link
Collaborator

madpah commented Nov 28, 2022

Thanks @gruebel and @jkowalleck !

@gruebel gruebel deleted the fix-method-type-hint branch December 5, 2022 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants