Interface Tree traversal #271

rmhasan · 2017-04-29T07:21:26Z

Added following methods in InterfaceViewSet which can be invoked by get requests to return view sets

parent
ancestors
children
descendants
siblings
root

Also implemented following methods in Interface model class in models.py which get invoked in the view methods

get_ancestors
get_children
get_descendants
get_root
get_siblings

Also added tests in tests/api_tests/test_interfaces.py and tests/model_tests/test_interfaces.py

Implemented all tree traversal methods like ancestors, parent, root, children, descendents etc.

jathanism · 2017-05-04T22:36:53Z

Oh awesome. Sorry I've been behind the curve. I'll check this out. Thank you!!

jathanism · 2017-05-11T15:36:21Z

I'm going to review this damn thing eventually. I need a clone.

jathanism

Looking great! Just have a couple minor adjustments.

jathanism · 2017-05-11T17:27:04Z

nsot/api/views.py

+        """Return parent of interface"""
+        interface = self.get_resource_object(pk, site_pk)
+        parent = [] if interface.parent is None else [interface.parent]
+        return self.list(request, queryset=parent, *args, **kwargs)


Parent should return only a single object vs. a list. See NetworkViewSet.parent for a good reference on how it's done for network objects.

jathanism · 2017-05-11T17:29:01Z

nsot/models.py

+
+    def get_siblings(self):
+        """Return the interfaces with same parent as an interface"""
+        return list(Interface.objects.filter(parent=self.parent).exclude(id=self.id))


Siblings should be interfaces with both the same parent AND the same device, otherwise all interfaces with no parent are considered to be siblings, which I don't think is what we really want.

@jathanism Currently you can create a interface on a device and set its parent to an interface on another device. Should I create a validation check that prevents that?

Yeah I guess that is a pretty sane constraint!

Okay I will work on this.

nickpegg · 2017-05-11T18:38:17Z

nsot/models.py

+        root = self.parent
+        while root is not None and root.parent is not None:
+            root = root.parent
+        return [] if root is None else [root]


Similar to @jathanism's comment on parent, this should have the same behavior and return a single object rather than a list.

rmhasan · 2017-05-16T05:05:21Z

Hi @jathanism and @nickpegg, I saw the feed back on #266 and #271. I will work on them soon.

- used self.retrieve to return parent and root of interfaces - changed filter query on siblings to look for interfaces with same parent and device

rmhasan · 2017-05-23T20:13:33Z

Hi @jathanism and @nickpegg I worked on the changes you requested. Sorry it took so long.

jathanism · 2017-05-23T20:15:07Z

@rmhasan There's no need to apologize, you're a hero!

- If someone calls the root view on an Interface object and its parent is None, then it should be returned as the root - Also added tests to check that if sibling is called on an Interface object, then Interface objects with same parent and device are returned

-Added validation check to prevent users from adding interfaces with different device than its parent or add a parent to an existing interface that has a different device than itself -Also added test to check that when a user calls siblings view on an interface object, then only interfaces with same parent and device are returned

rmhasan · 2017-05-24T00:22:49Z

Hi @jathanism and @nickpegg. I made the following changes.

use self.retreive to return parent and root interfaces
change filter query for siblings view to look for interfaces with same parent and device
return Interface whose parent is None as root if root is called on that interface
also added validation check to prevent users from setting an interfaces parent to a interface
with a different device
added test to check when siblings is called, that interfaces with same parent and devices are returned
added test to check when root is called on an interface with no parent that it is returned
added test to check that a user cannot set an interface's parent to an interface with different device

EDIT:
You can also checkout PR dropbox/pynsot#134 to test the new tree traversal methods.

jathanism

Everything looks pretty good overall! I just have some minor asks to wrap this up and a few comments to think about.

jathanism · 2017-05-24T13:56:53Z

nsot/models.py

+            return parent
+        if parent.device_hostname != self.device_hostname:
+            raise exc.ValidationError({
+                'parent': 'Parent\'s device does not match device with host name \"%s\"'%(self.device_hostname)


Sorry to nitpick but can you please change this string to be double-quoted so that we can eliminate the backslashes. It make for better readability. You can also use %r vs. %s to get the repr conversion of the object, which includes quotes.

e.g.

"Parent's device does not match device with host name %r" % self.device_hostname

results in

"Parent's device does not match device with host name 'foo'"

jathanism · 2017-05-24T14:00:18Z

nsot/api/views.py

        return self.list(request, queryset=interface.networks, *args, **kwargs)

    @detail_route(methods=['get'])
+    def parent(self, request, pk=None, site_pk=None, *args, **kwargs):
+        """Return parent of interface"""


Can you please update the docstring to be similar in style to the one at nsot.api.NetworkViewSet.parent()?

And for the remaining methods you've added can you please make sure they all have docstrings?

This is important because the docstrings are used in the automated API documentation on nsot.readthedocs.io and in the browsable API.

jathanism · 2017-05-24T14:06:11Z

nsot/models.py

+    def get_ancestors(self):
+        """Recursively get all parents of an interface"""
+        p = self.parent
+        ancestors = []


Just a comment: Returning a list instead of a queryset may be problematic if we attempt to implement any filtering (or filter chaining) for ancestors. This isn't currently a use-case but it will be something to keep in mind if the need arises in the future.

Hi @jathanism, how about I do the following. I get all the id's of the ancestors in a list like the following

ancestor_ids = [a.id for a in ancestors]

Then I return the Queryset from the following query

Interface.objects.filter(id__in=ancestor_ids)

jathanism · 2017-05-24T14:06:57Z

nsot/models.py

+    def get_descendants(self):
+        """Recursively return all the children of an interface"""
+        s = list(self.get_children())
+        children = []


Just a comment: Similarly to ancestors, returning a list may be problematic if we ever need to filter (or chain filters) on descendant interfaces.

I would use the same fix mentioned in the Interface.get_ancestors comment.

jathanism · 2017-05-24T14:08:03Z

nsot/models.py

+
+    def get_siblings(self):
+        """Return the interfaces with same parent as an interface"""
+        return list(Interface.objects.filter(parent=self.parent, device=self.device).exclude(id=self.id))


Just a comment: Last but not least, casting the queryset to a list may become problematic in the future in the event we need to implement filtering or desire to chain filters on these results.

- Updated docstrings for parent and added docstrings for other view functions - Changing ValidationError message for setting siblings to use a double quoted string - Also returning a queryset from get_siblings instead of a list

rmhasan · 2017-05-26T06:34:10Z

Hi @jathanism I made the following changes

I modified the docstrings of some functions and added docstrings to others
I changed the message in the ValidationError thrown from Interface.clean_parent to be a double quoted string
Instead of returning lists, I'm returning Querysets from Interface.get_descendants, Interface.get_ancestors, and Interface.get_siblings

- Update the docstring of some Interface model functions - Return queryset from get_descendants and get_ancestors in Interface model

jathanism

This is awesome. Thank you! I'm gonna ask @nickpegg to take a look, too!

nickpegg · 2017-05-30T18:19:19Z

Thanks for this contribution! :) I'm merging this and cutting a new version. I'll drop a note on this PR once that version's available on PyPI.

nickpegg · 2017-05-30T18:33:11Z

v.1.1.4 released to PyPI: https://pypi.python.org/pypi/nsot/1.1.4

rmhasan added 2 commits April 29, 2017 03:08

Fix for #260

d6221c5

Implemented all tree traversal methods like ancestors, parent, root, children, descendents etc.

added tests for interface tree traversal methods

1cba178

jathanism suggested changes May 11, 2017

View reviewed changes

nickpegg reviewed May 11, 2017

View reviewed changes

Added changes requested on 05/11/2017

aabe15b

- used self.retrieve to return parent and root of interfaces - changed filter query on siblings to look for interfaces with same parent and device

rmhasan added 2 commits May 23, 2017 16:37

jathanism suggested changes May 24, 2017

View reviewed changes

Changes include following

b9f8c26

- Updated docstrings for parent and added docstrings for other view functions - Changing ValidationError message for setting siblings to use a double quoted string - Also returning a queryset from get_siblings instead of a list

Changes include following

80c60e9

- Update the docstring of some Interface model functions - Return queryset from get_descendants and get_ancestors in Interface model

jathanism approved these changes May 26, 2017

View reviewed changes

jathanism assigned nickpegg May 26, 2017

nickpegg approved these changes May 30, 2017

View reviewed changes

nickpegg merged commit 08a89f4 into dropbox:develop May 30, 2017

nickpegg mentioned this pull request May 30, 2017

Implement tree traversal methods for Interface objects #260

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interface Tree traversal #271

Interface Tree traversal #271

rmhasan commented Apr 29, 2017

jathanism commented May 4, 2017

jathanism commented May 11, 2017

jathanism left a comment

jathanism May 11, 2017

jathanism May 11, 2017

rmhasan May 23, 2017

jathanism May 23, 2017

rmhasan May 23, 2017

nickpegg May 11, 2017

rmhasan commented May 16, 2017

rmhasan commented May 23, 2017

jathanism commented May 23, 2017

rmhasan commented May 24, 2017 •

edited

Loading

jathanism left a comment

jathanism May 24, 2017

jathanism May 24, 2017

jathanism May 24, 2017

rmhasan May 26, 2017

jathanism May 24, 2017

rmhasan May 26, 2017

jathanism May 24, 2017

rmhasan commented May 26, 2017

jathanism left a comment

nickpegg commented May 30, 2017

nickpegg commented May 30, 2017

Interface Tree traversal #271

Interface Tree traversal #271

Conversation

rmhasan commented Apr 29, 2017

jathanism commented May 4, 2017

jathanism commented May 11, 2017

jathanism left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmhasan commented May 16, 2017

rmhasan commented May 23, 2017

jathanism commented May 23, 2017

rmhasan commented May 24, 2017 • edited Loading

jathanism left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmhasan commented May 26, 2017

jathanism left a comment

Choose a reason for hiding this comment

nickpegg commented May 30, 2017

nickpegg commented May 30, 2017

rmhasan commented May 24, 2017 •

edited

Loading