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

Events table: migration relationships #388

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jsnshrmn
Copy link
Member

//: # (Thank you for uploading a PR to Wikilinks (externallinks)!)

Description

captures work done to shift many-to-many relationship from:
Events <> URLPatterns
to
URLPatterns <> Collections

Rationale

The current many to many relationships means that a event to url pattern mapping table is kept which is by necessity even larger than the events table itself. This is pretty ineffecient!

Phabricator Ticket

https://phabricator.wikimedia.org/T370901

How Has This Been Tested?

Screenshots of your changes (if appropriate):

Types of changes

What types of changes does your code introduce? Add an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

@katydidnot katydidnot changed the title WIP Events table: migration relationships Events table: migration relationships Sep 20, 2024
Copy link
Member Author

@jsnshrmn jsnshrmn left a comment

Choose a reason for hiding this comment

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

This is looking good! I do have a request to clean up the commit history (let me know if you need a hand); I'm currently testing this against production data locally, which is going to take a few hours because of the data migration.

nginx.conf Show resolved Hide resolved
@@ -45,6 +49,10 @@ class Meta:
on_delete=models.SET_NULL,
related_name="url",
)
Copy link
Member Author

Choose a reason for hiding this comment

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

bike shedding on code reuse for the hashtags tool:

If we wanted to move the link_events relationship here in preparation for making the events model reusable, we could define a field here with a one to many instead of a many to one (foreign key) in LinkEvents
https://docs.djangoproject.com/en/5.1/ref/models/fields/#django.db.models.Field.one_to_many

That's not particularly nice though, IMO, so maybe we could just create an mediawiki-events app that provides an abstract model class and a bunch of helpers for dealing with the server side events and provide status pages.

Copy link
Contributor

@katydidnot katydidnot Sep 25, 2024

Choose a reason for hiding this comment

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

As far as I can tell, after looking at that section in the docs, it looks like those are attributes on an instance of a field rather than a field type. I don't see any way to define a field as a one to many relationship outside of declaring a foreign key which makes sense from a relational db perspective. In that case you would store the foreign key on the many(Linkevents) not the one (URLPatttern) row. A list of all available field options is here

Copy link
Member Author

Choose a reason for hiding this comment

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

This would be used in a custom field class. But I may be overcomplicating things. A generic relationship might suffice. I'll try to whip up a quick example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants