ComfyUI icon indicating copy to clipboard operation
ComfyUI copied to clipboard

[bug] Custom nodes can overwrite core nodes through class and display name mappings

Open dotJack opened this issue 2 years ago • 0 comments

There's no protection or guard for core nodes in nodes.py's definition against being overwritten by custom nodes. nodes.py: NODE_CLASS_MAPPINGS | NODE_DISPLAY_NAME_MAPPINGS

I was writing custom logic to split all custom nodes to a separate subdirectory and saw that some of the core nodes were being overwritten by custom nodes. Core node fn seems to be linked properly if they're in different categories but names are still overwritten. If they are in the same category and share a name, one or the other will take precedent.

I think the node set to the unique id just gets swapped out along with the description.

I'm mainly a js/ts developer, so I don't know if it would make sense to move the name, description and maybe an additional group field to the node classes to allow for more descriptive definitions / splitting?But that's like a ComfyUi internal architecture decision.

I'm not sure it's possible to fix this issue simply in a backwards compatible way.

One way I could think of was to force each custom node into it's own subcategory (this would fix custom nodes overwriting core nodes) and then move the Display Name Mapping over to some combined unique identifier, so the mapping wouldn't be just off of the name but maybe a categoryName/nodeName format or group/nodeName (this would fix the name overwriting issue)

Alternative fix would be to always load the core nodes at the very end of this logic and overwrite any potential changes to the custom nodes but this will also introduce issues where custom nodes name's will be overwritten.. or the nodes themselves, if they were previously replacing the core nodes.

** EDIT: I added a fix that I thought might work but it's completely backwards not compatible: This fix doesn't work at all, since all the scripts would need to change the nodes in their workflows to use the new names

Keeping it below for visibility. **

~~The fix can be as easy as adding unique identifiers in-front of the entries in nodes.py's NODE_CLASS_MAPPINGS / NODE_DISPLAY_NAME_MAPPINGS.~~

NODE_CLASS_MAPPINGS = {
    "CoreComfy_KSampler": KSampler,
    "CoreComfy_CheckpointLoaderSimple": CheckpointLoaderSimple,
    "CoreComfy_CLIPTextEncode": CLIPTextEncode,
    ...

~~and~~

NODE_DISPLAY_NAME_MAPPINGS = {
    # Sampling
    "CoreComfy_KSampler": "KSampler",
    "CoreComfy_KSamplerAdvanced": "KSampler (Advanced)",
    ...

~~This is not a perfect solution obviously since any of the custom nodes COULD just go in and add CoreComfy_ to the front of their mappings and sidestep this fix if they wanted to for some reason. Not sure why they would outside of being a bad actor of some kind.~~

dotJack avatar Jul 07 '23 10:07 dotJack