Add vendor lookup endpoint #3354

stveit · 2025-04-28T17:15:06Z

Adds endpoint that accepts queries of a single MAC address with GET requests using url parameter ?mac, and a bulk query using POST with a JSON body.

Endpoint: /api/vendor/

github-actions · 2025-04-28T17:16:17Z

🦙 MegaLinter status: ✅ SUCCESS

Descriptor	Linter	Files	Fixed	Errors	Warnings	Elapsed time
✅ PYTHON	black	3		0	0	0.81s
✅ PYTHON	ruff	3		0	0	0.14s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

github-actions · 2025-04-28T17:22:54Z

Test results

12 files 12 suites 11m 53s ⏱️
2 206 tests 2 206 ✅ 0 💤 0 ❌
6 093 runs 6 093 ✅ 0 💤 0 ❌

Results for commit fafcba0.

♻️ This comment has been updated with latest results.

johannaengland

Some adding to the documentation at https://nav.readthedocs.io/en/latest/howto/api_parameters.html#detailed-endpoint-specifications is needed

johannaengland · 2025-04-30T11:17:21Z

python/nav/web/api/v1/views.py

+        if not mac:
+            return Response("Missing MAC address", status=status.HTTP_400_BAD_REQUEST)


This leads to already getting a 404 response when just initially accessing the endpoint, if you're using a browser to access the API

Not sure how to fix this issue honestly ..

Maybe just a 200 with the message "No mac address selected"

Made it so it just returns an empty dict like the POST endpoint does if you give it an empty list

johannaengland · 2025-04-30T11:19:28Z

python/nav/web/api/v1/views.py

+    @staticmethod
+    def post(request):
+        try:
+            mac_addresses = json.loads(request.body.decode('utf-8'))


If I use a browser to access the API and try to post to this endpoint I always get "Invalid JSON"

request.body looks like this in this case: _content_type=application%2Fjson&_content=%5B%22aa%3Abb%3Acc%3Add%3Aee%3Aff%22%2C+%2211%3A22%3A33%3A44%3A55%3A66%22%5D

Maybe request.data should be used instead here? Or does that only work if the JSON is a dict?

request.data works, but behaves differently based on what made the request. Via curl it is automatically converted from JSON to a list (including error messages if the JSON is invalid). So if you send a curl post with a JSON array, request.data will give you a list directly. But with the browseable API request.data is a QueryDict where the json string is accessible via the _content key.

Would be nice if the browseable API requsts also got automatically parsed. I imagine ModelViews has some builtin stuff that handles this, so would be nice if that can be hooked into somehow.

Made a fix specifically so the browseable API works, but its not all that pretty.. basically extra parsing to support this specific case

johannaengland · 2025-04-30T13:03:17Z

python/nav/web/api/v1/views.py

+        try:
+            results = get_vendor_names(mac_addresses)
+        except ValueError:
+            return Response("Invalid MAC address", status=status.HTTP_400_BAD_REQUEST)


Would be nice to here return which one(s) is/are invalid

This has been added

johannaengland · 2025-04-30T13:04:06Z

python/nav/web/api/v1/views.py

+        try:
+            results = get_vendor_names([mac])
+        except ValueError:
+            return Response("Invalid MAC address", status=status.HTTP_400_BAD_REQUEST)


Suggested change

return Response("Invalid MAC address", status=status.HTTP_400_BAD_REQUEST)

return Response(f"Invalid MAC address '{mac}'", status=status.HTTP_400_BAD_REQUEST)

johannaengland · 2025-04-30T13:25:16Z

tests/integration/api_test.py

@@ -346,6 +350,106 @@ def test_interface_with_last_used_should_be_listable(
    assert response.status_code == 200


+class TestVendorLookupGet:


The lines

endpoint = 'vendor' create_token_endpoint(token, endpoint)

feel like something we can set up here for the whole class instead of repeating it in every test

Changed it so it now uses a fixture

johannaengland · 2025-04-30T13:27:54Z

tests/integration/api_test.py

+        response = api_client.get(ENDPOINTS[endpoint])
+        assert response.status_code == 400
+
+


Same comment as above

A vendor-endpoint specific fixture would do, I guess :)

johannaengland · 2025-04-30T13:33:18Z

The SQL part looks perfect for @lunkwill42 to have a look at 😁

lunkwill42 · 2025-05-05T13:13:44Z

python/nav/web/api/v1/urls.py

@@ -73,4 +73,5 @@
        name="prefix-usage-detail",
    ),
    re_path(r'^', include(router.urls)),
+    re_path(r'^vendor/?$', views.VendorLookup.as_view(), name='vendor'),


My DRF-fu is rusty (as mentioned in another PR review today). I assume the primary reason the url pattern is added explicitly, rather than through DRF's router-thingy is that this view isn't actually a model view with built-in CRUD-magic?

yes, using the router stuff did not seem to work very well with APIView

lunkwill42 · 2025-05-05T13:21:42Z

python/nav/web/api/v1/views.py

+    cursor.execute(
+        """
+        CREATE TEMPORARY TABLE temp_macaddrs (
+            mac macaddr PRIMARY KEY
+        )
+        """
+    )


This fails spectacularly when I issue two queries in a row. Why?

Because the temporary table is temporary within the current database session. The web server process may re-use the same session until something forces it to be closed/reset. Which means that the next call to this API endpoint may be re-using the same session as the previous call to it - and the temporary table is still defined there.

Traceback (most recent call last): File "/home/vscode/.venv/lib/python3.9/site-packages/django/db/backends/utils.py", line 87, in _execute return self.cursor.execute(sql) The above exception (relation "temp_macaddrs" already exists ) was the direct cause of the following exception: File "/home/vscode/.venv/lib/python3.9/site-packages/django/core/handlers/exception.py", line 55, in inner response = get_response(request) File "/home/vscode/.venv/lib/python3.9/site-packages/django/core/handlers/base.py", line 197, in _get_response response = wrapped_callback(request, *callback_args, **callback_kwargs) File "/home/vscode/.venv/lib/python3.9/site-packages/django/views/decorators/csrf.py", line 56, in wrapper_view return view_func(*args, **kwargs) File "/home/vscode/.venv/lib/python3.9/site-packages/django/views/generic/base.py", line 104, in view return self.dispatch(request, *args, **kwargs) File "/home/vscode/.venv/lib/python3.9/site-packages/rest_framework/views.py", line 515, in dispatch response = self.handle_exception(exc) File "/home/vscode/.venv/lib/python3.9/site-packages/rest_framework/views.py", line 475, in handle_exception self.raise_uncaught_exception(exc) File "/home/vscode/.venv/lib/python3.9/site-packages/rest_framework/views.py", line 486, in raise_uncaught_exception raise exc File "/home/vscode/.venv/lib/python3.9/site-packages/rest_framework/views.py", line 512, in dispatch response = handler(request, *args, **kwargs) File "/workspaces/nav/python/nav/web/api/v1/views.py", line 1195, in get results = get_vendor_names([mac]) File "/usr/local/lib/python3.9/contextlib.py", line 79, in inner return func(*args, **kwds) File "/workspaces/nav/python/nav/web/api/v1/views.py", line 1230, in get_vendor_names cursor.execute( File "/home/vscode/.venv/lib/python3.9/site-packages/django/db/backends/utils.py", line 102, in execute return super().execute(sql, params) File "/home/vscode/.venv/lib/python3.9/site-packages/django/db/backends/utils.py", line 67, in execute return self._execute_with_wrappers( File "/home/vscode/.venv/lib/python3.9/site-packages/django/db/backends/utils.py", line 80, in _execute_with_wrappers return executor(sql, params, many, context) File "/home/vscode/.venv/lib/python3.9/site-packages/django/db/backends/utils.py", line 89, in _execute return self.cursor.execute(sql, params) File "/home/vscode/.venv/lib/python3.9/site-packages/django/db/utils.py", line 91, in __exit__ raise dj_exc_value.with_traceback(traceback) from exc_value File "/home/vscode/.venv/lib/python3.9/site-packages/django/db/backends/utils.py", line 87, in _execute return self.cursor.execute(sql) Exception Type: ProgrammingError at /api/1/vendor Exception Value: relation "temp_macaddrs" already exists

The temporary table should be explicitly deleted once the query is complete.

Since we're not using an async web server or Django's async stuff, I'm going to assume no other code is using the same session at the same time (otherwise, this might lead to conflicts even when two simultaneous API requests are processed at the same time). You may still want to consider adding a suffix to the temporary table that makes it unique to this specific API request.

Damn, I thought the session was closed and any temporary tables deleted when the function tagged with @django.db.transaction.atomic was completed 😞

lunkwill42 · 2025-05-05T13:37:23Z

python/nav/web/api/v1/views.py

+    cursor.execute(
+        """
+        SELECT mac, vendor FROM temp_macaddrs INNER JOIN oui ON trunc(mac)=oui
+        """
+    )


A potential different way of approaching this is to avoid any use of a temporary table, and instead materialize an in-query/in-memory table to join with:

SELECT mac, vendor FROM ( VALUES ('aa:bb:cc:dd:ee:ff'::macaddr), ('11:22:33:44:55:66'::macaddr), ('77:88:99:aa:bb:cc'::macaddr) ) AS temp_macaddrs(mac) INNER JOIN oui ON trunc(temp_macaddrs.mac) = oui.oui;

But this of course implies that you must validate all the incoming MAC addresses in Python before attempting to build a SQL query from them (no SQL injections, please).

yeah I would rather let postgres deal with validation of mac addresses, but maybe theres a way to do this with parametrization so you cant sql inject.

Using one query per mac address will prob make this easier I think, but I imagine it will be quite a lot slower when feeding it with a lot of mac addresses

Coercing every incoming mac address to nav.macaddress.MacAddress before using that as input to PostgreSQL shouldn't be too difficult...

that seems reasonable, ill give it a try

Implemented these suggestions

lunkwill42 · 2025-05-05T13:38:24Z

tests/integration/api_test.py

+        response = api_client.get(ENDPOINTS[endpoint])
+        assert response.status_code == 400
+
+


A vendor-endpoint specific fixture would do, I guess :)

request.data seems to be automatically conferted to a list when sending requests via cURL (and in tests), but when you use the browseable API request.data is instead a QueryDict that containst the JSON string. This adds support for both types, but its abit messy.. I assume ModelView has something built in that handles this stuff, so maybe Django has a function that can do this for us? Perhaps a Mixin or something like that

sonarqubecloud · 2025-05-09T07:08:26Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

stveit added the api label Apr 28, 2025

stveit self-assigned this Apr 28, 2025

stveit force-pushed the oui-api-endpoint branch from 6c15ccf to ff172ed Compare April 28, 2025 17:16

stveit force-pushed the oui-api-endpoint branch from ff172ed to 5535411 Compare April 29, 2025 07:19

stveit marked this pull request as ready for review April 29, 2025 07:19

stveit force-pushed the oui-api-endpoint branch from 5535411 to 363a643 Compare April 29, 2025 07:21

Add vendor/oui lookup endpoint

a34ad52

stveit force-pushed the oui-api-endpoint branch from 363a643 to a34ad52 Compare April 29, 2025 07:25

johannaengland requested a review from a team April 30, 2025 07:54

johannaengland requested changes Apr 30, 2025

View reviewed changes

johannaengland requested a review from lunkwill42 April 30, 2025 13:29

lunkwill42 requested changes May 5, 2025

View reviewed changes

stveit mentioned this pull request May 6, 2025

Add endpoint for JWT refresh tokens #3270

Open

stveit added 8 commits May 7, 2025 14:45

fixup! Add vendor/oui lookup endpoint

67344b4

fixup! Add vendor/oui lookup endpoint

de2ae4e

fixup! Add vendor/oui lookup endpoint

44e9fd0

fixup! Add vendor/oui lookup endpoint

da95afd

fixup! Handle queries from browseable API

f7f50ca

fixup! fixup! Add vendor/oui lookup endpoint

91e2418

fixup! fixup! fixup! Add vendor/oui lookup endpoint

fafcba0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vendor lookup endpoint #3354

Add vendor lookup endpoint #3354

stveit commented Apr 28, 2025 •

edited by johannaengland

Loading

github-actions bot commented Apr 28, 2025 •

edited

Loading

github-actions bot commented Apr 28, 2025 •

edited

Loading

johannaengland left a comment

johannaengland Apr 30, 2025

stveit May 7, 2025 •

edited

Loading

johannaengland May 7, 2025

stveit May 9, 2025

johannaengland Apr 30, 2025

stveit May 6, 2025

stveit May 7, 2025 •

edited

Loading

stveit May 8, 2025

johannaengland Apr 30, 2025

stveit May 7, 2025

johannaengland Apr 30, 2025

stveit May 7, 2025

johannaengland Apr 30, 2025

stveit May 7, 2025

johannaengland Apr 30, 2025

lunkwill42 May 5, 2025

johannaengland commented Apr 30, 2025

lunkwill42 May 5, 2025

stveit May 5, 2025

lunkwill42 May 5, 2025

stveit May 5, 2025

lunkwill42 May 5, 2025

stveit May 5, 2025 •

edited

Loading

lunkwill42 May 6, 2025

stveit May 6, 2025

stveit May 7, 2025

lunkwill42 May 5, 2025

sonarqubecloud bot commented May 9, 2025

		if not mac:
		return Response("Missing MAC address", status=status.HTTP_400_BAD_REQUEST)

	return Response("Invalid MAC address", status=status.HTTP_400_BAD_REQUEST)
	return Response(f"Invalid MAC address '{mac}'", status=status.HTTP_400_BAD_REQUEST)

		@@ -346,6 +350,106 @@ def test_interface_with_last_used_should_be_listable(
		assert response.status_code == 200


		class TestVendorLookupGet:

		response = api_client.get(ENDPOINTS[endpoint])
		assert response.status_code == 400

Add vendor lookup endpoint #3354

Are you sure you want to change the base?

Add vendor lookup endpoint #3354

Conversation

stveit commented Apr 28, 2025 • edited by johannaengland Loading

github-actions bot commented Apr 28, 2025 • edited Loading

🦙 MegaLinter status: ✅ SUCCESS

github-actions bot commented Apr 28, 2025 • edited Loading

Test results

johannaengland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stveit May 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stveit May 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannaengland commented Apr 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stveit May 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented May 9, 2025

Quality Gate passed

stveit commented Apr 28, 2025 •

edited by johannaengland

Loading

github-actions bot commented Apr 28, 2025 •

edited

Loading

github-actions bot commented Apr 28, 2025 •

edited

Loading

stveit May 7, 2025 •

edited

Loading

stveit May 7, 2025 •

edited

Loading

stveit May 5, 2025 •

edited

Loading