Skip to content

Commit

Permalink
Introduce semgrep rule to warn about use of TypeAdapter
Browse files Browse the repository at this point in the history
We had several performance regressions in the past because of the use of
TypeAdapter in often called code paths. We now add this rule to warn
developers about the use of TypeAdapter. After careful review, checking
the performance aspect, the developer can add an annotation next to the
TypeAdapter call to suppress the warning.

Besides the nature of the rule, this is also introducing the first semgrep
rule to the code base. The idea is to replace some of our custom pylint
checkers with semgrep rules in the near future. We will also integrate
semgrep with our CI in the next steps.

CMK-19526

Change-Id: Id464a5de0e06f13cd3141c5c671ea49cfae0b27b
  • Loading branch information
LarsMichelsen committed Nov 13, 2024
1 parent 48edbbe commit 086862a
Show file tree
Hide file tree
Showing 8 changed files with 30 additions and 0 deletions.
2 changes: 2 additions & 0 deletions cmk/gui/graphing/_graph_render_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ class GraphRenderOptions(GraphRenderOptionsBase, total=False):
def graph_grender_options_from_vs(options_vs: GraphRenderOptionsVS) -> GraphRenderOptions:
# no assignment expressions due to https://github.com/pylint-dev/pylint/issues/8486
title_format_vs = options_vs.get("title_format")
# Performance impact needs to be investigated (see CMK-19527)
# nosemgrep: type-adapter-detected
return TypeAdapter(GraphRenderOptions).validate_python(
options_vs
| (
Expand Down
2 changes: 2 additions & 0 deletions cmk/gui/userdb/_user_spec.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,7 @@ def add_internal_attributes(usr: UserSpec) -> int:


def validate_users_details(users: object) -> dict[str, UserDetails]:
# Performance impact needs to be investigated (see CMK-19527)
# nosemgrep: type-adapter-detected
validator = TypeAdapter(dict[str, UserDetails])
return validator.validate_python(users, strict=True)
2 changes: 2 additions & 0 deletions cmk/gui/watolib/simple_config_file.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ def __init__(
spec_class: TypeAlias,
) -> None:
self._config_file_path = config_file_path
# Performance impact needs to be investigated (see CMK-19527)
# nosemgrep: type-adapter-detected
self.validator = TypeAdapter(spec_class)

def validate(self, raw: object) -> _G:
Expand Down
2 changes: 2 additions & 0 deletions cmk/plugins/kube/performance.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ class Samples:


def parse_performance_metrics(cluster_collector_metrics: bytes) -> Sequence[_AllSamples]:
# Performance impact needs to be investigated (see CMK-19527)
# nosemgrep: type-adapter-detected
adapter = TypeAdapter(list[_AllSamples])
return adapter.validate_json(cluster_collector_metrics)

Expand Down
2 changes: 2 additions & 0 deletions cmk/plugins/kube/prometheus_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ def parse_raw_response(
response: bytes | str,
) -> Response | ValidationError | JSONDecodeError:
try:
# Performance impact needs to be investigated (see CMK-19527)
# nosemgrep: type-adapter-detected
adapter: TypeAdapter[Response] = TypeAdapter(Response)
return adapter.validate_json(response)
except (ValidationError, JSONDecodeError) as e:
Expand Down
2 changes: 2 additions & 0 deletions cmk/plugins/kube/special_agents/agent_kube.py
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,8 @@ class ClusterConnectionError(Exception):


def _parse_raw_metrics(content: bytes) -> list[RawMetrics]:
# Performance impact needs to be investigated (see CMK-19527)
# nosemgrep: type-adapter-detected
adapter = TypeAdapter(list[RawMetrics])
return adapter.validate_json(content)

Expand Down
2 changes: 2 additions & 0 deletions cmk/utils/werks/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ def load(base_dir: Path | None = None) -> dict[int, Werk]:

def load_precompiled_werks_file(path: Path) -> dict[int, Werk]:
# ? what is the content of these files, to which the path shows
# There is no performance issue with this TypeAdapter call
# nosemgrep: type-adapter-detected
adapter = TypeAdapter(dict[int, Werk | WerkV1])
with path.open("r", encoding="utf-8") as f:

Expand Down
16 changes: 16 additions & 0 deletions tests/semgrep/rules/pydantic_type_adapter.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
---
rules:
- id: type-adapter-detected
patterns:
- pattern: |
TypeAdapter(...)
- pattern-inside: |
def $FUNC(...): ...
message: >-
Detected the use of TypeAdapter(). Executing TypeAdapter() is a costly operation which can
lead to a major performance issue if used wrong. Its use may be acceptable when invoked
only once, e.g. during program startup. Ensure it does not impact performance of your
program negatively. Add "# nosemgrep: type-adapter-detected" to acknowledge this
warning.
languages: [python]
severity: WARNING

0 comments on commit 086862a

Please sign in to comment.