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

Lightweight customization of built-in model validation #5963

Closed
jeremystretch opened this issue Mar 11, 2021 · 10 comments · Fixed by #6576
Closed

Lightweight customization of built-in model validation #5963

jeremystretch opened this issue Mar 11, 2021 · 10 comments · Fixed by #6576
Labels
status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application
Milestone

Comments

@jeremystretch
Copy link
Member

NetBox version

v2.10.6

Feature type

New functionality

Proposed functionality

Provide a mechanism for users to configure simple custom validation rules for fields on built-in models. This could be done by declaring a suite of custom validators to apply to specific model fields within NetBox's configuration. For example, such a configuration might look like this:

VALIDATION_RULES = {
    'dcim.Site': {
        'name': {
            'regex': '^[A-Z]{3}\d{2}$',
        },
        'asn': {
            'min_value': 65000,
        }
    }
}

When a site is saved, NetBox would reference this configuration in addition to its own built-in validators. This specific example would require each site's name to match the specified regular expression, and its AS number (if provided) to be equal to or greater than the specified value.

I can see common use cases for the following validator types:

  • regex: Force matching of a specified regular expression
  • min_value: Minimum numeric value
  • max_value: Maximum numeric value
  • min_length: Minimum length (string)
  • max_length: Maximum length (string)
  • required: Force a normally-optional field to be required (this would not work in the inverse)

This feature is being proposed with the intention of providing a very simple avenue to address very simple requirements. Bearing that in mind, it does not seek to provide conditional validation, which cannot be reasonably expressed in a declarative manner.

Use case

Common examples might include:

  • Forcing device names to follow a particular pattern
  • Requiring a tenant to be assigned for each prefix
  • Limiting the range of AS numbers that can be assigned to sites

Database changes

None, provided the validation rules are defined in the configuration as in the example above.

External dependencies

No response

@jeremystretch jeremystretch added type: feature Introduction of new functionality to the application status: under review Further discussion is needed to determine this issue's scope and/or implementation labels Mar 11, 2021
@jeremystretch jeremystretch changed the title Lightweight customization of built-in model attributes Lightweight customization of built-in model validation Mar 11, 2021
@jeremystretch
Copy link
Member Author

it does not seek to provide conditional validation, which cannot be reasonably expressed in a declarative manner.

After playing with this idea a bit, it might be feasible to check the instance being validated against a dictionary of attributes to determine whether to enforce the custom rule. Further experimentation is needed.

@sdktr
Copy link
Contributor

sdktr commented Mar 12, 2021

Big plus on requiring an optional field! Could this cover assigning default values as well? On both required and optional fields?

@cpmills1975
Copy link
Contributor

Enforcing validation would be nice when input is driven by the UI, but if it could be optional when writing code, that would be awesome. Use case: I've written a custom plugin to get data in the the DB. Some of the info that I would like to enforce for users through the UI may not be available at the time of import using the ORM so having the ability to override this setting in code would be quite nice.

@jeremystretch
Copy link
Member Author

Enforcing validation would be nice when input is driven by the UI, but if it could be optional when writing code, that would be awesome.

That sounds like a bad idea. Data is either valid or invalid, regardless of the means by which it was input. IMO if you try to make validation conditional in that manner you're bound to run into problems.

@cpmills1975
Copy link
Contributor

Enforcing validation would be nice when input is driven by the UI, but if it could be optional when writing code, that would be awesome.

That sounds like a bad idea. Data is either valid or invalid, regardless of the means by which it was input. IMO if you try to make validation conditional in that manner you're bound to run into problems.

I was thinking more about enforcing field presence. Example, my permissions are all based on tenants so it makes sense to enforce that all objects have tenants. However, I also import data using a custom plugin I've written and at that point I may not have allocated a device to a tenant - it's just in the rack for my eyes only.

I guess so long as I could craft granular validation rules, a bit like the permissions constraints, it would be OK - perhaps something like tenant is required if status isn't Staged?

@proudbro
Copy link

Can you please add the possibility to make fieids unique? For example - Serial Number filed in Device tab
Or it is out of scope of this issue?

@jeremystretch jeremystretch added needs milestone Awaiting prioritization for inclusion with a future NetBox release and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation labels Mar 29, 2021
@candlerb
Copy link
Contributor

I'd really like to see validation extensible via python code - even if it's just a lambda that receives a new model and returns None or a set of errors. This would become another extension point like reports, scripts etc.

I believe the validation rules provided above - e.g. enforcing that all device names meet a single static regex - are not particularly useful. With a lambda you could say e.g. "if this device has role X and is in site Y, then its name must match Z", or reference other objects, e.g. "if this device is in site X, all its interface IP addresses must be within site X prefixes" etc

@jeremystretch
Copy link
Member Author

@candlerb I understand your use case, however let's also recognize that requiring users to develop and maintain custom code solely to effect custom validation rules is a pretty high bar, and likely undesirable for a large portion of the user base.

I believe the validation rules provided above - e.g. enforcing that all device names meet a single static regex - are not particularly useful.

Granted, this may not suffice for everyone, but I've worked in many shops where this would be very useful, and I suspect I'm not alone.

@jeremystretch
Copy link
Member Author

I suppose what we could do is introduce a CustomValidator class, and allow users to define a mapping of models to subclasses of it. For example:

CUSTOM_VALIDATORS = {
    'dcim.site': [
        MySiteValidator,
        GlobalFooValidator
    ],
    'ipam.prefix': [
        MyPrefixValidator,
        GlobalFooValidator
    ]
}

A custom validator would just define a validate() method that has access to the model being validated. Example:

class MySiteValidator(CustomValidator):
    def validate(self, instance):
        if len(instance.name) > 50:
            self.fail("name", "Whoa there hoss, why don't you pick a shorter name?")

The fail() method would then raise a ValidationError to hook into the normal validation logic and reporting.

I like this approach because the validators can be defined anywhere and imported into configuration.py (or even defined locally) with minimal hassle.

@candlerb
Copy link
Contributor

candlerb commented Jun 3, 2021

LGTM: this lets you provide a generic class that implements the basic regexp/min/max/required validations you described before.

CUSTOM_VALIDATORS = {
    'dcim.site': [AttributeValidator({
        'name': {
            'regex': '^[A-Z]{3}\d{2}$',
        },
        'asn': {
            'min_value': 65000,
        }
    })],

@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation and removed needs milestone Awaiting prioritization for inclusion with a future NetBox release labels Jun 8, 2021
@jeremystretch jeremystretch added this to the v3.0 milestone Jun 8, 2021
@jeremystretch jeremystretch linked a pull request Jun 9, 2021 that will close this issue
jeremystretch added a commit that referenced this issue Jun 9, 2021
jeremystretch added a commit that referenced this issue Jun 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 8, 2021
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: feature Introduction of new functionality to the application
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants