Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change IX->IXLan to a OneToOneField? #55

Open
paravoid opened this issue Sep 25, 2020 · 9 comments
Open

Change IX->IXLan to a OneToOneField? #55

paravoid opened this issue Sep 25, 2020 · 9 comments
Assignees
Labels
Minor Up to 4 hours Requires Notice Implementing this ticket requires user be notified

Comments

@paravoid
Copy link
Contributor

Currently IXLan seems to have a ForeignKey to InternetExchange, which creates a forward relationship of e.g. print(ixlan.ix.name) and the reverse relationship of e.g. print(ix.ixlan_set.all()[0].mtu).

However, no matter how hard I looked at the data, I could not find a single instance where an InternetExchange has multiple IXLans. Even in the case of LINX for example, LON1 and LON2 are represented as separate InternetExchange instances, and even LON1 Multicast is a separate InternetExchange instance. To put this in a database query terms:

InternetExchange.objects.annotate(num_ixl=Count("ixlan_set")).exclude(num_ixl=1).count()  # 0

So I suppose I'm not exactly sure why this is a separate model, and if it's for database cleanliness, why is it not using a OneToOneField that would allow much easier accessors (ix.lan.mtu).

Would there be appetite for changing this? If so, I wonder how I'd go about it, as the disappearance of the ixlan_set would break the API. Depending on the answer, I may explore some custom solution to reinstate it, perhaps emitting a DeprecationNotice in the process or something.

@arnoldnipper arnoldnipper self-assigned this Sep 25, 2020
@arnoldnipper
Copy link

A while ago we made ixlan_id == ix_id. In v3 we will get rid of object ixlan. Until then we have to live with it

@paravoid
Copy link
Contributor Author

Thanks for the super quick response! That's great to hear & seems we are on the same page.

I'd argue like the issue should stay open but with a milestone:3.0 or something, as I fear it'll get forgotten over time, but YMMV...

@arnoldnipper
Copy link

see peeringdb/peeringdb#625

@paravoid
Copy link
Contributor Author

Ah, thank you! I may be slightly confused, however: this seems to describe HTTP API and schema changes.

I was referring to a Django-internal change -- changing from ForeignKey -> OneToOneField only changes the accessors for the reverse relationship. It has the implication of course that there is a 1:1 mapping (or, in other words, that the foreign key is unique), but that's about the only database change involved here. From what I understand from the above, it sounds like unique constraints won't be a problem.

It would change the Django API as consumed by users of the reusable app, however.

@paravoid
Copy link
Contributor Author

Does that ^^ make sense? Sorry for bumping this but given the issue is resolved I fear we'll forget :)

@grizz
Copy link
Member

grizz commented Nov 6, 2020

Arnold is correct, it's leftover from the initial v2 schema. I think this is a good idea and can be done now.

@grizz grizz reopened this Nov 6, 2020
@grizz grizz added the Minor Up to 4 hours label Nov 6, 2020
@grizz
Copy link
Member

grizz commented Nov 6, 2020

@peeringdb/pc please vote

@grizz grizz assigned grizz and unassigned arnoldnipper Nov 6, 2020
@grizz grizz added this to the 1 Decide milestone Nov 6, 2020
@mcmanuss8
Copy link

+1 let's clean this up

@grizz grizz added the Requires Notice Implementing this ticket requires user be notified label Nov 20, 2020
@egfrank
Copy link
Contributor

egfrank commented Nov 20, 2020

Summary

  • Change Ixlan on Ix from ForeignKey -> OneToOneField

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Minor Up to 4 hours Requires Notice Implementing this ticket requires user be notified
Projects
None yet
Development

No branches or pull requests

5 participants