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

Apply Custom Validators without the need of a DB lookup for ContentType #3633

Merged

Conversation

bryanculver
Copy link
Member

@bryanculver bryanculver commented Apr 21, 2023

Closes: #DNE

What's Changed

Observing the need for this: https://github.com/nautobot/nautobot/pull/3614/files#diff-cca2a364f59877c49898f8d14d311f7567adfb80b5aec6fa51748be4f79ef9b5R44-R46

This might be one of the weird DB calls we see on startup pre-fork in Celery 🤷

TODO

  • Explanation of Change(s)
  • Added change log fragment(s) (for more information see the documentation)
  • Attached Screenshots, Payload Example
  • Unit, Integration Tests
  • Documentation Updates (when adding/changing features)
  • Example Plugin Updates (when adding/changing features)
  • Outline Remaining Work, Constraints from Design

Comment on lines 43 to 46
for app_label, models in FeatureQuery("custom_validators").get_dict():
for model in models:
model_class = apps.get_model(app_label=app_label, model_name=model)
model_class.clean = custom_validator_clean(model_class.clean)
Copy link
Member Author

Choose a reason for hiding this comment

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

I assume in this case we don't need the ContentType for some FK lookup in the DB but we just need to wrap the class' clean method, probably better to go straight to the Django for that. Cut out the middle man I think.

@itdependsnetworks
Copy link
Contributor

Brilliant!

model_class.clean = custom_validator_clean(model_class.clean)
for app_label, models in FeatureQuery("custom_validators").get_dict():
for model in models:
model_class = apps.get_model(app_label=app_label, model_name=model)
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly might need/want the wrap_model_clean_methods function to take an apps parameter (defaulting to the global apps) so that for example we could call this function during a migration or a nautobot_database_ready signal handler where apps is a different context? Not sure, just thinking aloud.

Copy link
Member Author

Choose a reason for hiding this comment

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

@glennmatthews your thought is so if a custom validator is available we can use it in migrations now? Because I assume that doesn't work at present?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, seems pretty clear it wouldn't work at present. We probably don't want them to apply during migrations in general (that would be troublesome) but I feel like there might be a use case for having them apply during the database-ready callback signals? As I said, not sure. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Required for this PR or future housekeeping/feature?

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely not required.

query |= Q(app_label=app_label, model__in=models)

return query

def get_dict(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe items or as_dict instead? Not blocking.

@bryanculver bryanculver merged commit 9761249 into develop Apr 21, 2023
@bryanculver bryanculver deleted the u/bryanculver-wrap-custom-validators-without-query branch April 21, 2023 16:35
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.

None yet

3 participants