[api] RESTful api for Subnet and IpAddress #30

anurag-ks · 2018-07-06T15:34:53Z

Implements and closes #10

This is a WIP PR, I wanted to confirm all the endpoints that we would be implementing in django-ipam.

coveralls · 2018-07-06T15:37:23Z

Pull Request Test Coverage Report for Build 98

66 of 66 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 87:	0.0%
Covered Lines:	273
Relevant Lines:	273

💛 - Coveralls

nemesifier

@anurag-ks where are the tests and the reusable views?

anurag-ks · 2018-07-09T11:31:28Z

@nemesisdesign I first wanted to confirm these endpoints are okay or not and then move on to writing tests for these views.

nemesifier · 2018-07-09T11:41:30Z

@anurag-ks you have to implement CRUD operations as described in http://openwisp.org/gsoc/ideas-2018.html

nemesifier

Good progress. See below.

could you avoid _create_subnet(dict(subnet="10.0.0.0/24")) in favour of _create_subnet(subnet="10.0.0.0/24")? See how is done here: https://github.com/openwisp/openwisp-controller/blob/connections/openwisp_controller/connection/tests/base.py#L18-L30
please add tests for the sample app as well to ensure the generic views work with custom models as well.
please add a test for the case in which a user is not logged-in
see my inline comments as well

nemesifier · 2018-07-10T08:45:51Z

django_ipam/tests/base/test_api.py

+    def test_read_subnet_api(self):
+        subnet_id = self._create_subnet(dict(subnet="fdb6:21b:a477::9f7/64")).id
+        response = self.client.get(reverse('ipam:subnet', args=(subnet_id,)))
+        self.assertEqual(response.status_code, 200)


perform a basic check of the contents also please

nemesifier · 2018-07-10T08:46:25Z

django_ipam/tests/base/test_api.py

+        subnet_id = self._create_subnet(dict(subnet="10.0.0.0/32")).id
+        response = self.client.delete(reverse('ipam:subnet', args=(subnet_id,)))
+        self.assertEqual(response.status_code, 204)
+        self.assertEqual(len(Subnet.objects.all()), 0)


use Subnet.objects.count() which is faster

nemesifier · 2018-07-10T08:47:22Z

django_ipam/tests/base/test_api.py

+                                    data=json.dumps(data),
+                                    content_type='application/json')
+        self.assertEqual(response.status_code, 201)
+        self.assertEqual(str(IpAddress.objects.first()), '10.0.0.2')


access the ip_address attribute because str() representation may be changed and would break this test

nemesifier · 2018-07-10T08:47:35Z

django_ipam/tests/base/test_api.py

+        subnet = self._create_subnet(dict(subnet="10.0.0.0/24"))
+        ip_address = self._create_ipaddress(dict(ip_address="10.0.0.1", subnet=subnet))
+        response = self.client.get(reverse('ipam:ip_address', args=(ip_address.id,)))
+        self.assertEqual(response.status_code, 200)


perform a basic check of the contents please

nemesifier · 2018-07-10T08:47:58Z

django_ipam/tests/base/test_api.py

+        ip_address = self._create_ipaddress(dict(ip_address="10.0.0.1", subnet=subnet))
+        response = self.client.delete(reverse('ipam:ip_address', args=(ip_address.id,)))
+        self.assertEqual(response.status_code, 204)
+        self.assertEqual(len(IpAddress.objects.all()), 0)


use count as indicated in a previous comment

nemesifier · 2018-07-10T08:50:34Z

django_ipam/tests/base/test_api.py

+        self._create_ipaddress(dict(ip_address="10.0.0.1", subnet=subnet))
+        self._create_ipaddress(dict(ip_address="10.0.0.2", subnet=subnet))
+        response = self.client.get(reverse('ipam:list_create_ip_address', args=(subnet.id,)))
+        self.assertEqual(response.status_code, 200)


please perform a basic check of the contents too

nemesifier

some minor improvements to the tests are needed, see see my comments

nemesifier · 2018-07-10T15:15:12Z

django_ipam/api/serializers.py

@@ -13,4 +14,10 @@ class Meta:
 class IpAddressSerializer(serializers.ModelSerializer):
    class Meta:
        model = IpAddress
-        fields = ('ip_address', 'subnet', 'description')
+        fields = '__all__'


I think you should make some fields readonly like created and modified

nemesifier · 2018-07-10T15:15:17Z

django_ipam/api/serializers.py

+class SubnetSerializer(serializers.ModelSerializer):
+    class Meta:
+        model = Subnet
+        fields = '__all__'


I think you should make some fields readonly like created and modified

nemesifier · 2018-07-10T15:16:53Z

django_ipam/tests/base/test_api.py

+        subnet_id = self._create_subnet(subnet="fdb6:21b:a477::9f7/64").id
+        response = self.client.get(reverse('ipam:subnet', args=(subnet_id,)))
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, str(subnet_id))


this is not good enough, you should check one of the attributes of the JSON response in response.data.

nemesifier · 2018-07-10T15:17:07Z

django_ipam/tests/base/test_api.py

+        ip_address = self._create_ipaddress(ip_address="10.0.0.1", subnet=subnet)
+        response = self.client.get(reverse('ipam:ip_address', args=(ip_address.id,)))
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, '10.0.0.1')


nemesifier · 2018-07-10T15:17:53Z

django_ipam/tests/base/test_api.py

+    def test_update_subnet_api(self):
+        data = {
+            'description': 'Test Subnet'
+        }


write this as one line

nemesifier · 2018-07-10T15:18:15Z

django_ipam/tests/base/test_api.py

+        response = self.client.get(reverse('ipam:list_create_ip_address', args=(subnet.id,)))
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, '10.0.0.1')
+        self.assertContains(response, '10.0.0.2')


nemesifier · 2018-07-10T15:18:55Z

django_ipam/tests/base/test_api.py

+            'description': 'Test Subnet'
+        }
+        response = self.client.post(reverse('ipam:subnet_list_create'),
+                                    data=json.dumps(data),


simply use data=json.dumps({}), and remove the definition of data

nemesifier

Looks good, only docs are missing:

explain how to use new api methods
remember to explain how to authenticate
explain how to extend the views, take inspiration from the docs of django-netjsongraph

nemesifier

Some improvements needed, see below.

nemesifier · 2018-07-11T09:56:29Z

README.rst

+
+.. code-block:: text
+
+    <api-url> -H 'Cookie: csrftoken=<csrftoken>; sessionid=<session-id>'


no man, this is not good. the API must work without the CSRF mechanism, this is not a webpage.

Ensure the API works without the CSRF token

nemesifier · 2018-07-11T09:57:23Z

README.rst

+An api enpoint to retrieve or create IP addresses under a specific subnet
+
+GET
++++


should be +++

nemesifier · 2018-07-11T09:57:37Z

README.rst

@@ -168,6 +178,176 @@ Response
        "description": "optional description"
    }

+IpAddress-Subnet List and Create View
+#####################################
+An api enpoint to retrieve or create IP addresses under a specific subnet


always leave a blank line after the title

nemesifier · 2018-07-11T09:57:42Z

README.rst

+
+GET
++++
+Returns the list of IP addresses under a particular subnet.


always leave a blank line after the title

nemesifier · 2018-07-11T09:57:46Z

README.rst

+
+POST
++++
+Create an Ip Address record


always leave a blank line after the title

nemesifier · 2018-07-11T10:00:07Z

README.rst

+
+DELETE
++++++
+Delete a subnet instance


Leave blank line after title, add final dot, double quote Subnet (model class name).

nemesifier · 2018-07-11T10:00:13Z

README.rst

+
+PUT
+++
+Update details of a subnet instance


nemesifier · 2018-07-11T10:00:20Z

README.rst

+
+GET
+++
+Get details of an IP address instance


nemesifier · 2018-07-11T10:00:25Z

README.rst

+
+DELETE
++++++
+Delete an IP address instance


nemesifier · 2018-07-11T10:00:29Z

README.rst

+
+PUT
+++
+Update details of an IP address instance


nemesifier

👍

anurag-ks self-assigned this Jul 6, 2018

anurag-ks requested review from nemesifier and hispanico July 6, 2018 15:34

anurag-ks added this to To do in [GSOC-18] openwisp-ipam via automation Jul 6, 2018

anurag-ks force-pushed the api branch from 634524d to c1ca237 Compare July 6, 2018 19:26

nemesifier suggested changes Jul 9, 2018

View reviewed changes

nemesifier moved this from To do to In progress in [GSOC-18] openwisp-ipam Jul 9, 2018

anurag-ks force-pushed the api branch 3 times, most recently from 7a51b50 to c82473a Compare July 10, 2018 08:40

nemesifier suggested changes Jul 10, 2018

View reviewed changes

anurag-ks force-pushed the api branch from c82473a to 6fc4d49 Compare July 10, 2018 13:09

nemesifier suggested changes Jul 10, 2018

View reviewed changes

anurag-ks force-pushed the api branch 2 times, most recently from c680bd7 to b742fd5 Compare July 10, 2018 16:06

nemesifier reviewed Jul 10, 2018

View reviewed changes

anurag-ks force-pushed the api branch from b742fd5 to ee9c75a Compare July 10, 2018 18:23

nemesifier suggested changes Jul 11, 2018

View reviewed changes

anurag-ks force-pushed the api branch from ee9c75a to c54b1a2 Compare July 11, 2018 13:09

[api] RESTful api for Subnet and IpAddress

0ae8ec5

anurag-ks force-pushed the api branch from c54b1a2 to 0ae8ec5 Compare July 11, 2018 13:11

nemesifier approved these changes Jul 11, 2018

View reviewed changes

nemesifier merged commit 021559c into master Jul 11, 2018

[GSOC-18] openwisp-ipam automation moved this from In progress to Done Jul 11, 2018

anurag-ks deleted the api branch July 11, 2018 13:24

anurag-ks restored the api branch July 12, 2018 22:01

anurag-ks deleted the api branch July 12, 2018 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api] RESTful api for Subnet and IpAddress #30

[api] RESTful api for Subnet and IpAddress #30

anurag-ks commented Jul 6, 2018

coveralls commented Jul 6, 2018 •

edited

nemesifier left a comment

anurag-ks commented Jul 9, 2018

nemesifier commented Jul 9, 2018

nemesifier left a comment

nemesifier Jul 10, 2018

nemesifier Jul 10, 2018

nemesifier Jul 10, 2018

nemesifier Jul 10, 2018

nemesifier Jul 10, 2018

nemesifier Jul 10, 2018

nemesifier left a comment

nemesifier Jul 10, 2018

nemesifier Jul 10, 2018

nemesifier Jul 10, 2018

nemesifier Jul 10, 2018

nemesifier Jul 10, 2018

nemesifier Jul 10, 2018

nemesifier Jul 10, 2018

nemesifier left a comment

nemesifier left a comment

nemesifier Jul 11, 2018

nemesifier Jul 11, 2018

nemesifier Jul 11, 2018

nemesifier Jul 11, 2018

nemesifier Jul 11, 2018

nemesifier Jul 11, 2018

nemesifier Jul 11, 2018

nemesifier Jul 11, 2018

nemesifier Jul 11, 2018

nemesifier Jul 11, 2018

nemesifier left a comment


		.. code-block:: text

		<api-url> -H 'Cookie: csrftoken=<csrftoken>; sessionid=<session-id>'

[api] RESTful api for Subnet and IpAddress #30

[api] RESTful api for Subnet and IpAddress #30

Conversation

anurag-ks commented Jul 6, 2018

coveralls commented Jul 6, 2018 • edited

Pull Request Test Coverage Report for Build 98

💛 - Coveralls

nemesifier left a comment

Choose a reason for hiding this comment

anurag-ks commented Jul 9, 2018

nemesifier commented Jul 9, 2018

nemesifier 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

nemesifier 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

nemesifier left a comment

Choose a reason for hiding this comment

nemesifier 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

coveralls commented Jul 6, 2018 •

edited