Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 89 additions & 2 deletions pynetbox/models/dcim.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,56 @@
See the License for the specific language governing permissions and
limitations under the License.
"""
from six.moves.urllib.parse import urlsplit

from pynetbox.core.query import Request
from pynetbox.core.response import Record, JsonField
from pynetbox.core.endpoint import RODetailEndpoint
from pynetbox.models.ipam import IpAddresses
from pynetbox.models.circuits import Circuits


class TraceableRecord(Record):
Comment thread
raddessi marked this conversation as resolved.
@property
def trace(self):
req = Request(
key=str(self.id) + "/trace" if not self.url else None,
base=self.endpoint.url,
token=self.api.token,
session_key=self.api.session_key,
http_session=self.api.http_session,
)
ret = []
for (termination_a_data, cable_data, termination_b_data) in req.get():
this_hop_ret = []
for hop_item_data in (termination_a_data, cable_data, termination_b_data):
# if not fully terminated then some items will be None
if not hop_item_data:
this_hop_ret.append(hop_item_data)
continue

url_path = urlsplit(hop_item_data["url"]).path
if url_path.startswith("/api/dcim/cables"):
return_obj_class = Cables
elif url_path.startswith("/api/dcim/front-ports"):
return_obj_class = FrontPorts
elif url_path.startswith("/api/dcim/interfaces"):
return_obj_class = Interfaces
elif url_path.startswith("/api/dcim/rear-ports"):
return_obj_class = RearPorts
else:
raise NotImplementedError(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this how you would want to handle this situation?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably make a dict here that mapped the endpoint name it might encounter with the custom object instead of an if/else tree (really wish switches were a thing in python). e.g.

{
    "cables": Cables,
    "front-ports": FrontPorts,
...
}

You'll probably want to add some other endpoint/objects you might come across in the traces as well like ConsolePort/ConsoleServerPort and PowerPort/Outlets. I'd probably default to just a simple Record object at the end of it instead of raising.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect. I'll update to a map and return a Record instead of raising. It's been a while.. I think I verified all possible objects returned are listed here but I'll check once more, good call.

"unable to unpack item data from endpoint '{}'".format(url_path)
)
this_hop_ret.append(
return_obj_class(hop_item_data, self.endpoint.api, self.endpoint)
Comment thread
raddessi marked this conversation as resolved.
)

ret.append(this_hop_ret)

return ret


class DeviceTypes(Record):
def __str__(self):
return self.model
Expand Down Expand Up @@ -80,11 +124,27 @@ class ConnectedEndpoint(Record):
device = Devices


class Interfaces(Record):
class Interfaces(TraceableRecord):
interface_connection = InterfaceConnection
connected_endpoint = ConnectedEndpoint


class PowerOutlets(TraceableRecord):
device = Devices


class PowerPorts(TraceableRecord):
device = Devices


class ConsolePorts(TraceableRecord):
device = Devices


class ConsoleServerPorts(TraceableRecord):
device = Devices


class RackReservations(Record):
def __str__(self):
return self.description
Expand All @@ -99,6 +159,14 @@ class RUs(Record):
device = Devices


class FrontPorts(Record):
device = Devices


class RearPorts(Record):
device = Devices


class Racks(Record):
@property
def units(self):
Expand Down Expand Up @@ -154,7 +222,26 @@ def __str__(self):

class Cables(Record):
def __str__(self):
return "{} <> {}".format(self.termination_a, self.termination_b)
# populate the terminations to get the full names if they are not already
try:
termination_a_name = self.termination_a.name
except AttributeError:
try:
self.termination_a.full_details()
except TypeError:
self.termination_a.full_details(self)
termination_a_name = getattr(self.termination_a, "name", self.termination_a)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like this logic but I'm not sure of a better method. Ideas welcomed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the goal here? Both of those should be Termination objects which return .name (assuming they're not a circuit) anyways. I suspect this might be the source of the excessive calls to NetBox in your tests.

@raddessi raddessi Dec 1, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a change to force each of the returned items along the trace to get fully populated so that when you printed their name in the trace it wasn't just displayed as a Record item with no data. Once each item was accessed they would then print their name properly, but having a trace full of unlabeled hops was not very useful. It's not very clear, and I don't like it at all. I think it may be better to do something like this in the trace function itself (I think maybe you mentioned that before?) so that only on a trace will it try to prefetch the data of each item in the hops so they print out nicely.

EDIT: When I said It's not very clear, and I don't like it at all. I was referring to my first attempt to patch this :)

@zachmoody zachmoody Dec 2, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's weird. Do you have an example you can show of the old behavior? Those should be Termination objects, but either way even Record's default __str__ behavior is to return .name, iirc.

@raddessi raddessi Dec 3, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, with this section of code reverted this is what I'm getting now:

In [1]: iface.trace()                                                                                                             
Out[1]: 
[[em1,
  <class 'pynetbox.models.dcim.Termination'> <> <class 'pynetbox.models.dcim.Termination'>,
  pair-11 (ports 21-22)],
 [port-2,
  <class 'pynetbox.models.dcim.Termination'> <> <class 'pynetbox.models.dcim.Termination'>,
  port-2],
 [pair-11 (ports 21-22),
  <class 'pynetbox.models.dcim.Termination'> <> <class 'pynetbox.models.dcim.Termination'>,
  Ethernet11]]

So it looks like maybe I was mistaken or confused about only the middle (not the first or last) having the issue, it may be all of the items returned from the trace until they are accessed in some way. You can see here how the info is filled out once the item is accessed directly:

In [2]: trace = iface.trace()                                                                                                     

In [3]: trace                                                                                                                     
Out[3]: 
[[em1,
  <class 'pynetbox.models.dcim.Termination'> <> <class 'pynetbox.models.dcim.Termination'>,
  pair-11 (ports 21-22)],
 [port-2,
  <class 'pynetbox.models.dcim.Termination'> <> <class 'pynetbox.models.dcim.Termination'>,
  port-2],
 [pair-11 (ports 21-22),
  <class 'pynetbox.models.dcim.Termination'> <> <class 'pynetbox.models.dcim.Termination'>,
  Ethernet11]]

In [4]: trace[0][1]                                                                                                               
Out[4]: em1 <> pair-11 (ports 21-22)

In [5]: trace                                                                                                                     
Out[5]: 
[[em1, em1 <> pair-11 (ports 21-22), pair-11 (ports 21-22)],
 [port-2,
  <class 'pynetbox.models.dcim.Termination'> <> <class 'pynetbox.models.dcim.Termination'>,
  port-2],
 [pair-11 (ports 21-22),
  <class 'pynetbox.models.dcim.Termination'> <> <class 'pynetbox.models.dcim.Termination'>,
  Ethernet11]]

You are correct, they are Termination objects. I'm sorry it's been a while since I worked on this so I've forgotten a little of the details. It seems like none of the cable connection object information at all is set until the item is accessed directly for some reason though. I have not dug in to that much yet.

EDIT: Actually to be more clear, it seems like the Termination objects referenced by the Cable connection object are not being loaded until the Cable object is itself referenced directly. The Cable object is always printing in the correct format, just the Termination objects inside of it have no data in them yet.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By moving this to inside the trace function it is muuuuch clearer now.


try:
termination_b_name = self.termination_b.name
except AttributeError:
try:
self.termination_b.full_details()
except TypeError:
self.termination_b.full_details(self)
termination_b_name = getattr(self.termination_b, "name", self.termination_b)

return "{} <> {}".format(termination_a_name, termination_b_name)

termination_a = Termination
termination_b = Termination
38 changes: 29 additions & 9 deletions tests/test_dcim.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
from .util import Response

if six.PY3:
from unittest.mock import patch
from unittest.mock import patch, call
else:
from mock import patch
from mock import patch, call


api = pynetbox.api("http://localhost:8000", token="abc123",)
Expand Down Expand Up @@ -526,11 +526,31 @@ def test_get_circuit(self):
self.assertTrue(isinstance(ret, self.ret))
self.assertTrue(isinstance(str(ret), str))
self.assertTrue(isinstance(dict(ret), dict))
mock.assert_called_with(
"http://localhost:8000/api/{}/{}/1/".format(
self.app, self.name.replace("_", "-")
),
headers=HEADERS,
params={},
json=None,
mock.assert_has_calls(
[
call(
"http://localhost:8000/api/{}/{}/1/".format(
self.app, self.name.replace("_", "-")
),
headers=HEADERS,
params={},
json=None,
),
call(
"http://localhost:8000/api/{}/{}/1/".format(
"circuits", "circuit-terminations"
),
headers=HEADERS,
params={},
json=None,
),
call(
"http://localhost:8000/api/{}/{}/1/".format(
"circuits", "circuit-terminations"
),
headers=HEADERS,
params={},
json=None,
),
]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't love the idea that each call for a cable will now result in 3 calls.. I didn't realize when fixing this test that that is essentially what changed in this test. It doesn't feel right that this needs to happen for each cable returned.. I want to dig in to this because I think this should only be needed for the cables between either the first or last cable returned in a trace since those are the only ones I have ever seen that exhibited the problem of their names not being initialized until you actually interact with the cable object (then it calling it's full_details method) so it seems wrong that this would be called 3 times for this simple query. I'll dig later when I have time.

Alternatively.. we could move this logic to the trace property and have it done there only for cables returned from traces but that feels less consistent somehow. I'm on the fence.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, have a look at the previous comment, the __str__() method might have something to do with it. Either way, we'd definitely want some tests that are specifically testing .trace.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I'll also add testing with the next round of changes.

)