Skip to content

20089 use get_queryset function for valid_models #20090

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

Merged
merged 1 commit into from
Aug 14, 2025

Conversation

arthanson
Copy link
Collaborator

Fixes: #20089

@arthanson arthanson marked this pull request as ready for review August 13, 2025 22:50
@arthanson arthanson requested review from a team and jeremystretch and removed request for a team August 13, 2025 22:50
Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

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

Rather than changing the views, can we address the root issue in the valid_models() method? Seems like it would be possible by removing the forced resolution of the ContentType queryset.

@@ -9,7 +9,7 @@
router.register('data-sources', views.DataSourceViewSet)
router.register('data-files', views.DataFileViewSet)
router.register('jobs', views.JobViewSet)
router.register('object-changes', views.ObjectChangeViewSet)
router.register('object-changes', views.ObjectChangeViewSet, basename='objectchange')
Copy link
Member

Choose a reason for hiding this comment

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

Why set basename?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When you remove queryset (change to function) from API view it throws an error unless basename is set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I looked at trying to move to Q objects, like the following, however the issue is get_models returns a dict and going like this still causes the issue where the content types are only resolved at instantiation. As discussed an alternative solution might be to put it into the housekeeping task or such but that would be a breaking change:

    def valid_models(self):
        # Exclude any change records which refer to an instance of a model that's no longer installed. This
        # can happen when a plugin is removed but its data remains in the database, for example.
        from django.db.models import Q
        
        # Get all currently registered models at query time
        current_models = apps.get_models()
        
        # Build a Q object that matches any ContentType that corresponds to a currently registered model
        valid_content_type_conditions = Q()
        for model in current_models:
            valid_content_type_conditions |= Q(
                changed_object_type__app_label=model._meta.app_label,
                changed_object_type__model=model._meta.model_name
            )
        
        return self.filter(valid_content_type_conditions)

@jeremystretch jeremystretch merged commit efcf9e5 into feature Aug 14, 2025
10 checks passed
@jeremystretch jeremystretch deleted the 20089-valid-models branch August 14, 2025 18:27
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