Fixes #20042: Support Bridge-Interfaces in Interface-Template Import #20082
Conversation
The related object fields are not covered by the form, so don't pass any validation before trying to iterate over them and accessing their elements. Instead of allowing a hard technical error to be raised, explicitly check that it is indeed a list, and raise a normal validation error if not.
Elements of the "related objects list" are passed to the `prep_related_object_data` function before any validation takes place, with the potential of failing with a hard error. Similar to the "related objects not list" case explicitly validate the elements general type, and raise a normal validation error if it isn't a dictionary.
Adding the field to the InterfaceTemplateImportForm is enough. The two clean_*_type methods are for properly dynamically restricting what values can be selected.
Related objects are saved in the same order as they are defined in the input by the user. So related objects with a dependency on another need to be saved after the related object they depend on. Which means that when required, the order needs to be changed. The related object lists are not part of the actual form, and the individual related-object forms only cover the elements themselves. So while the re-order should be done at the form-level during the clean- step, there is no form covering the lists to use for that. Plus should all errors be reported with their original location/index and not with the fixed/resorted one, because these are not useful and even confusing for the user. So the sorting should be done as late as possible, ideally just before the save. This adds a new `prep_related_object_list` hook, which gets passed each list of related objects. This should only be used for modifications to the list (aka order). Any changes to the individual objects should still be done using the existing `prep_related_object_data` hook. The passed list is not the plain list, but an enumerated variant. This is needed to preserve the original index of the elements independently from the actual (possibly changed) order the list, so any later detected errors (for example by the elements form validation) can accurately be reported back to the user.
Use the new hook to re-sort the interface template list. It is only done when required to preserve the original order and prevent any unneeded computation. But even when sorting, it tries its best to preserve as the original order as much as possible. The sorting is still done before any validation of the individual elements, so needs to be able to work with invalid/incomplete/malformed data. The only guarantee is that we get a list (of something). It builds a simple dependency graph, uses Kahn's algorithm to create a topological sorting of it, and applies that to the interface template list.
Use a depth-first search to identify any cycles, to be able to report them as such to the user.
m-hau
force-pushed
the
20042-interface-template-import-bridge
branch
from
9e7ced1 to
96cdddd
Compare
|
This is a first draft to show the general idea and implementation, not ready to be merged, with a few parts missing and questions/TODOs still open. I would kindly request some first feedback from the more knowledgeable and codebase-familiar people around here, if this is a valid and acceptable way of doing this. |
jnovinger
requested review from
a team and
jeremystretch
and removed request for
a team
There was a problem hiding this comment.
@m-hau while it's clear you've invested a lot of effort in this, I'm afraid the complexity of the proposed implementation is disproportionate to the value of the functionality it provides. Have you considered any alternative approaches which don't require such extensive modifications to the existing code?
|
I am ignoring the list/dict validation fixes here (the first two commits), since those are general fixes that should be applied anyway. Would it be good to extract them into their own issue/PR? The simplest solution would be to just add the field to the form (what commit 3 does). This generally allows that field to be imported, but since there is no sorting happening the interfaces in the input data already need to be properly sorted, such that bridge interfaces come before their bridged interfaces, which may be acceptable from a UX perspective. The next step up would be the new hook to sort the list, ensuring a simple sort order like "first all non-bridged-interfaces, then the rest". This works for all non-recursive cases (where a bridge interface is never also a bridged interface), and should also work for simple cases if in future "parent interface" relations are also importable. If validation isn't explicitly handled, it suffers from the same unspecific validation error message as above. And the imho "correct" solution is the full blown implementation I did using a dependency graph, which covers all current (and future) cases and allows specific and actionable user feedback. EDIT: There is also the variant of splitting the interface creation into two phases. First create all interfaces but without any relations (bridge, parent, ...), and in phase two add all the relations. In that case the sort order is irrelevant, but requires a special handling/implementation for interface imports. It would also allow for circular dependencies, which may or may not be desired. |
Fixes: #20042
When importing device- or module-types, allow their interface-templates to contain the
bridgeattribute and properly save it.The unexpectedly hardest part was finding the right place to hook into the existing process, which in this cases required to create an entirely new hook-point. While developing and testing I also found some other minor bugs in the generalized import validation, and attempted to fix them. Please see the individual commit messages for more information.