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

Overhaul the custom fields model #4878

Closed
jeremystretch opened this issue Jul 22, 2020 · 8 comments
Closed

Overhaul the custom fields model #4878

jeremystretch opened this issue Jul 22, 2020 · 8 comments
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
Milestone

Comments

@jeremystretch
Copy link
Member

Proposed Changes

The current approach to storing custom fields employs a CustomField model for each field created by a user, and one instance of a CustomFieldValue per object per field to hold assigned values. This issue proposes replacing the current implementation with a more robust, better-performing solution yet to be identified.

One approach is to employ a new custom_fields JSONField on each model which support custom fields. This field would hold a dictionary mapping field names to their native values. This solution would retain the current CustomField model (possibly with some minor modifications) but ditch CustomFieldValue. There are several benefits to this approach:

  • Custom field data is stored locally with every instance.
  • JSON data can be filtered against directly when forming a QuerySet (e.g. Site.objects.filter(custom_fields__foo='abc')).
  • Custom field values can be set directly on an instance as dictionary keys on its custom_fields attribute.
  • Data serialization is handled transparently.

Django does not currently support writing JSON data directly via a QuerySet, however issue #29112 has been opened to implement this functionality and appears to be gaining traction. It is also something we could potentially implement locally within NetBox prior to its official implementation.

It's also worth noting that with this approach we lose the ability to easily find all values for a custom field across all models. (This can currently be achieved with e.g. CustomFieldValue.objects.filter(field=some_field).) However, I cannot think of a scenario where we've done this or where it would be especially useful.

Further investigation into this proposal is needed.

Justification

The current implementation has several drawbacks:

  • All custom field values are stored in a single database table.
  • The low-level process for assigning a new custom field value is cumbersome and unintuitive. (A new CustomFieldValue instance must be created and associated with the instance.)
  • Custom field values are serialized to string forms for storage, which can be error-prone and complicates ordering and validation.
@jeremystretch jeremystretch added type: housekeeping Changes to the application which do not directly impact the end user status: under review Further discussion is needed to determine this issue's scope and/or implementation labels Jul 22, 2020
@paravoid
Copy link
Contributor

For the little that's worth, I think that makes a ton of sense -- I proposed it back in #3083 (comment), which was declined out of performance considerations, which I think this issue has the potential to address. I had played around with a PoC back then and it seemed feasible and scalable. I can't find the code that I wrote back then, but I doubt it'd be useful after the custom field manager changes anyway…

@shane-davidson
Copy link

Another potential idea for custom field types is the ability to use existing model types as the value.

Examples to be able to create the following custom fields:
Site -> "Primary DNS" -> Links to IP
Site -> "Secondary DNS" -> Links to IP
Tenant -> "Public IP" -> Links to IP
Virtual Machine -> "Failover/DR cluster" -> Links to cluster
Linking virtual machines together - primary/secondary DB etc
Adding a secret to an IP address containing the certificate for it

Although i'm not sure how easy it would be to create/maintain the relations

@jeremystretch jeremystretch added this to the v2.10 milestone Aug 20, 2020
@jeremystretch jeremystretch self-assigned this Aug 25, 2020
@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation labels Sep 1, 2020
@jeremystretch
Copy link
Member Author

I've made great progress on this, however it has drawn attention to a question that I don't think we've ever really considered: What should happen when a custom field is deleted?

Current behavior (v2.9)

  • CustomField instance is deleted
  • All CustomFieldValues related to that CustomField are deleted
  • No changes are recorded against NetBox objects to which those CustomFieldValues were assigned
  • When such an object is next updated, the resulting ObjectChange record will show both the user-triggered change(s) made and the apparent deletion of the custom field data.

Obviously, the delay in reflecting the change is misleading and undesirable. I see a few options for handling custom field deletions under the new approach.

Option A: Prevent the deletion of a CustomField which still has data assigned to one or more objects. This would help mitigate accidental deletions, but also impedes intentional deletion, as it requires that all values be deleted before the field can be removed.

Option B: Allow the deletion of a CustomField but retain all data. This is no doubt the simplest approach, however it results in the retention of data which is likely no longer wanted. Also, if a custom field is deleted and a new one is later created with the same name, instances will immediately report values assigned when the original field was present.

Option C: Delete the custom field and all its values, but do not record change records. This essentially replicates NetBox's current behavior.

Option D: Delete the custom field and all its values, and record a change record for every affected object. IMO this is the most desirable action, however it should be noted that deleting a custom field may introduce a huge number of change records. This may or may not be acceptable given that the deletion of a CustomField is expected to happen very infrequently.

@dgarros
Copy link
Contributor

dgarros commented Sep 16, 2020

I think D would be ideal and as mentioned it shouldn't happen too often.

@itdependsnetworks
Copy link

Agreed on option D

@jeremystretch
Copy link
Member Author

I've got a proof-of-concept working for the stale data cleanup in 2d56a65, using the m2m_changed and pre_delete signals to identify the affected object types. Seems like a reasonable approach, though further testing is needed.

@jeremystretch
Copy link
Member Author

I've merged this work int odevelop-2.10. Although further work and testing may be needed, the implementation itself can be considered complete.

@paravoid
Copy link
Contributor

That's so awesome to see, thanks so much @jeremystretch! 👍

One (hopefully quick) question: I noticed that you've added custom fields to all primary models, and said in 5146 that it's explicitly not for other models at this time. Would it be possible to elaborate on what's missing in terms of functionality/limitations in order for this to be added to other models? Asking so that future contributors (possibly including myself ;)) know what to expect in terms of difficulty :) (The one I care about FWIW at the moment is InventoryItem, to add some tracking fields to items like Juniper routing engines and linecards).

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 17, 2020
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

5 participants