netbox-community / pynetbox

Python API client library for Netbox.
Apache License 2.0
567 stars 168 forks source link

core/response.py: avoid _endpoint_from_url if url is empty #632

Closed elukey closed 1 month ago

elukey commented 2 months ago

Hi folks!

First pull request for me so please be patient :)

When testing pynetbox with Netbox 4's API, I got the following error:

line 436, in _endpoint_from_url
    split_url_path = url_path.split("/")
TypeError: a bytes-like object is required, not 'str'

With some logging it was clear that url_path was b'', and in turn "path" itself was b'' as well.

The caller seemed to be:

line 286, in __init__
    self._endpoint_from_url(values["url"])

Adding more debugging to __init__ led to the following dump of the values dict:

{'obj': None, 'url': None, 'time': '2024-07-31T12:41:33.339876+00:00',
 'status': 'success',
 'message': 'Generated successfully, see the output tab for result.'}

I am not 100% sure th origin of that dict, but it should be safer to test values["url"] for None/empty values before calling _endpoint_from_url.

markkuleinio commented 2 months ago

What's the case when url is empty?

XioNoX commented 2 months ago

Running Netbox 4.0.8, below you can find the full output of one of our scripts (https://github.com/wikimedia/operations-software-netbox-extras/blob/master/customscripts/capirca.py) The key ['results']['data']['log']['url'] exists, but its value is null

{
    "id": 1,
    "url": "https://netbox.wikimedia.org/api/extras/scripts/1/",
    "module": 1,
    "name": "GetHosts",
    "description": "Returns all the Netbox hosts IPs, Anycast IPs and VIPs in a Capirca [NETWORKS.net](http://networks.net/) format.",
    "vars": {},
    "result": {
        "id": 58022,
        "url": "https://netbox.wikimedia.org/api/core/jobs/58022/",
        "display": "1931e338-1beb-4d1a-9569-9ac539b43411",
        "object_type": "extras.script",
        "object_id": 1,
        "name": "GetHosts",
        "status": {
            "value": "completed",
            "label": "Completed"
        },
        "created": "2024-08-01T07:07:26.894324Z",
        "scheduled": null,
        "interval": null,
        "started": "2024-08-01T07:07:26.943559Z",
        "completed": "2024-08-01T07:08:53.222116Z",
        "user": {
            "id": 6,
            "url": "https://netbox.wikimedia.org/api/users/users/6/",
            "display": "ayounsi (Ayounsi)",
            "username": "ayounsi"
        },
        "data": {
            "log": [
                {
                    "obj": null,
                    "url": null,
                    "time": "2024-08-01T07:08:53.192589+00:00",
                    "status": "success",
                    "message": "Generated successfully, see the output tab for result."
                }
            ],
            "tests": {},
            "output": "[truncated]"
        },
        "error": "",
        "job_id": "1931e338-1beb-4d1a-9569-9ac539b43411"
    },
    "display": "GetHosts (capirca)",
    "is_executable": true
}
XioNoX commented 2 months ago

After some more investigation, it seems like the bug was introduced in Netbox 4.0.6 with this PR: https://github.com/netbox-community/netbox/pull/16271

elukey commented 2 months ago

@markkuleinio Hi! My opinion is that pynetbox should benefit to be a little more defensive when using a dict value, in this case it seems a good protection regardless of the origin of the issue. Let us know what you think about it!

markkuleinio commented 2 months ago

@markkuleinio Hi! My opinion is that pynetbox should benefit to be a little more defensive when using a dict value, in this case it seems a good protection regardless of the origin of the issue. Let us know what you think about it!

It really does not matter what I think about it. That said, @XioNoX provided a valid case. pynetbox development has sadly not progressed lately. My production environments are still at 3.7, and when I'm going to 4.x, I likely fork my own internal and slimlined package to have the fixes I need at that point. But let's see, I think NetBox Labs had recently some job opening for a developer that would also assist in pynetbox, maybe they'll progress with the fixes again.

elukey commented 2 months ago

@markkuleinio Hi! My opinion is that pynetbox should benefit to be a little more defensive when using a dict value, in this case it seems a good protection regardless of the origin of the issue. Let us know what you think about it!

It really does not matter what I think about it.

@markkuleinio probably it doesn't matter to be polite with others on Github too, as I see it. Jumping on an issue and providing this kind of feedback is not constructive in my opinion, but YMMV.

markkuleinio commented 2 months ago

Umm, I don't get the point of your message. You submitted a PR for an issue that another user later proved worthy, and something I said upset you? Sure, I hope the maintainers will review and merge your PR, just like the other PRs that fix issues in pynetbox. Let me know if you need some clarification from me.

elukey commented 2 months ago

Umm, I don't get the point of your message. You submitted a PR for an issue that another user later proved worthy, and something I said upset you? Sure, I hope the maintainers will review and merge your PR, just like the other PRs that fix issues in pynetbox. Let me know if you need some clarification from me.

No clarification needed, I just find your tone and way of answering others question is not very welcoming/friendly. As I pointed out, YMMV, no problem at all.

markkuleinio commented 2 months ago

Ok, I think I got it now. You spent time and effort in your first ever PR, and I (another pynetbox user) did not appreciate your request for comments enough. Please take my assurance that I was solely concentrating on the goal of getting the fix into the pynetbox codebase, and in that perspective I meant just what I said: my opinion does not weigh anything when the pynetbox maintainers possibly at some point come and see this PR. If you took that as an understating comment to your work or you as a person, I apologise, that was not the purpose. Now that NetBox 4.0 API clearly injects null URLs in the responses (in contrary to the older versions), let's hope your PR gets traction and gets merged, and it then gives one more reason for me to not to have to fork this package for my own purposes.

XioNoX commented 1 month ago

@jeremystretch hi, I've been told you could maybe help get this (hopefully simple) patch reviewed and released to unbreak pynetbox on Netbox 4.0.6 and above. Thanks