Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

elukey
Copy link
Contributor

@elukey elukey commented Jul 31, 2024

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.

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
Copy link
Contributor

What's the case when url is empty?

@XioNoX
Copy link

XioNoX commented Aug 1, 2024

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
Copy link

XioNoX commented Aug 2, 2024

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

@elukey
Copy link
Contributor Author

elukey commented Aug 2, 2024

@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
Copy link
Contributor

@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.

wmfgerrit pushed a commit to wikimedia/operations-software-netbox-extras that referenced this pull request Aug 2, 2024
New netbox feature, but also breaks pynetbox:
netbox-community/pynetbox#632

It's nice to have it anyway, and it will workaround the "url" bug in
most usecases.

+ add some types

Bug: T371653
Change-Id: I2a1108572ccffb01d8c939654d37c4b40ce2dc2a
wmfgerrit pushed a commit to wikimedia/operations-software-netbox-extras that referenced this pull request Aug 2, 2024
Not needed and triggers a bug in pynetbox:
netbox-community/pynetbox#632

Bug: T371653
Change-Id: If87b7c45d05565f8b8b5139c63b9ad081c555b78
@elukey
Copy link
Contributor Author

elukey commented Aug 2, 2024

@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
Copy link
Contributor

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
Copy link
Contributor Author

elukey commented Aug 2, 2024

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
Copy link
Contributor

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
Copy link

XioNoX commented Aug 7, 2024

@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

@arthanson arthanson merged commit ad8bc8b into netbox-community:master Aug 7, 2024
16 checks passed
wmfgerrit pushed a commit to wikimedia/operations-software-homer-deploy that referenced this pull request Aug 13, 2024
paramiko/paramiko#2419
netbox-community/pynetbox#632

Bug: T371890
Change-Id: I103664be599d617d3c8b7c102a039323c94c6486
wmfgerrit pushed a commit to wikimedia/operations-puppet that referenced this pull request Aug 19, 2024
Fixing Netbox 4 breaking change
Fixing some minor linting issues

Not tested as impacted by:
netbox-community/pynetbox#632

Change-Id: I4a42ced0c10eb7c75547bca157e98da154506325
wmfgerrit pushed a commit to wikimedia/operations-software-netbox-deploy that referenced this pull request Aug 20, 2024
netbox-community/pynetbox#632

Bug: T371890
Change-Id: I951fda89d553731e7c9fa07fd5214278f69028e9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants