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 build_cpe.py #9834

Merged
merged 10 commits into from
Nov 23, 2022
8 changes: 8 additions & 0 deletions ssg/boolean_expression.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,14 @@ def arg(self):
def name(self):
return self.requirement.project_name

@staticmethod
def cpe_id_is_parametrized(cpe_id):
return re.search(r'^\w+\[\w+\]$', cpe_id)

@staticmethod
def is_cpe_name(cpe_id_or_name):
return cpe_id_or_name.startswith("cpe:")


class Algebra(boolean.BooleanAlgebra):
"""
Expand Down
22 changes: 8 additions & 14 deletions ssg/build_cpe.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
from __future__ import print_function
import os
import sys
import re

from .constants import oval_namespace
from .constants import PREFIX_TO_NS
Expand Down Expand Up @@ -81,22 +80,17 @@ def add_cpe_item(self, cpe_item):
if cpe_item.is_product_cpe:
self.product_cpes[cpe_item.id_] = cpe_item

def _is_name(self, ref):
return ref.startswith("cpe:")

def _is_parametrized(self, ref):
return re.search(r'^\w+\[\w+\]$', ref)
Copy link
Collaborator

Choose a reason for hiding this comment

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

By applying this change, the re module is no longer needed and does not need to be imported.


def get_cpe(self, ref):
def get_cpe(self, cpe_id_or_name):
try:
if self._is_name(ref):
return self.cpes_by_name[ref]
if Symbol.is_cpe_name(cpe_id_or_name):
return self.cpes_by_name[cpe_id_or_name]
else:
if self._is_parametrized(ref):
ref = get_base_name_of_parametrized_platform(ref)
return self.cpes_by_id[ref]
if Symbol.cpe_id_is_parametrized(cpe_id_or_name):
cpe_id_or_name = get_base_name_of_parametrized_platform(
cpe_id_or_name)
return self.cpes_by_id[cpe_id_or_name]
except KeyError:
raise CPEDoesNotExist("CPE %s is not defined" % (ref))
raise CPEDoesNotExist("CPE %s is not defined" % cpe_id_or_name)

def get_cpe_name(self, cpe_id):
cpe = self.get_cpe(cpe_id)
Expand Down
18 changes: 12 additions & 6 deletions ssg/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,16 @@ def enum(*args):
return type('Enum', (), enums)


def recurse_or_substitute_or_do_nothing(
v, string_dict, ignored_keys=frozenset()):
if isinstance(v, dict):
return apply_formatting_on_dict_values(v, string_dict, ignored_keys)
elif isinstance(v, str):
return v.format(**string_dict)
else:
return v

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@evgenyz Where the frozenset should be after the refactoring to make it work as expected? Should it be in recurse_or_substitute_or_do_nothing or in the apply_formatting_on_dict_values?

Copy link
Member

Choose a reason for hiding this comment

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

Preferably in both places, so the default argument to ignored_keys is an empty iterable that is not mutable. That way, you even won't have to set ignored_keys to that empty list.
However, the decomposition into those two functions in this form was not such great idea, because the recurse_or_substitute_or_do_nothing accepts ignored_keys that it doesn't care about - it just passes those on, and it also hardcodes "recursion" to calling apply_formatting_on_dict_values, which creates a circular dependency. So I would say that having seen this, I would either revert to the original code, or if you became sold to the idea of decomposing handling of container and handling of items, I would pass recurse_or_substitute_or_do_nothing a callback, e.g. lambda value, substitutions: recurse_or_substitute_or_do_nothing(value, substitutions, ignored_keys) that would be used in the item-processing function if the item passed is a dictionary.
So sorry, suggesting the decomposition like this was a mistake.

Copy link
Member

Choose a reason for hiding this comment

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

I'd say every place where ignored_keys is an optional argument, so — both.


def apply_formatting_on_dict_values(source_dict, string_dict, ignored_keys=frozenset()):
"""
Uses Python built-in string replacement.
Expand All @@ -343,12 +353,8 @@ def apply_formatting_on_dict_values(source_dict, string_dict, ignored_keys=froze
new_dict = {}
for k, v in source_dict.items():
if k not in ignored_keys:
if isinstance(v, dict):
new_dict[k] = apply_formatting_on_dict_values(v, string_dict, ignored_keys)
elif isinstance(v, str):
new_dict[k] = v.format(**string_dict)
else:
new_dict[k] = v
new_dict[k] = recurse_or_substitute_or_do_nothing(
v, string_dict, ignored_keys)
else:
new_dict[k] = v
return new_dict