netbox-community / pynetbox

Python API client library for Netbox.
Apache License 2.0
543 stars 167 forks source link

pynetbox overwrites it's own attributes when calling update() #499

Closed jwbensley closed 1 year ago

jwbensley commented 1 year ago

When calling update() in the example below, pynetbox replaces the tenant attribute (which is a Record object) of the prefix object, with an int:

$python3
Python 3.10.4 (main, Jun 29 2022, 12:14:53) [GCC 11.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import pynetbox
>>> nb = pynetbox.api('http://localhost:8080',token='0123456789abcdef0123456789abcdef01234567')
>>> prefix = nb.ipam.prefixes.get(id=1)
>>> prefix.tenant
Example Company
>>> type(prefix.tenant)
<class 'pynetbox.core.response.Record'>
>>> prefix.tenant.id
1
>>> type(prefix.tenant.id)
<class 'int'>
>>>
>>> prefix.update(
...     {
...         "tenant": 2,
...         "id": 1,
...     }
... )
True
>>> prefix.tenant
2
>>> prefix.tenant.id
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'int' object has no attribute 'id'
>>> type(prefix.tenant)
<class 'int'>

I have to GET the object again to see the updated version:

>>>
>>> prefix = nb.ipam.prefixes.get(id=1)
>>> prefix.tenant
customer1
>>> prefix.tenant.id
2

I think this is a bug because after calling update() I think prefix.tenant should be update to have an attribute prefix.tenant.id which is an int with the value of 2.

I also can't update the prefix two times in a row because the prefix object has replaced it's own attribute and methods locally instead of pushing the update to NetBox:

$python3
Python 3.10.4 (main, Jun 29 2022, 12:14:53) [GCC 11.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import pynetbox
>>> nb = pynetbox.api('http://localhost:8080',token='0123456789abcdef0123456789abcdef01234567')
>>> prefix = nb.ipam.prefixes.get(id=1)
>>> prefix.tenant.id
2
>>> prefix.update({"tenant": 1, "id": 1})
True
>>> prefix.update({"tenant": 2, "id": 1})
False           <<<<<<<<<<<<<<<<<<<<<<<<<<<<< 2nd update has failed
>>> prefix = nb.ipam.prefixes.get(id=1)
>>> prefix.update({"tenant": 2, "id": 1})
True
>>> 
markkuleinio commented 1 year ago

See https://github.com/netbox-community/pynetbox/issues/496#issuecomment-1238332529

jwbensley commented 1 year ago

@markkuleinio https://github.com/netbox-community/pynetbox/issues/496#issuecomment-1238332529 refers to a different problem.

That issue is about making an update, and the local object still containing the pre-update value.

I'm saying that after making an update, the local object is destroyed!

To try and be clearer, the issue in #496 is that after making an update to an ID value, the old ID value or the new ID value maybe stored in the local object. One needs to GET the data again from NetBox to be sure your are seeing the changes. A bit annoying that the local object doesn't update itself but I can live with that. What I'm saying in this issue, is that when making an update to an ID value, afterwards, there is no local ID value, regardless of weather it is the old or new ID value, the local object has deleted part of it's own attributes.

>>> type(prefix.tenant)
<class 'pynetbox.core.response.Record'>
>>> prefix.tenant.id
1
>>> type(prefix.tenant.id)
<class 'int'>
>>>
>>> prefix.update(
...     {
...         "tenant": 2,
...         "id": 1,
...     }
... )
True
>>>
>>> prefix.tenant
2
>>> type(prefix.tenant)
<class 'int'>

^ Why has prefix.tenant changed from a Record to an int?

>>> prefix.tenant.id
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'int' object has no attribute 'id'

^ Why has this attribute been removed? Shouldn't this just have the old value?

Surely this isn't expected behaviour?

markkuleinio commented 1 year ago

Why has prefix.tenant changed from a Record to an int?

Because you just replaced the tenant attribute value (the Record object) with an int 2.

As mentioned in the linked comment: if you update some of the objects properties and save it, pynetbox does not make extra calls to NetBox API automatically to update the full object (there is no other way to know all the field contents).

If you need the object to be fully updated, you need to call .full_details() after partially updating the object: that forces execution of extra NetBox API calls (one or more) to populate all the details.

jwbensley commented 1 year ago

Because you just replaced the tenant attribute value (the Record object) with an int 2.

How else can I update the tenant ID without destroying the object?

tenant_id doesn't work:

>>> prefix.update({"tenant_id": 2, "id": 1})
False

When I look at the NetBox API for PATCH it expects a value called tenant which is an int, but for GET it expects tenant_id: grafik

I should be able to say prefix.update({"tenant_id": 2, "id": 1}) or prefix.update({"tenant.id": 2, "id": 1}) or something similar I think, without destroying the object.

markkuleinio commented 1 year ago

How else can I update the tenant ID without destroying the object?

Oh, I didn't mean you did something wrong, that's how you should do it.

It is just a fact that when a Record object is replaced by an integer, there is no way for pynetbox to know what that integer means. (Or, what the other attributes of the tenant object should look like after the tenant ID is updated.) Pynetbox just sends that update request to NetBox and the object in NetBox is updated. Now, if that change is wanted to be fully reflected to the original object, at least one additional API call is needed to populate the object attributes. In the pynetbox design it has been left for the pynetbox user to decide if he/she wants those additional API calls to occur. Pynetbox as it currently is is not making very heavy abstraction over the NetBox API.

For example, in my own applications that heavily use pynetbox, I always decide in the coding process if I need to use that object again with the updated details. Sometimes I need, sometimes I don't. If I need, I'll then call the .full_details() method after making the update, and I'm good to fully use that same object later in the same application invocation. But it costs me at least one additional API call to NetBox (which means accessing the database as well). In most cases my applications just make the changes and don't need to access the objects after that (or, it only accesses the unchanged attributes of the object), and in those cases I don't call that method and I save a lot of resources on the server (both NetBox and the database). In my cases I have quite significant latency between the applications and the NetBox server, so I'm very happy that every API call is justified.

I guess you expected pynetbox to behave more like ORMs, or something along those lines, where the middleware is interfacing directly with the database. That's not the case with pynetbox.

jwbensley commented 1 year ago

Thanks for the explanation but I feel like you're saying two different things, which is confusing me.

Pynetbox just sends that update request to NetBox and the object in NetBox is updated. Now, if that change is wanted to be fully reflected to the original object, at least one additional API call is needed to populate the object attributes.

I think this is your explenation:

  1. I execute a GET request and pynetbox GET's some data from NetBox and updates the local object based on the data received in the GET request.
  2. I execute .update() and pynetbox sends the update to NetBox.
  3. NetBox is updated but until I run .full_details() my local object is now out of sync with NetBox.

This is clear.

But, as I am seeing in my examples above; if all that happens when I call .update() is that my updates are sent to NetBox, my local object would not have been mangled. It would still be intact, but with outdated data.

I guess you expected pynetbox to behave more like ORMs, or something along those lines, where the middleware is interfacing directly with the database. That's not the case with pynetbox.

No. I'm expecting pynetbox to send the data to NetBox and prefix.tenant.id still to point to the old tenant ID. But what is actually happening when I call .update() is more than what you described Pynetbox just sends that update request to NetBox and the object in NetBox is updated. - actually pynetbox updates the local object with my update data, and then sends a request to NetBox based on the state of the local object.

Is this understanding correct?

If yes, then I think we can close this issue.

markkuleinio commented 1 year ago

Correct, I failed to say that .update() internally actually modifies the object attributes and then calls .save(). It's the .save() operation that triggers the API call. (I attempted to use the word "fully" to mean that the updated attributes are saved in the object as assigned, yes, but nothing else is modified in the object.)

.update(): https://github.com/netbox-community/pynetbox/blob/0f1e588a367e6b7f375ce0c863c1cf784ac462cc/pynetbox/core/response.py#L563-L565

jwbensley commented 1 year ago

Thank you @markkuleinio - everything is now crystal clear! :pray: