nautobot-app-ssot icon indicating copy to clipboard operation
nautobot-app-ssot copied to clipboard

Contrib: Generic Foreign Key raises FieldError on update or delete

Open gioccher opened this issue 1 year ago • 2 comments

Environment

  • Python version: 3.9.18
  • Nautobot version: 1.6.8
  • nautobot-ssot version: 1.6.0

Expected Behavior

Create, Update, Delete operations work

Observed Behavior

The Create operation works, but Update and Delete fail with FieldError: Field 'termination_a' does not generate an automatic reverse relation and therefore cannot be used for reverse querying. If it is a GenericForeignKey, consider adding a GenericRelation. full exception trace with context

Steps to Reproduce

I'm trying to have my SSOT integration handle Cables using the ssot contrib modules. Cable is a somewhat special model since it contains two generic foreign keys. This is how I modeled the cable in SSOT:

class CableModel(NautobotModel):
    """Diffsync model for Cable"""

    _model = Cable
    _modelname = "cable"
    _identifiers = (
        "termination_a__device__name",
        "termination_a__name",
        "termination_a__app_label",
        "termination_a__model",
        "termination_b__device__name",
        "termination_b__name",
        "termination_b__app_label",
        "termination_b__model",
    )
    _attributes = (
        "label",
        "color",
        "type",
        "status__slug",
    )
    _children = {}

    termination_a__device__name: str
    termination_a__name: str
    termination_a__app_label: str
    termination_a__model: str
    termination_b__device__name: str
    termination_b__name: str
    termination_b__app_label: str
    termination_b__model: str
    label: Optional[str]
    color: Optional[str]
    type: Optional[str]  # noqa: A003
    status__slug: str

If the integration only has to create Cables then everything works as expected. image (3)

If I modify a cable in the nautobot web interface (let's say I change the cable Label) and I run the integration again with the same source data then it fails raising the above exception image (4)

Here's how I'm populating the cable model

class SourceGitAdapter(DiffSync):
    """DiffSync adapter for git repo."""

    vlangroup = VLANGroupModel
    vlan = VLANModel
    device = DeviceModel
    interface = InterfaceModel
    ipaddr = IPAddressModel
    cable = CableModel
    top_level = ("ipaddr", "device", "vlangroup", "vlan", "cable")
    
    ...
    
    def load_cables(self):
        def _get_model(dev_name, int_name):
            if dev_name[6:9] == "ppa":
                return "rearport" if int_name.startswith("Rear") else "frontport"
            return "interface"

        for site_data, _ in self.get_site_configs():
            cables = site_data.get("cables", [])
            for cable in cables:
                a_device, a_port = cable["a"].split(".")
                b_device, b_port = cable["b"].split(".")
                loaded_cable = self.cable(
                    termination_a__device__name=a_device,
                    termination_a__name=a_port,
                    termination_a__app_label="dcim",
                    termination_a__model=_get_model(a_device, a_port),
                    termination_b__device__name=b_device,
                    termination_b__name=b_port,
                    termination_b__app_label="dcim",
                    termination_b__model=_get_model(b_device, b_port),
                    type=cable.get("type", ""),
                    color=cable.get("color", ""),
                    label=cable.get("label", ""),
                    status__slug=cable.get("status", "connected"),
                )
                self.add(loaded_cable)

gioccher avatar Jan 16 '24 17:01 gioccher

Hi, thanks for the submission! As part of #292 I looked at cables, too. One important thing to note here is that updating the terminations of a cable is not supported, but since you have all four termination fields in _identifiers I don't think that this is the issue. IMO there is two parts to this issue:

  • [ ] Improving the error message for when things like this happen, i.e. catch the FieldError and output an error message that is more useful to the end user
  • [ ] Change this line right here as well as the corresponding one for the B side such that the generic foreign key generate an inverse relation on the other side of the model - I have created https://github.com/nautobot/nautobot/issues/5140 on the Nautobot side to track this

Kircheneer avatar Jan 22 '24 12:01 Kircheneer

So Cable was indeed special, but for another reason than I was thinking! Thanks for troubleshooting this The confusing part of the error message was figuring out who it was mad with (me not modeling the class correctly or something else?).

I'm still on NB 1.6.X but I won't need the fix backported: to workaround this I ended up rewriting the CableModel class to inherit from DiffSyncModel instead of NautobotModel. Made me appreciate how much of a time-saver the Contrib module is when it works as expected :)

gioccher avatar Jan 22 '24 23:01 gioccher