Stop scan crashing when casting none to int #848
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #848 +/- ##
==========================================
- Coverage 62.69% 62.65% -0.05%
==========================================
Files 342 342
Lines 14491 14520 +29
==========================================
+ Hits 9085 9097 +12
- Misses 5406 5423 +17 ☔ View full report in Codecov by Sentry. |
| @pytest.mark.docker | ||
| def testRuntimeScanStop_whenUnrelatedNetworks_RemovesScanServiceWithoutCrash( | ||
| mocker, db_engine_path | ||
| ): |
There was a problem hiding this comment.
| ): | |
| ) -> None: |
![ostorlab-ai-pr-review[bot]](https://avatars.githubusercontent.com/in/1072062?s=60&v=4){kind=small}
| @@ -309,9 +309,13 @@ | |||
| networks = self._docker_client.networks.list() | |||
| for network in networks: | |||
| network_labels = network.attrs["Labels"] | |||
| if network_labels is None: | |||
| continue | |||
| universe = network_labels.get("ostorlab.universe") | |||
There was a problem hiding this comment.
Consider verifying that 'network_labels' is indeed a dictionary or mapping type to prevent unexpected attribute access errors.
| universe = network_labels.get("ostorlab.universe") | |
| if isinstance(network_labels, dict): | |
| universe = network_labels.get('ostorlab.universe') |
There was a problem hiding this comment.
@oussamaessaji also not a bad comment.
There was a problem hiding this comment.
Taken into consideration.
| if ( | ||
| network_labels is not None | ||
| and int(network_labels.get("ostorlab.universe")) == scan_id | ||
| universe is not None |
There was a problem hiding this comment.
For clarity, ensure 'universe is not None' is part of an explicit check in an 'if' statement or logical condition rather than being implicitly assumed.
| universe is not None | |
| if universe is not None: |
| network_labels is not None | ||
| and int(network_labels.get("ostorlab.universe")) == scan_id | ||
| universe is not None | ||
| and network_labels is not None |
There was a problem hiding this comment.
This condition should be part of a larger logical statement. Use an explicit 'if' with combined conditions instead to enhance readability and prevent future logic errors.
| and network_labels is not None | |
| if universe is not None and isinstance(network_labels, dict) and 'ostorlab.universe' in network_labels: |
| and int(network_labels.get("ostorlab.universe")) == scan_id | ||
| universe is not None | ||
| and network_labels is not None | ||
| and int(universe) == scan_id |
There was a problem hiding this comment.
Using 'int()' conversion can lead to exceptions if 'universe' is not a convertible type. Consider implementing a safer conversion or validation before this comparison.
| and int(universe) == scan_id | |
| if universe.isdigit() and int(universe) == scan_id: |
| @@ -309,9 +309,13 @@ | |||
| networks = self._docker_client.networks.list() | |||
| for network in networks: | |||
| network_labels = network.attrs["Labels"] | |||
| if network_labels is None: | |||
| continue | |||
There was a problem hiding this comment.
After skipping an iteration, ensure the loop continues as expected, and consider logging the reason for the skip to aid in debugging.
| continue | |
| logger.debug('Skipping network with no labels'); continue |
There was a problem hiding this comment.
Not a bad comment @oussamaessaji .
| @@ -309,9 +309,13 @@ | |||
| networks = self._docker_client.networks.list() | |||
| for network in networks: | |||
| network_labels = network.attrs["Labels"] | |||
| if network_labels is None: | |||
| continue | |||
| universe = network_labels.get("ostorlab.universe") | |||
There was a problem hiding this comment.
This line assumes that 'network_labels' structure is always the same. Consider validating the expected structure before accessing it or using a safer method to prevent potential KeyError.
| universe = network_labels.get("ostorlab.universe") | |
| universe = network_labels.get("ostorlab.universe") if isinstance(network_labels, dict) else None |
| and int(network_labels.get("ostorlab.universe")) == scan_id | ||
| universe is not None | ||
| and network_labels is not None | ||
| and int(universe) == scan_id |
There was a problem hiding this comment.
Make the condition more readable by breaking it into multiple lines or using helper variables. Explicitly check that 'universe' is a string before converting it to integer to avoid ValueError.
| and int(universe) == scan_id | |
| if universe and isinstance(universe, str) and int(universe) == scan_id: |
oussamaessaji
dismissed stale reviews from PiranhaSa and mohsinenar
via
9ef1082
Co-authored-by: Hamza AIT BEN YISSA <[email protected]>
tests/runtimes/local/runtime_test.py
Outdated
| mocker.patch.object(models, "ENGINE_URL", db_engine_path) | ||
| create_scan_db = models.Scan.create("test") | ||
|
|
||
| def docker_services(): |
tests/runtimes/local/runtime_test.py
Outdated
|
|
||
| return [services_model.Service(attrs=service) for service in services] | ||
|
|
||
| def docker_networks(): |
| network_labels is not None | ||
| and int(network_labels.get("ostorlab.universe")) == scan_id | ||
| universe is not None | ||
| and network_labels is not None |
There was a problem hiding this comment.
you dont need that, since you already have a check before
| and network_labels is not None |
…ing_none_to_int' into fix/stop_scan_crashing_when_casting_none_to_int
| mocker.patch.object(models, "ENGINE_URL", db_engine_path) | ||
| create_scan_db = models.Scan.create("test") | ||
|
|
||
| def docker_services() -> list[services_model.Service]: |
There was a problem hiding this comment.
Would this be useful as a fixture?
| @@ -309,9 +309,13 @@ | |||
| networks = self._docker_client.networks.list() | |||
| for network in networks: | |||
| network_labels = network.attrs["Labels"] | |||
| if network_labels is None: | |||
| continue | |||
There was a problem hiding this comment.
Not a bad comment @oussamaessaji .
| @@ -309,9 +309,13 @@ | |||
| networks = self._docker_client.networks.list() | |||
| for network in networks: | |||
| network_labels = network.attrs["Labels"] | |||
| if network_labels is None: | |||
| continue | |||
| universe = network_labels.get("ostorlab.universe") | |||
There was a problem hiding this comment.
@oussamaessaji also not a bad comment.
In the stop scan, we list all the networks and attempt to retrieve
ostorlab.universefrom each network, casting it to an integer. However, this approach fails because there are networks that are not related to thescan universeand do not containostorlab.universe. This pull request refactors the code to first check if a network hasostorlab.universebefore proceeding with the casting.