bug/category to viper role#170
Conversation
📝 WalkthroughWalkthroughThis PR refactors Viper integration to standardize JSON payloads to camelCase, changes vendor ID sourcing to use asset manufacturer, extends asset upsert to accept device classification fields, and validates the complete TapirXL telemetry pipeline with new regression tests. ChangesViper camelCase and device classification pipeline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
blueflow/celery/tasks.py (1)
31-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a timeout to the Viper callback POST
requests.post(...)at blueflow/celery/tasks.py:36 is a blocking network call with notimeout, so a slow/stuck callback can pin Celery workers.Suggested fix
response = requests.post( viper_data.callback, json=as_dict, headers={"Content-Type": "application/json"}, + timeout=30, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@blueflow/celery/tasks.py` around lines 31 - 41, The POST to the Viper callback in the loop uses requests.post(...) without a timeout which can block Celery workers; update the call in the loop (where ViperWebhookResponseList.from_request is iterated and response.to_dict() is used) to pass a reasonable timeout parameter (e.g., timeout=5) to requests.post and handle requests.exceptions.Timeout/RequestException around the call so the task can log the failure and continue/raise appropriately rather than blocking the worker.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@blueflow/tests/test_tapirxl_viper_regression.py`:
- Around line 164-214: Move all local imports (eg. "from blueflow import models"
found inside test_device_class_alias_persisted_through_upsert,
test_device_class_persisted_through_upsert,
test_version_persisted_through_upsert and other tests) to the top of the test
module; replace any identity comparisons against numeric HTTP status codes (e.g.
"is 201" or "is 200") with equality checks or named constants (e.g.
resp.status_code == 201 or use HTTPStatus.CREATED/OK) to remove magic-status
lint warnings; and shorten or rewrap the overlong docstring in
test_version_persisted_through_upsert so it fits the line length limit. Ensure
you update all occurrences (also around the other failing ranges mentioned) so
the module passes lint.
In `@blueflow/views/asset.py`:
- Around line 119-123: The current validate method overwrites category when
attrs contains a falsey value; change the condition to only map device_class if
the category key is absent. In the validate function, replace the truthiness
check (attrs.get("category")) with a key-presence check (e.g., "'category' not
in attrs") so attrs["category"] is set from device_class only when the category
key isn't provided. Ensure you keep the existing device_class extraction
(device_class = attrs.pop("device_class", None)) and the rest of the method
intact.
---
Outside diff comments:
In `@blueflow/celery/tasks.py`:
- Around line 31-41: The POST to the Viper callback in the loop uses
requests.post(...) without a timeout which can block Celery workers; update the
call in the loop (where ViperWebhookResponseList.from_request is iterated and
response.to_dict() is used) to pass a reasonable timeout parameter (e.g.,
timeout=5) to requests.post and handle
requests.exceptions.Timeout/RequestException around the call so the task can log
the failure and continue/raise appropriately rather than blocking the worker.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85a9dd16-3e17-4073-8006-823d4c1db71b
📒 Files selected for processing (7)
blueflow/celery/tasks.pyblueflow/celery/tests/test_viper.pyblueflow/models/viper.pyblueflow/tests/fixtures/tapirxl_telemetry.jsonlblueflow/tests/test_tapirxl_viper_regression.pyblueflow/tests/test_viper_models.pyblueflow/views/asset.py
| def test_device_class_alias_persisted_through_upsert(asset_edit_client) -> None: | ||
| """Raw TapirXL device_class field maps to category when Vector is bypassed.""" | ||
| from blueflow import models | ||
|
|
||
| resp = asset_edit_client.put( | ||
| "/api/assets/upsert/", | ||
| json.dumps( | ||
| { | ||
| "mac_address": "DE:AD:BE:EF:00:01", | ||
| "ip_address": "10.10.10.99", | ||
| "device_class": "patient_monitor", | ||
| } | ||
| ), | ||
| content_type="application/json", | ||
| ) | ||
| assert resp.status_code == 201 | ||
| asset = models.Asset.objects.get(mac_address="de:ad:be:ef:00:01") | ||
| assert asset.category == "patient_monitor" | ||
|
|
||
|
|
||
| def test_device_class_persisted_through_upsert(asset_edit_client) -> None: | ||
| """category from VRL transform survives PUT /api/assets/upsert/ → Asset.category.""" | ||
| from blueflow import models | ||
|
|
||
| payload = _vrl_transform( | ||
| { | ||
| "mac_address": "00:09:FB:BD:75:6D", | ||
| "ip_address": "10.10.10.21", | ||
| "hostname": "MX700-bed12", | ||
| "vendor": "philips", | ||
| "product": "intellivue_mx700", | ||
| "version": None, | ||
| "device_class": "patient_monitor", | ||
| "open_ports": [3702], | ||
| "confidence": "LOW", | ||
| } | ||
| ) | ||
|
|
||
| resp = asset_edit_client.put( | ||
| "/api/assets/upsert/", | ||
| json.dumps(payload), | ||
| content_type="application/json", | ||
| ) | ||
| assert resp.status_code == 201 | ||
| asset = models.Asset.objects.get(mac_address="00:09:fb:bd:75:6d") | ||
| assert asset.category == "patient_monitor" | ||
|
|
||
|
|
||
| def test_version_persisted_through_upsert(asset_edit_client) -> None: | ||
| """app_sw_version from VRL transform survives PUT /api/assets/upsert/ → Asset.app_sw_version.""" | ||
| from blueflow import models |
There was a problem hiding this comment.
Fix current lint blockers in this test module before merge.
The new file still fails lint on: imports inside functions (Line 166, Line 186, Line 214, Line 275), magic status code comparisons (Line 207, Line 236), and one overlong docstring line (Line 213).
Suggested fix
import json
import uuid
+from http import HTTPStatus
from pathlib import Path
from unittest.mock import patch
import pytest
+from blueflow import models
+from blueflow.celery.tasks import viper_webhook
from blueflow.models.viper import ViperWebhookRequest, ViperWebhookResponseList
@@
def test_device_class_alias_persisted_through_upsert(asset_edit_client) -> None:
"""Raw TapirXL device_class field maps to category when Vector is bypassed."""
- from blueflow import models
@@
- assert resp.status_code == 201
+ assert resp.status_code == HTTPStatus.CREATED
@@
def test_device_class_persisted_through_upsert(asset_edit_client) -> None:
"""category from VRL transform survives PUT /api/assets/upsert/ → Asset.category."""
- from blueflow import models
@@
- assert resp.status_code == 201
+ assert resp.status_code == HTTPStatus.CREATED
@@
def test_version_persisted_through_upsert(asset_edit_client) -> None:
- """app_sw_version from VRL transform survives PUT /api/assets/upsert/ → Asset.app_sw_version."""
- from blueflow import models
+ """app_sw_version from VRL transform survives
+ PUT /api/assets/upsert/ → Asset.app_sw_version.
+ """
@@
- assert resp.status_code == 201
+ assert resp.status_code == HTTPStatus.CREATED
@@
with patch("blueflow.celery.tasks.requests.post") as mock_post:
- from blueflow.celery.tasks import viper_webhook
-
viper_webhook.apply(Also applies to: 231-236, 275-287
🧰 Tools
🪛 GitHub Actions: Lint / 0_lint.txt
[error] 186-186: PLC0415: import should be at the top-level of a file
[error] 207-207: PLR2004: Magic value used in comparison; consider replacing 201 with a constant variable
[error] 213-213: E501: Line too long (100 > 88)
[error] 214-214: PLC0415: import should be at the top-level of a file
🪛 GitHub Actions: Lint / lint
[error] 186-186: PLC0415 import should be at the top-level of a file (from blueflow import models)
[error] 207-207: PLR2004 Magic value used in comparison, consider replacing 201 with a constant variable (assert resp.status_code == 201)
[error] 213-213: E501 Line too long (100 > 88) in test_version_persisted_through_upsert docstring
[error] 214-214: PLC0415 import should be at the top-level of a file (from blueflow import models)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@blueflow/tests/test_tapirxl_viper_regression.py` around lines 164 - 214, Move
all local imports (eg. "from blueflow import models" found inside
test_device_class_alias_persisted_through_upsert,
test_device_class_persisted_through_upsert,
test_version_persisted_through_upsert and other tests) to the top of the test
module; replace any identity comparisons against numeric HTTP status codes (e.g.
"is 201" or "is 200") with equality checks or named constants (e.g.
resp.status_code == 201 or use HTTPStatus.CREATED/OK) to remove magic-status
lint warnings; and shorten or rewrap the overlong docstring in
test_version_persisted_through_upsert so it fits the line length limit. Ensure
you update all occurrences (also around the other failing ranges mentioned) so
the module passes lint.
There was a problem hiding this comment.
further to the rabbit's point django has a preferred pattern for lazy imports: https://docs.djangoproject.com/en/6.0/ref/applications/#django.apps.apps.get_model
There was a problem hiding this comment.
@t-a-y-l-o-r — great callout on apps.get_model(). That's the right Django idiom for production code where lazy lookup is needed to break circular import cycles — e.g. inside model methods, signal handlers, or anything that may execute before the full app registry is ready:
# Good in production code (models, signals, middleware)
from django.apps import apps
def some_signal_handler(sender, instance, **kwargs):
Asset = apps.get_model("blueflow", "Asset")
Asset.objects.filter(...)In test files though, pytest-django fully populates the app registry before any test runs, so circular imports aren't a concern. The standard pattern — and the one that satisfies the PLC0415 lint rule — is a plain top-level import:
# Idiomatic for test files
from blueflow import models # ← top of file
def test_device_class_alias_persisted_through_upsert(asset_edit_client) -> None:
...
asset = models.Asset.objects.get(mac_address="de:ad:be:ef:00:01")So for this PR the lint fix is just hoisting the from blueflow import models (and from blueflow.celery.tasks import viper_webhook) to the module top, which is both simpler and what the linter expects.
✏️ Learnings added
Learnt from: t-a-y-l-o-r
Repo: virtalabs/blueflow PR: 170
File: blueflow/tests/test_tapirxl_viper_regression.py:164-214
Timestamp: 2026-05-25T16:10:21.212Z
Learning: In the blueflow project (virtalabs/blueflow), `apps.get_model()` is the Django-preferred pattern for lazy model lookups in production code (model methods, signals, middleware) to avoid circular import cycles. In test files, however, pytest-django fully populates the app registry before tests run, so plain top-level imports (`from blueflow import models`) are both correct and idiomatic — and are required by the PLC0415 lint rule. Do not flag top-level imports in test files as needing `apps.get_model()` replacement.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: t-a-y-l-o-r
Repo: virtalabs/blueflow PR: 78
File: blueflow/views/asset.py:1058-1058
Timestamp: 2026-04-07T16:44:13.557Z
Learning: In blueflow’s Django `Asset` model (`blueflow/models`), `open_ports_tcp` is a list field declared with `default=list`, so it will not be `None` at the model/database level. When reviewing code, avoid flagging (or adding) unnecessary guards such as `asset.open_ports_tcp or []`; you can treat `asset.open_ports_tcp` as an always-present list.
| def validate(self, attrs: dict) -> dict: | ||
| """Map TapirXL ``device_class`` to ``category`` when Vector is bypassed.""" | ||
| device_class = attrs.pop("device_class", None) | ||
| if device_class and not attrs.get("category"): | ||
| attrs["category"] = device_class |
There was a problem hiding this comment.
Map device_class only when category is truly absent.
Line 122 currently treats falsey values ("", None) as missing and will overwrite an explicit category input. The contract here is “when category is not present”.
Suggested fix
- if device_class and not attrs.get("category"):
+ if device_class and "category" not in attrs:
attrs["category"] = device_class🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@blueflow/views/asset.py` around lines 119 - 123, The current validate method
overwrites category when attrs contains a falsey value; change the condition to
only map device_class if the category key is absent. In the validate function,
replace the truthiness check (attrs.get("category")) with a key-presence check
(e.g., "'category' not in attrs") so attrs["category"] is set from device_class
only when the category key isn't provided. Ensure you keep the existing
device_class extraction (device_class = attrs.pop("device_class", None)) and the
rest of the method intact.
t-a-y-l-o-r
left a comment
t-a-y-l-o-r
left a comment
There was a problem hiding this comment.
looks workable for hackathon
There was a problem hiding this comment.
I've been running with the assumption we dont want to normalize having network data in the repo just to be extra safe. I'm sure this is synthetic but maybe that's worth chatting about post-hackathon
| def test_device_class_alias_persisted_through_upsert(asset_edit_client) -> None: | ||
| """Raw TapirXL device_class field maps to category when Vector is bypassed.""" | ||
| from blueflow import models | ||
|
|
||
| resp = asset_edit_client.put( | ||
| "/api/assets/upsert/", | ||
| json.dumps( | ||
| { | ||
| "mac_address": "DE:AD:BE:EF:00:01", | ||
| "ip_address": "10.10.10.99", | ||
| "device_class": "patient_monitor", | ||
| } | ||
| ), | ||
| content_type="application/json", | ||
| ) | ||
| assert resp.status_code == 201 | ||
| asset = models.Asset.objects.get(mac_address="de:ad:be:ef:00:01") | ||
| assert asset.category == "patient_monitor" | ||
|
|
||
|
|
||
| def test_device_class_persisted_through_upsert(asset_edit_client) -> None: | ||
| """category from VRL transform survives PUT /api/assets/upsert/ → Asset.category.""" | ||
| from blueflow import models | ||
|
|
||
| payload = _vrl_transform( | ||
| { | ||
| "mac_address": "00:09:FB:BD:75:6D", | ||
| "ip_address": "10.10.10.21", | ||
| "hostname": "MX700-bed12", | ||
| "vendor": "philips", | ||
| "product": "intellivue_mx700", | ||
| "version": None, | ||
| "device_class": "patient_monitor", | ||
| "open_ports": [3702], | ||
| "confidence": "LOW", | ||
| } | ||
| ) | ||
|
|
||
| resp = asset_edit_client.put( | ||
| "/api/assets/upsert/", | ||
| json.dumps(payload), | ||
| content_type="application/json", | ||
| ) | ||
| assert resp.status_code == 201 | ||
| asset = models.Asset.objects.get(mac_address="00:09:fb:bd:75:6d") | ||
| assert asset.category == "patient_monitor" | ||
|
|
||
|
|
||
| def test_version_persisted_through_upsert(asset_edit_client) -> None: | ||
| """app_sw_version from VRL transform survives PUT /api/assets/upsert/ → Asset.app_sw_version.""" | ||
| from blueflow import models |
There was a problem hiding this comment.
further to the rabbit's point django has a preferred pattern for lazy imports: https://docs.djangoproject.com/en/6.0/ref/applications/#django.apps.apps.get_model
| required=False, allow_blank=True, allow_null=True | ||
| ) | ||
| os = serializers.CharField(required=False, allow_blank=True, allow_null=True) | ||
| app_sw_version = serializers.CharField( |
There was a problem hiding this comment.
all_blank and all_null in Django is generally considered an anti-pattern in Django. we should prefer either blank or null as our empty state; more post-hackathon stuff
There was a problem hiding this comment.
as a quick note, I generally make blank-able false and null-able true. But there are arguments for either
2 participants
Summary by CodeRabbit
New Features
app_sw_versionanddevice_classfields.Tests