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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor to simplify scanresults model #214

Merged
merged 5 commits into from
Feb 5, 2025

Conversation

abraemer
Copy link
Contributor

@abraemer abraemer commented Feb 3, 2025

Summary of changes

  • slightly improve OpossumProvider to allow construction of Opossum where review_results=None
  • fix bug in ScanResults were the logic around the attribution keys assumed attribution_to_id to be a default_dict and would break otherwise.
  • change fields of ScanResults that were either list | None or dict | None to disallow None and use an empty container instead. This removes some null checks throughout the codebase and requires some adaption of the tests. Mainly to the round-trip of OpossumFileModel due to the normalization of the fields.

Context and reason for change

This PR aims to simplify the internal OpossumModel by making the types on the fields stricter. This makes the subsequent implementation of merging of opossums simpler.

@abraemer abraemer force-pushed the refactor-simplify-scanresults branch 2 times, most recently from a998764 to 6248152 Compare February 3, 2025 15:21
* disallow `None` for fields that are containers. Use empty container instead.
* Change converter from OpossumFileModel accordingly
* Equality of OpossumFileModel in tests needs to respect that None might change to [] or {}
Copy link
Contributor

@Hellgartner Hellgartner left a comment

Choose a reason for hiding this comment

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

Nice PR showcasing how good it is to decouple the working entities from the models directly associated to the file

The only one thing I would like to discuss is whether we should keep the change transparent for the end file, i.e. restore the Nones when converting back to the OpossumModel?
That would also mean we would avoid changing the tests

I did double check OpossumUI and from loading the data into OpossumUI there is no difference whether the entry is not part of the json at all or whether it is simply and empty dict/list.

However IMHO I would prefer keeping the .opossum files un-mutetated while processing.

@abraemer
Copy link
Contributor Author

abraemer commented Feb 5, 2025

The only one thing I would like to discuss is whether we should keep the change transparent for the end file, i.e. restore the Nones when converting back to the OpossumModel?

However IMHO I would prefer keeping the .opossum files un-mutetated while processing.

I think the main issue here is that we cannot distinguish whether the input was missing the field or had an empty container. Thus "keeping ... unmutated" is impossible in principle. We need to make an a priori arbitrary choice whether we want to write [] or skip the field.
Personally I like that the structure we write always has all the keys. That makes it easy for someone to look at and see everything that could be there without experimentation.

* rename ScanResults.get_attribution_key to clarify that it is private and can modify the internal state
* move _assert_equal_or_both_falsy into the only file that uses it
* rename tests for internal opossum model and make generation of attribution_to_id explicit
@Hellgartner
Copy link
Contributor

Hellgartner commented Feb 5, 2025

Thus "keeping ... unmutated" is impossible in principle.

Agreed, however all files we have seen up to now adhered to the principle of just leaving empty fields out. So this is a break in procedure.

@abraemer abraemer requested a review from Hellgartner February 5, 2025 14:31
Copy link
Contributor

@Hellgartner Hellgartner left a comment

Choose a reason for hiding this comment

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

Discussed non vs empty list and agreed to keep empty list

@Hellgartner Hellgartner merged commit dca7a40 into main Feb 5, 2025
10 checks passed
@Hellgartner Hellgartner deleted the refactor-simplify-scanresults branch February 5, 2025 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants