openwisp / django-netjsongraph

Network Topology Visualizer & Network Topology Collector
MIT License
141 stars 64 forks source link

[models] Addresses implementation is not optimal #61 #104

Closed ghost closed 4 years ago

ghost commented 4 years ago

Implemented m2m fields. Closes #61

ghost commented 4 years ago

Try importing this network topology (use FETCH strategy, then tell the application to update it either via the admin or the management command): https://openwisp.nnxx.ninux.org/api/topology/36d5ba3e-1364-49d2-9e29-1f6dceb7cbbb/?format=json

The visualization will fail.

I have imported this before trying the patch. After applying the patch I cannot run the fetch operation anymore as well.

I ran the tests and it worked for me- Screenshot_5

atb00ker commented 4 years ago

@Weirdo914 try this:

  1. Delete database
  2. Checkout to master
  3. Create this topology: https://openwisp.nnxx.ninux.org/api/topology/36d5ba3e-1364-49d2-9e29-1f6dceb7cbbb/?format=json
  4. Go back to your branch
  5. Make migrations
  6. Look at the topology, it'll have no links.
  7. Update topology (using admin or management command.)
  8. Look at the topology graph again!
ghost commented 4 years ago

Looks like build is failing because of https://github.com/openwisp/django-netjsongraph/commit/a4bd9c69bf766c7782f48a262a5b379069d19783

ghost commented 4 years ago

@Weirdo914 try this:

  1. Delete database
  2. Checkout to master
  3. Create this topology: https://openwisp.nnxx.ninux.org/api/topology/36d5ba3e-1364-49d2-9e29-1f6dceb7cbbb/?format=json
  4. Go back to your branch
  5. Make migrations
  6. Look at the topology, it'll have no links.
  7. Update topology (using admin or management command.)
  8. Look at the topology graph again!

I made some migrations for this.

ghost commented 4 years ago

@nemesisdesign That is strange because I looked at it before but when I tried it only showed 5 queries for me. Screenshot_6

nemesifier commented 4 years ago

You can write a test for it to be 100% sure: https://docs.djangoproject.com/en/dev/topics/testing/tools/#django.test.TransactionTestCase.assertNumQueries

ghost commented 4 years ago

I tried testing it and it actually made 47 sql queries so I used JsonField instead, like you earlier suggested.