GeezerVison is showing github.com — Fix diff-scan poll so omit_license_details is honored · SocketDev/socket-python-cli@2dba87d
File treeExpand file treeCollapse file tree Open diff view settings Expand file treeCollapse file tree Open diff view settings Collapse file Copy file name to clipboardExpand all lines: socketsecurity/core/__init__.py +30-4Lines changed: 30 additions & 4 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -1337,6 +1337,11 @@ def get_diff_scan_artifacts( | 1337 | 1337 | the backend computes, so the comparison survives network idle timeouts | 1338 | 1338 | (CE-354). See the DIFF_SCAN_POLL_* constants for the polling policy. | 1339 | 1339 |
| | 1340 | + When ``include_license_details`` is False (the default), a final fetch | | 1341 | + without ``cached`` requests ``omit_license_details=true``. The API | | 1342 | + ignores that flag on cached responses, so the lean payload has to come | | 1343 | + from a separate non-cached get (CE-224). | | 1344 | +
| 1340 | 1345 | Requires an org token with the ``diff-scans:create``, ``diff-scans:list`` | 1341 | 1346 | and ``full-scans:list`` scopes; callers are expected to catch failures and | 1342 | 1347 | fall back to the legacy streaming comparison. | @@ -1368,10 +1373,16 @@ def get_diff_scan_artifacts( | 1368 | 1373 | # which case the create response already carries the artifacts. | 1369 | 1374 | artifacts_dict = diff_scan.get("artifacts") | 1370 | 1375 |
| 1371 | | - poll_params = { | 1372 | | - "cached": "true", | 1373 | | - "omit_license_details": "false" if include_license_details else "true", | 1374 | | - } | | 1376 | + # Poll with cached=true for short bounded 202/200 responses (CE-354). | | 1377 | + # The API ignores omit_license_details whenever cached=true — cached | | 1378 | + # payloads always embed full license data — so readiness polling never | | 1379 | + # requests it. When license details should be omitted (the default; | | 1380 | + # CE-224), the lean payload is fetched separately below without cached. | | 1381 | + poll_params = {"cached": "true"} | | 1382 | + if not include_license_details: | | 1383 | + # Keep the readiness response small so the ignored omit doesn't | | 1384 | + # reintroduce large-response truncation while we wait for 200. | | 1385 | + poll_params["omit_unchanged"] = "true" | 1375 | 1386 | deadline = time.monotonic() + DIFF_SCAN_POLL_TIMEOUT_SECONDS | 1376 | 1387 | interval = DIFF_SCAN_POLL_INITIAL_INTERVAL_SECONDS | 1377 | 1388 | while artifacts_dict is None: | @@ -1404,6 +1415,21 @@ def get_diff_scan_artifacts( | 1404 | 1415 | time.sleep(interval) | 1405 | 1416 | interval = min(interval * DIFF_SCAN_POLL_BACKOFF_MULTIPLIER, DIFF_SCAN_POLL_MAX_INTERVAL_SECONDS) | 1406 | 1417 |
| | 1418 | + # Cached results always include license details. Re-fetch once without | | 1419 | + # cached so omit_license_details is honored and the diff stays lean. | | 1420 | + if not include_license_details: | | 1421 | + response = self.sdk.diffscans.get( | | 1422 | + self.config.org_slug, | | 1423 | + diff_scan_id, | | 1424 | + params={"omit_license_details": "true"}, | | 1425 | + ) | | 1426 | + scan = response.get("diff_scan") or {} | | 1427 | + if scan.get("artifacts") is None: | | 1428 | + raise Exception( | | 1429 | + f"Error fetching diff scan {diff_scan_id}: unexpected response: {str(response)[:500]}" | | 1430 | + ) | | 1431 | + artifacts_dict = scan["artifacts"] | | 1432 | + | 1407 | 1433 | return DiffArtifacts.from_dict({ | 1408 | 1434 | key: artifacts_dict.get(key) or [] | 1409 | 1435 | for key in ("added", "removed", "unchanged", "replaced", "updated") | |
Collapse file Copy file name to clipboardExpand all lines: tests/core/test_diff_scan_polling.py +44-7Lines changed: 44 additions & 7 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -26,11 +26,14 @@ def no_sleep(mocker): | 26 | 26 | def test_polls_until_diff_scan_ready(core, diff_scan_get_response, no_sleep): | 27 | 27 | """202 processing responses are polled through until the 200 result arrives.""" | 28 | 28 | processing = {"status": "processing", "id": "diff-scan-123"} | 29 | | - core.sdk.diffscans.get.side_effect = [processing, processing, diff_scan_get_response] | | 29 | + # Final cached poll + lean omit_license_details re-fetch. | | 30 | + core.sdk.diffscans.get.side_effect = [ | | 31 | + processing, processing, diff_scan_get_response, diff_scan_get_response | | 32 | + ] | 30 | 33 |
| 31 | 34 | artifacts = core.get_diff_scan_artifacts("head", "new") | 32 | 35 |
| 33 | | - assert core.sdk.diffscans.get.call_count == 3 | | 36 | + assert core.sdk.diffscans.get.call_count == 4 | 34 | 37 | assert no_sleep.call_count == 2 # slept between polls, never during them | 35 | 38 | assert len(artifacts.added) > 0 | 36 | 39 |
| @@ -40,7 +43,9 @@ def test_poll_interval_backs_off(core, diff_scan_get_response, no_sleep, monkeyp | 40 | 43 | monkeypatch.setattr(core_module, "DIFF_SCAN_POLL_INITIAL_INTERVAL_SECONDS", 4.0) | 41 | 44 | monkeypatch.setattr(core_module, "DIFF_SCAN_POLL_MAX_INTERVAL_SECONDS", 10.0) | 42 | 45 | processing = {"status": "processing", "id": "diff-scan-123"} | 43 | | - core.sdk.diffscans.get.side_effect = [processing] * 4 + [diff_scan_get_response] | | 46 | + core.sdk.diffscans.get.side_effect = ( | | 47 | + [processing] * 4 + [diff_scan_get_response, diff_scan_get_response] | | 48 | + ) | 44 | 49 |
| 45 | 50 | core.get_diff_scan_artifacts("head", "new") | 46 | 51 |
| @@ -50,11 +55,13 @@ def test_poll_interval_backs_off(core, diff_scan_get_response, no_sleep, monkeyp | 50 | 55 |
| 51 | 56 | def test_transient_poll_error_is_retried(core, diff_scan_get_response, no_sleep): | 52 | 57 | """A dropped poll doesn't abandon the flow - the diff keeps computing server-side.""" | 53 | | - core.sdk.diffscans.get.side_effect = [APIConnectionError("reset"), diff_scan_get_response] | | 58 | + core.sdk.diffscans.get.side_effect = [ | | 59 | + APIConnectionError("reset"), diff_scan_get_response, diff_scan_get_response | | 60 | + ] | 54 | 61 |
| 55 | 62 | artifacts = core.get_diff_scan_artifacts("head", "new") | 56 | 63 |
| 57 | | - assert core.sdk.diffscans.get.call_count == 2 | | 64 | + assert core.sdk.diffscans.get.call_count == 3 | 58 | 65 | assert len(artifacts.added) > 0 | 59 | 66 |
| 60 | 67 |
| @@ -76,15 +83,45 @@ def test_poll_timeout_raises(core, no_sleep, monkeypatch): | 76 | 83 |
| 77 | 84 |
| 78 | 85 | def test_duplicate_redirect_uses_embedded_artifacts(core, diff_scan_get_response): | 79 | | - """An on_duplicate redirect can return the computed diff scan straight away.""" | | 86 | + """An on_duplicate redirect skips readiness polling; lean re-fetch still runs.""" | 80 | 87 | core.sdk.diffscans.create_from_ids.return_value = diff_scan_get_response | 81 | 88 |
| 82 | 89 | artifacts = core.get_diff_scan_artifacts("head", "new") | 83 | 90 |
| 84 | | - core.sdk.diffscans.get.assert_not_called() | | 91 | + # Create already carried artifacts, so cached readiness polling is skipped, | | 92 | + # but omit_license_details still needs a non-cached get (cached ignores it). | | 93 | + core.sdk.diffscans.get.assert_called_once_with( | | 94 | + core.config.org_slug, | | 95 | + "diff-scan-123", | | 96 | + params={"omit_license_details": "true"}, | | 97 | + ) | 85 | 98 | assert len(artifacts.added) > 0 | 86 | 99 |
| 87 | 100 |
| | 101 | +def test_lean_refetch_omits_license_details_without_cached( | | 102 | + core, diff_scan_get_response, no_sleep | | 103 | +): | | 104 | + """omit_license_details is fetched without cached=true (API ignores it otherwise).""" | | 105 | + processing = {"status": "processing", "id": "diff-scan-123"} | | 106 | + core.sdk.diffscans.get.side_effect = [ | | 107 | + processing, diff_scan_get_response, diff_scan_get_response | | 108 | + ] | | 109 | + | | 110 | + core.get_diff_scan_artifacts("head", "new") | | 111 | + | | 112 | + assert core.sdk.diffscans.get.call_args_list[0].kwargs["params"] == { | | 113 | + "cached": "true", | | 114 | + "omit_unchanged": "true", | | 115 | + } | | 116 | + assert core.sdk.diffscans.get.call_args_list[1].kwargs["params"] == { | | 117 | + "cached": "true", | | 118 | + "omit_unchanged": "true", | | 119 | + } | | 120 | + assert core.sdk.diffscans.get.call_args_list[2].kwargs["params"] == { | | 121 | + "omit_license_details": "true", | | 122 | + } | | 123 | + | | 124 | + | 88 | 125 | def test_fallback_to_streaming_diff_on_failure(core): | 89 | 126 | """If the diff-scans flow fails (e.g. token missing the diff-scans scopes), | 90 | 127 | the comparison falls back to the legacy streaming diff transparently.""" | |
Collapse file Copy file name to clipboardExpand all lines: tests/core/test_sdk_methods.py +12-7Lines changed: 12 additions & 7 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -239,12 +239,15 @@ def test_get_added_and_removed_packages(core): | 239 | 239 | # include_license_details defaults to False: the diff path never consumes | 240 | 240 | # embedded license data (license artifacts come from the PURL endpoint), so | 241 | 241 | # requesting it only bloats the response and risks the truncation | 242 | | - # crash on large repos. | 243 | | - core.sdk.diffscans.get.assert_called_once_with( | 244 | | - core.config.org_slug, | 245 | | - "diff-scan-123", | 246 | | - params={"cached": "true", "omit_license_details": "true"}, | 247 | | - ) | | 242 | + # crash on large repos. cached=true ignores omit_license_details, so the | | 243 | + # poll checks readiness (optionally omitting unchanged to stay small) and | | 244 | + # a separate non-cached get fetches the lean payload. | | 245 | + get_calls = core.sdk.diffscans.get.call_args_list | | 246 | + assert len(get_calls) == 2 | | 247 | + assert get_calls[0].args == (core.config.org_slug, "diff-scan-123") | | 248 | + assert get_calls[0].kwargs["params"] == {"cached": "true", "omit_unchanged": "true"} | | 249 | + assert get_calls[1].args == (core.config.org_slug, "diff-scan-123") | | 250 | + assert get_calls[1].kwargs["params"] == {"omit_license_details": "true"} | 248 | 251 | core.sdk.fullscans.stream_diff.assert_not_called() | 249 | 252 |
| 250 | 253 | # Verify the results | @@ -263,10 +266,12 @@ def test_get_added_and_removed_packages_license_override(core): | 263 | 266 | """The include_license_details override seam still works when explicitly requested.""" | 264 | 267 | core.get_added_and_removed_packages("head", "new", include_license_details=True) | 265 | 268 |
| | 269 | + # When license details are wanted, the cached poll response is used directly | | 270 | + # — no lean re-fetch, and omit_license_details is not sent. | 266 | 271 | core.sdk.diffscans.get.assert_called_once_with( | 267 | 272 | core.config.org_slug, | 268 | 273 | "diff-scan-123", | 269 | | - params={"cached": "true", "omit_license_details": "false"}, | | 274 | + params={"cached": "true"}, | 270 | 275 | ) | 271 | 276 |
| 272 | 277 | def test_empty_alerts_preserved(core): | |
|