Skip to content

All foreign keys to ContentType should use robust filtering for limit_choices_to #3892

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

Closed
jeremystretch opened this issue Jan 10, 2020 · 1 comment
Assignees
Labels
status: accepted This issue has been accepted for implementation type: housekeeping Changes to the application which do not directly impact the end user

Comments

@jeremystretch
Copy link
Member

Proposed Changes

Ensure that all model fields equating to ForeignKey(to=ContentType) where limit_choices_to is in use employ robust filtering by both app and model name.

Justification

There are several models in NetBox which have one or more ForeignKey fields to ContentType and utilize limit_choices_to to constrain the available choices. In all instances where the set of ContentTypes is constrained, a concise list of (app, model) pairings should be used. For example, the Cable model currently has:

CABLE_TERMINATION_TYPES = [
    'consoleport',
    'consoleserverport',
    'interface',
    ...
]

class Cable(...):
    termination_a_type = models.ForeignKey(
        to=ContentType,
        limit_choices_to={'model__in': CABLE_TERMINATION_TYPES},
        on_delete=models.PROTECT,
        related_name='+'
    )

This is problematic, because it allows for a potential name collision across apps within NetBox. Although not currently an issue, we would do well to address this before it presents a problem.

The above instance should be changed to use the model_names_to_filter_dict() and an explicit list of (app, model) pairings to filter the field's associated queryset:

CABLE_TERMINATION_MODELS = (
    'dcim.consoleport',
    'dcim.consoleserverport',
    'dcim.interface',
    ...
)

def get_cable_termination_models():
    return model_names_to_filter_dict(CABLE_TERMINATION_MODELS)

class Cable(...):
    termination_a_type = models.ForeignKey(
        to=ContentType,
        limit_choices_to=get_cable_termination_models(),
        on_delete=models.PROTECT,
        related_name='+'
    )

This change also entails extending the model_names_to_filter_dict() function to match on both app_label and model; it currently matches only on model. A set of Q objects can be used to achieve this.

@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation type: housekeeping Changes to the application which do not directly impact the end user labels Jan 10, 2020
@jeremystretch jeremystretch self-assigned this Jan 15, 2020
@jeremystretch
Copy link
Member Author

I ended up ditching the <app>.<model> format altogether, since these constants ultimately are used to filter ContentType querysets anyway. I have replaced each list of strings with a static Q() object defining the specific query filter to be used.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: accepted This issue has been accepted for implementation type: housekeeping Changes to the application which do not directly impact the end user
Projects
None yet
Development

No branches or pull requests

1 participant