search

netbox-community / pynetbox

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

Record url is not set + Endpoint is not updated #161

Closed louis-6wind closed 4 years ago

louis-6wind commented 5 years ago

Environment Python version: 3.6.6 NetBox version: 2.5.10 PynetBox version: 4.0.6

Steps to reproduce : Python 3.6.6 (default, Aug 13 2018, 18:24:23) [GCC 4.8.5 20150623 (Red Hat 4.8.5-28)] on linux Type "help", "copyright", "credits" or "license" for more information.

import pynetbox api = pynetbox.api( ... 'http://YYYYYYYY', ... token='XXXXXXXXX', ... )

db=api.dcim.device_bays.get(1) db.url db.endpoint.url 'http://YYYYYYYY/api/dcim/device-bays'

device=db.installed_device device.url 'http://YYYYYYYY/api/dcim/devices/2/' device.endpoint.url 'http://YYYYYYYY/api/dcim/device-bays' api.dcim.device_bays.get(2).delete() True device.delete() Traceback (most recent call last): File "", line 1, in File "/usr/local/lib/python3.6/site-packages/pynetbox/core/response.py", line 423, in delete return True if req.delete() else False File "/usr/local/lib/python3.6/site-packages/pynetbox/core/query.py", line 318, in delete raise RequestError(req) pynetbox.core.query.RequestError: The requested url: http://YYYYYYYY/api/dcim/device-bays/2/ could not be found.

Expected Behavior

Observed Behavior

louis-6wind commented 5 years ago

It seems the bug is there in response.py def _parse_values(self, values) -> def list_parser(list_item) . self.endpoint is used there. In my opinion, a new endpoint should be used:

v = lookup(v, self.api, self.endpoint) v = self.default_ret(v, self.api, self.endpoint)

louis-6wind commented 5 years ago

Please kindly review the pull request

zachmoody commented 5 years ago

Hi @louis-oui, thanks for the bug report and PR. I'm not sure I want to go down the path of making nested Records equivalent to Records returned from an Endpoint. Nested Records were really only intended for read operations. The docs, and code, should be more explicit about that.

It's just my opinion, but I feel it greatly increases the potential for less than intuitive client code and unexpected behavior. That all being said, I'm not completely opposed to it. Will this issue open for a bit to see it gets traction.

Btw, somehow PR #148 got mixed up in your PR.

louis-6wind commented 5 years ago

Thank you for your review.

I mistakely put PR #148. Sorry for that.

In current code, Record delete method use the endpoint url to delete the record (this is what I have observed). If Record endpoint url is from a nested Record, it will try to delete another Record.

In addition to that, it could be useful to use endpoint url to know which kind of record it is

louis-6wind commented 5 years ago

Hello @zachmoody

Any news on the issue ?

According to me, it a bug. I think it will be useful to correct the delete issue that mistakenly try to delete nested objetct with ID with the endpoint url.

zachmoody commented 5 years ago

Hey @louis-oui - I was just leaving it open to see if anyone else had an opinion. I agree that it's a bug, I'm just not completely sure the best way to handle it.

louis-6wind commented 5 years ago

I clicked on the wrong button. Let me know how you want to handle the bug. Thank you

louis-6wind commented 5 years ago

@zachmoody kindly handle the bug. Your help is appreciated

kartiksubbarao commented 5 years ago

I'm running into the same issue as well, while trying to delete Cable objects inside Interface objects. I'm working around the issue as follows:

def delete_interface_cable(cable):
    cable.endpoint.url = cable.endpoint.url.replace('interfaces', 'cables')
    cable.delete()

interface = nb.dcim.interfaces.get(device='host1.example.com', name='eth0')
delete_interface_cable(interface.cable)

I think it's useful to be able to delete cable objects that are discovered within interface objects (i.e. as opposed to having to separately search for them).

louis-6wind commented 4 years ago

@zachmoody Because of this bug, some devices on our netbox were wrongly deleted.

What is the blocker to not merge the fix ?

zachmoody commented 4 years ago

What is the blocker to not merge the fix ?

Mainly keeping the API's behavior intuitive. While device > device-bay and interface > cable are great examples of where this behavior works well. The PR would allow ones that would be unintuitive, like deleting a site from an IPAddress.

Ideally, we'd allow it in the places it's logical but otherwise disallow it. Just haven't had the time to address it properly.

louis-6wind commented 4 years ago

Thank your for your reply.

Pynetbox is an awesome project that saved me a lot of time but current behaviour does not fit my needs even for read-only operation.

In that situation, I cannot know which kind of cable termination termination_a is refering:

cable = nb.dcim.cables.get(7605)
cable.termination_a.endpoint.url
'http://XXX/api/dcim/cables'

Results is always cables url. But it could be interface, front_ports... I need this info to manage patch panel with netbox. I have not found any workaround for this.

Mainly keeping the API's behavior intuitive. While device > device-bay and interface > cable are great examples of where this behavior works well.

I would to like to contribute to the pynetbox project and to find a common solution for this. I can take more times to suggest you a new pull request on a behaviour that we both agree. However, am not sure to understand what is your concern about this. Is it the following behaviour that you want to keep ?

I'm not sure I want to go down the path of making nested Records equivalent to Records returned from an Endpoint.

What is the use case for keeping nested Records equivalent to Records returned from an Endpoint ?

May be we can create a new different endpoint attribute to store the url on which delete function will rely on to do the delete API request. Also it will be used to know which type of record a nested record is.

The PR would allow ones that would be unintuitive, like deleting a site from an IPAddress. In that case, site deletion would imply IPAddress->Device->Site.

It seems that deletion operation is explicit in the code. It is unlikely that someone deletes an object like a site by mistake.

zachmoody commented 4 years ago

In that situation, I cannot know which kind of cable termination termination_a is refering:

cable = nb.dcim.cables.get(7605)
cable.termination_a.endpoint.url
'http://XXX/api/dcim/cables'

Results is always cables url. But it could be interface, front_ports... I need this info to manage patch panel with netbox. I have not found any workaround for this.

fwiw, there should be a termination_a_type attr that should tell you this on cables.

I would to like to contribute to the pynetbox project and to find a common solution for this. I can take more times to suggest you a new pull request on a behaviour that we both agree. However, am not sure to understand what is your concern about this. Is it the following behaviour that you want to keep ?

Yeah, I agree it makes sense that at least in those two examples you should be able to perform write operations on nested objects. There may be more, but they can be addressed as they come up as. I'm interested in coming up with a uniform way to handle them.

I'm not sure I want to go down the path of making nested Records equivalent to Records returned from an Endpoint.

What is the use case for keeping nested Records equivalent to Records returned from an Endpoint ?

May be we can create a new different endpoint attribute to store the url on which delete function will rely on to do the delete API request. Also it will be used to know which type of record a nested record is.

I'm beginning to feel they shouldn't be equivalent. Like, nested objects might need to be some kind of read-only record objects that have write ops raise NotImpelemented. But that's the easy part, building in the desired behavior where we want it, and sorta how that plays in with custom models, is where I have to stop thinking about it. :grimacing:

It seems that deletion operation is explicit in the code. It is unlikely that someone deletes an object like a site by mistake.

I don't disagree, but having a calamity like that just a miss-assigned variable away is something I'm leery of.

louis-6wind commented 4 years ago

fwiw, there should be a termination_a_type attr that should tell you this on cables.

You are right. I took a wrong example. I will let you know what was my real issue.

I seems it is unecessary to implement safeguard to avoid undesired write operation because netbox application does not allow deleting objects that are used by other objects.

Let's take the example of deletion of site from IPaddresses with the fix :

>>> nb.ipam.ip_addresses.get(614)                             
10.10.20.1/30
>>> ip=nb.ipam.ip_addresses.get(614)
>>> intf=ip.interface
>>> device=intf.device
>>> site=device.site
>>> site.delete()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.6/site-packages/pynetbox-4.0.7.dev34+g4c1a140-py3.6.egg/pynetbox/core/response.py", line 461, in delete
    return True if req.delete() else False
  File "/usr/local/lib/python3.6/site-packages/pynetbox-4.0.7.dev34+g4c1a140-py3.6.egg/pynetbox/core/query.py", line 342, in delete
    raise RequestError(req)
pynetbox.core.query.RequestError: The request failed with code 409 Conflict: {'detail': 'Unable to delete object. The following dependent objects were found: Side A (circuits.circuittermination), Side A (circuits.circuittermination), Side Z (circuits.circuittermination), Side Z (circuits.circuittermination), Side A (circuits.circuittermination)'}

Same in webui image

zachmoody commented 4 years ago

Yeah, that's a good point. Had forgotten about that behavior.

zachmoody commented 4 years ago

Yeah, it was mostly that. I think the API's cleaner when you can't do write ops on nested Records, but I admit that's not a good reason to prevent it. I'll have another look at the PR when I get a chance this week.

louis-6wind commented 4 years ago

Thank you for that. I updated the PR yesterday to resolve conflicts.