Add some directory manipulation nodes #2519
Conversation
joeyballentine
force-pushed
the
path-nodes
branch
from
71c78c2 to
e36535c
Compare
backend/src/packages/chaiNNer_standard/utility/directories/into_directory.py
Outdated
Show resolved
Hide resolved
backend/src/packages/chaiNNer_standard/utility/directories/into_directory.py
Outdated
Show resolved
Hide resolved
backend/src/packages/chaiNNer_standard/utility/directories/into_directory.py
Show resolved
Hide resolved
| ], | ||
| ) | ||
| def into_directory_node(directory: Path, folder: str) -> Path: | ||
| return directory / folder |
There was a problem hiding this comment.
This has issues with path traversal. Example:
>>> Path("C:/foo/bar") / "/bar"
WindowsPath('C:/bar')
>>> Path("C:/foo/bar") / "D:/bar"
WindowsPath('D:/bar')
>>> Path("C:/foo/bar") / "../"
WindowsPath('C:/foo/bar/..')
# ".." is not a valid dir name, so some libraries may resolve the path before.
# This could lead to parts of chainner interpreting paths differently, which can lead to potential security vulnerabilities.
>>> (Path("C:/foo/bar") / "../").parent
WindowsPath('C:/foo/bar')We need to define proper behavior for all these cases. The first question is whether we even want to allow .. and absolute paths.
There was a problem hiding this comment.
I believe with my most recent push all of these are now consistent between the python value and navi type and work as you would expect.
There was a problem hiding this comment.
... You solve one thing. Since you .resolve() now, it does the path traversal for ...
But my question was whether we should even allow that. Same for absolute paths.
There was a problem hiding this comment.
I think so, because it mirrors the behavior of both javascript and python for how they combine paths. Why would we make this different if there's a "standard" for resolving a valid path out of combining path strings like this?
There was a problem hiding this comment.
Because they are joining paths. They don't call these functions "go into directory".
There was a problem hiding this comment.
Sure. Then let's support ... But I don't see how joining with absolute paths would be useful.
There was a problem hiding this comment.
But I don't see how joining with absolute paths would be useful.
These are not absolute paths
There was a problem hiding this comment.
See the following examples:
>>> Path("C:/foo/bar") / "/bar"
WindowsPath('C:/bar')
>>> Path("C:/foo/bar") / "D:/bar"
WindowsPath('D:/bar')The string value this node gets can very well represent an absolute path.
There was a problem hiding this comment.
Ok, then what do you suggest we do? restrict it to just single folder names?
There was a problem hiding this comment.
restrict it to just single folder names?
We can't do that since nodes like Load Images can return multiple levels as their subdirectory path.
I guess we should just disallow absolute paths as the join argument. Unfortunately, python doesn't consider /bar an absolute path on windows. This is consistent with how windows itself treats paths, but it doesn't make sense for a cross-platform API. So we have to handle this ourselves.
Just add an intrinsic function. |
|
Question: do we need the back directory node if into directory supports |
There was a problem hiding this comment.
I think we could just get rid of back directory entirely and rename into directory to change directory
User experience. Normal people don't know that they can use .. in paths.
Also, about naming. I'm not sure whether "back" and "into" are the terms to communicate the operations. I dislike that there is no symmetry between "back" and "into" in their normal usage. "Back" also has the issue that is term is typically used in browsers and file explorers to refer to the history of the current tab/window.
The programming terms are "parent" and "join", but I don't think that we should necessarily copy that (UX again).
I was thinking that we could use the terms "up" and "down", but I'm not sure how to integrate that into the node names...
| if create_path and not resolved.exists(): | ||
| resolved.mkdir(parents=True) | ||
| elif not resolved.exists(): | ||
| raise FileNotFoundError(f"Directory does not exist: {resolved}") |
There was a problem hiding this comment.
So the directory must always exist?
There was a problem hiding this comment.
Yes. Do you not think so? I don't think we should be passing in invalid directories into nodes willy nilly
There was a problem hiding this comment.
Yeah, I don't like it.
- This means that the correctness of a chain now depends on the file system. If user A has folder
foobut user B doesn't, it's not gonna work for user B unless they check that box. Depending on the file system is unavoidable for node like Load Image (singular), but it can be avoided here. - This leads to errors being ignored. Nodes like Load Images don't allow non-existent directories as input for obvious reasons. So if you always just create those directories, then Load Images can't error for incorrect chains.
- We already allow passing non-existent directories. Save Image and the Directory node support it.
- Side effects. You turned this node into a side effect node.
|
Also, we have to handle invalid characters ourselves. Basically, names can't contain |
what would be your recommended approach for that? |
|
Idk. Use a regex or define a set of characters and check each one. Doesn't really matter. |
Making this draft for feedback on these nodes. AFAIK, doing the type for the back directory node is impossible right now.