Closes #19002: Module type profiles

[jeremystretch]

Closes: #19002

• Move Module & ModuleType models to modules.py
• Introduce the new ModuleTypeProfile model
• Add profile ForeignKey and attribute_data JSONField on ModuleType
• Implement JSONSchemaProperty for rendering form fields from JSON schema properties
• Introduce MultipleOfValidator
• Extend the model form for ModuleType to render attribute fields depending on the selected profile
• Enable filtering of module types by attribute values
• Include some initial profiles to be installed during migration
• Documentation & tests

[alehaa]

Would it make sense to add the supported ModuleTypeProfile to the ModuleBay / ModuleBayTemplate? This would allow filtering like for the manufacturer field when installing a module, i.e. "get all modules with same or no profile". From a usability point of view, it would not make sense to install a CPU profiled Module into a hard_disk ModuleBay.

[jeremystretch]

@alehaa I considered that as part of the planning for this FR, but wasn't happy with the idea of having to define the constraint for every module bay, and defining it on the template feels a bit hacky IMO. At any rate, I opted to punt on it for now as there's already a lot going on in this FR.

You're certainly welcome to open a new FR for it if you'd like. I almost did but wasn't sure if we'd be able to get to it in time for v4.3.

[jnovinger]

Wanted to quickly note one bug I found while clicking around. When you create an module type where an attribute is an integer, then change the value definition in the associated module type profile to "number" and try to edit the module type, you receive KeyError: 'attr_<field>'.

STR:
1. Create a new hard drive module type from the prepopulated "Hard disk" module type profile (/dcim/module-type-profiles/4/), making sure to enter an integer for the size attribute--I used 524 (actual value is 524.3).
2. Modify the module type profile's schema to loosen the size.type specification from integer to number.
3. Now go back and edit the hard drive you created in step 1 to change the size from 524 to 524.3 and save.
4. Edit the module type profile's schema to tighten the size.type specification back up from number to integer.
5. Open the edit form for the hard drive created in step 1 (and edited in step 3) again. Click "Save" without changing anything.

Expected:
- Application happily saves the non-existent changes.

Actual:
- Application raises KeyError: 'attr_size'

Resolved

[jnovinger]

Relatedly, there's what I believe is unexpected behavior related to this flow.

See below, only the last step changes.

Wanted to quickly note one bug I found while clicking around. When you create an module type where an attribute is an integer, then change the value definition in the associated module type profile to "number" and try to edit the module type, you receive KeyError: 'attr_<field>'.

STR:

1. Create a new hard drive module type from the prepopulated "Hard disk" module type profile (`/dcim/module-type-profiles/4/`), making sure to enter an integer for the `size` attribute--I used 524 (actual value is 524.3).

2. Modify the module type profile's `schema` to loosen the `size.type` specification from `integer` to `number`.

3. Now go back and edit the hard drive you created in step 1 to change the size from 524 to 524.3 and save.

4. Edit the module type profile's `schema` to tighten the `size.type` specification back up from `number` to `integer`.

5. Open the edit form for the hard drive created in step 1 (and edited in step 3) again. Change the size attribute from 524.3 back to 524.

Expected:

* Application would change the hard drive module type's size attribute from 524.3 to 524.

Actual:

* Application returns errors in the form for the size field: This field is required. This happens even though a valid value is present in the field.

Resolved

[jnovinger]

One more attribute validation bug.

STR:
1. Create a GPU module type using the GPU module type profile (dcim/module-type-profiles/3/)
2. Select any of the enumerated Interface values and save
3. Edit the GPU module type you just created
4. Clear the selection from the Interface field and try to save

Expected:

- Form validates changed data and saves the existing module type

Actual:

- Form return validation error indicating lack of selection is invalid, even though the field is not marked required: Invalid schema: '' is not one of ['PCIe 4.0', 'PCIe 4.0 x8', 'PCIe 4.0 x16', 'PCIe 5.0 x16', 'SoC'] Failed validating 'enum' in schema['properties']['interface']: {'enum': ['PCIe 4.0', 'PCIe 4.0 x8', 'PCIe 4.0 x16', 'PCIe 5.0 x16', 'SoC'], 'type': 'string'} On instance['interface']: ''

Edited to add

It looks like you don't even need to save it with a value and then edit it. The Interface field comes preopulated with the 'PCIe 4.0' value and exhibits the same behavior when you try to remove it and save.

Resolved

[jnovinger]

Grr, sorry, one more.

STR:
1. Create module type with the GPU module type profile (dcim/module-type-profiles/3/)
2. Edit the newly created module type and clear out the Profile field so that no profile is selected
3. Click Save

Expected:

- Form validates successfully and saves the changes in data

Actual:

- Form performs validation as if the previously attached module type profile is still attached--it returns a form error for the Memory field (which was correctly hidden when the Profile field was cleared).

Resolved

[jeremystretch]

There's a weird oddity with the HTML form when switching from a float (decimal) to an integer. Assuming an initial attribute value of 1.5, the form field will render as below, with no step attribute.

<input type="number" name="attr_foo" value="1.5">

Because there's no step attribute, a default step value of 1 is assumed. Unfortunately, this means the field only accept values ending in .5 (0.5, 1.5, 2.5, etc.), and the user is stuck: Integer values fail frontend validation and decimal values fail backend validation.

I'm not sure how best to work around this. Attempting to re-cast the initial value could lead to data loss, so we probably want to avoid that. The cleanest approach would be to enforce validation against existing attributes whenever the schema is changed.

Edit: I ended up just using Django's FloatField for both integers and decimals to work around this. The appropriate validation is still enforced on the backend.

[jeremystretch]

Form performs validation as if the previously attached module type profile is still attached--it returns a form error for the Memory field (which was correctly hidden when the Profile field was cleared).

This is due to bug #19023 (for which a PR is currently open). Once the fix for that is available, I'll merge it here and confirm.

[jnovinger]

Noted some bugs in regular PR comments. This review also has a couple of questions.

[jnovinger]

AttributeFiltersMixin seems to have some interesting characteristics. For instance, the following query string works for finding CPUs module types with 12 or 120 or even 512 cores: ?attr_cores__icontains=12.

I suppose that not necessarily bad, just wondering if it's intended for a string-style lookup to work on an attribute specified as an integer. It does ignore things that don't make sense as lookups like (?attr__jason=jason when there's no jason attribute), which seems to be the correct behavior.

[jnovinger]

Although, this got me to wondering if I could use __isnull on an attribute that is not required. When I tried ?attr_core__isnull=True I got a surprising result: ValueError: The QuerySet value for an isnull lookup must be True or False.

I think this probably points to the need to clamp down on what lookups are allowed for attributes. Otherwise, I think we'll playing whackamole as people try things we didn't think of.

Resolved

[jeremystretch]

I looked into this a bit but I don't think there's much we can do here realistically. The filterset can't know ahead of time what type of queries to enforce, and module types assigned different profiles may have overlapping fields. For example, one profile might declare an integer named foo and another profile a string named foo. There's no way to reconcile these when filtering for ?foo=, other than perhaps making some assumptions based on the value passed.

The only way I can think of to lock this down reliably would be to exclude anything other than exact lookups on attributes, but that would remove a substantial amount of functionality from the feature.

[jnovinger]

That's an incredibly fair point.

Any thoughts on the __isnull lookup issue? I don't think it would be as big a deal if it just silently ignored it, but raising a ValueError and causing a 500 seem pretty not ideal.

[jnovinger]

One more weird bug around attributes and schemas. If you specify an array-type field in a module type profile, but try to use it when creating a module type, a TypeError is raised.

1. Add a new attribute to any schema, with "type": "array" and no other arguments. Save the profile.
2. Try to create a new module type with that profile.

Expected:

- Form is rendered

Actual:

- TypeError: SimpleArrayField.__init__() missing 1 required positional argument: 'base_field' is raised

This might be as simple as validating array-type attributes, but at the moment, you are allowed to save a schema like the following:

{
    "properties": {
        "aliases": {
            "type": "array"
        },
        "architecture": {
            "type": "string"
        },
        "cores": {
            "minimum": 1,
            "multipleOf": 2,
            "type": "number"
        },
        "speed": {
            "maximum": 1,
            "minimum": 0.5,
            "multipleOf": 0.01,
            "type": "number"
        }
    }
}

Resolved

[jnovinger]

I noticed that schema values of null and "" don't affect any change on the items selected. However, I noticed that {} does essentially clear the schemas of the selected items.

This might be the intended behavior, but was not super intuitive.

Resolved

[jnovinger]

Relatedly, true and false literals were accepted as valid input for this field, even though they don't necessarily make sense.

Perhaps values should be locked down to some representation of no schema (null or {} perhaps) and some schema that is at least of type object. Not sure, though, just thinking out loud.

Resolved

[jnovinger]

This is due to bug #19023 (for which a PR is currently open). Once the fix for that is available, I'll merge it here and confirm.

I went ahead and snagged the review for that one. It's been merged.

[jnovinger]

💥