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

Static and Dynamic Query Params Are Copied between APISelect Widgets within Separate Forms #7176

Closed
thatmattlove opened this issue Sep 4, 2021 · 1 comment
Assignees
Labels
status: under review Further discussion is needed to determine this issue's scope and/or implementation type: bug A confirmed report of unexpected behavior in the application

Comments

@thatmattlove
Copy link
Contributor

thatmattlove commented Sep 4, 2021

NetBox version

v3.0.1

Python version

3.8

Steps to Reproduce

  1. In one tab, open a device and edit one of its interfaces (DCIM → Devices → Interfaces Tab). For this example, I'm going to assume this device's PK is 1
  2. In another tab, open another device, and edit one of its interfaces. For this example, I'm going to assume this device's PK is 2
  3. Select an 802.1Q mode of Access
  4. In developer tools, use the Element selector tool to find the Select element for the untagged_vlans field for Device `. It should look like this:
<select
    name="untagged_vlan"
    class="netbox-api-select"
    data-dynamic-params='[{"fieldName":"vlan_group","queryParam":"group_id"},{"fieldName":"vlan_group","queryParam":"group_id"}]'
    data-empty-option=""
    placeholder="Untagged VLAN" 
    data-static-params='[{"queryParam":"available_on_device","queryValue":[1]}]'
    data-url="/api/ipam/vlans/?available_on_device=1&brief=true"
    id="id_untagged_vlan"
    tabindex="-1"
    data-ssid="ss-62088"
    style="display: block; opacity: 0; width: 0px; height: 0px; position: absolute; pointer-events: none;">
</select>
  1. Do the same for Device 2.
  2. Refresh the tab for Device 1. You should now see:
<select
    name="untagged_vlan"
    class="netbox-api-select"
    data-dynamic-params='[{"fieldName":"vlan_group","queryParam":"group_id"},{"fieldName":"vlan_group","queryParam":"group_id"}]'
    data-empty-option=""
    placeholder="Untagged VLAN" 
    data-static-params='[{"queryParam":"available_on_device","queryValue":[1,2]}]'
    data-url="/api/ipam/vlans/?available_on_device=1&available_on_device=2&brief=true"
    id="id_untagged_vlan"
    tabindex="-1"
    data-ssid="ss-62088"
    style="display: block; opacity: 0; width: 0px; height: 0px; position: absolute; pointer-events: none;">
</select>

If you refresh Device 2's tab, you should see the same result.

This can also be replicated in nbshell:

>>> from dcim.forms import InterfaceForm
>>> d1 = Device.objects.filter(pk=1).first()
>>> d2 = Device.objects.filter(pk=2).first()
>>> f1 = InterfaceForm(data={'id': d1.interfaces.first().pk, 'device': d1.pk})
>>> f2 = InterfaceForm(data={'id': d2.interfaces.first().pk, 'device': d2.pk})
>>> print(f1.fields['untagged_vlan'].widget.static_params)
{'available_on_device': [1, 2]}
>>> print(f2.fields['untagged_vlan'].widget.static_params)
{'available_on_device': [1, 2]}

After digging into why, I found that when each field is initialize, Django deep copies the field's widget. This uses the __deepcopy__ method on the widget. Since APISelect subclasses django.forms.Select and not django.forms.Widget, that deep copy takes place via this method.

To fix the issue, adding a __deepcopy__ method on APISelect that runs the built-in __deepcopy__ and creates new static_params and dynamic_params seems to do the trick:

# netbox/utilities/forms/widgets.py

class APISelect:
# Omitted current code for brevity
    def __deepcopy__(self, memo):
        result = super().__deepcopy__(memo)
        result.dynamic_params = {}
        result.static_params = {}
        return result

Expected Behavior

static_params and dynamic_params are custom properties added to track the widget's dynamic and static query parameters. Historically, this was tracked directly on the underlying Widget (field['<field_name>'].widget.attrs), and overridden by Django after the deep copy. While we could semi-replicate this behavior with the new static_params and dynamic_params properties, it seems to be the fact that those properties are copied over to begin with is not desired. I could see getting into a situation in the future where these properties being copied over could cause some weird bugs. To me, it makes more sense to exclude these from the widget copy.

Essentially, I'd like a thumbs up on this approach, or a suggestion for a different approach in case there are things I haven't considered.

Observed Behavior

By default, any custom properties defined on a field that are not excluded from deep copying are copied over between forms. See above Steps to Reproduce for examples.

@thatmattlove thatmattlove added the type: bug A confirmed report of unexpected behavior in the application label Sep 4, 2021
@thatmattlove thatmattlove added the status: under review Further discussion is needed to determine this issue's scope and/or implementation label Sep 4, 2021
@jeremystretch
Copy link
Member

To fix the issue, adding a deepcopy method on APISelect that runs the built-in deepcopy and creates new static_params and dynamic_params seems to do the trick

I suppose this works for a quick fix, but long-term I think we need a more robust solution. The APISelect stuff was all upset a bit during the v3.0 migration and probably needs to be revisited as a whole. But I'm okay with this solution for now if it resolves the bug.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: under review Further discussion is needed to determine this issue's scope and/or implementation type: bug A confirmed report of unexpected behavior in the application
Projects
None yet
Development

No branches or pull requests

2 participants