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.

Sign up for GitHub

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

Jump to bottom

Add some directory manipulation nodes #2519

Closed
wants to merge 5 commits into from
Closed

Add some directory manipulation nodes #2519

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e36535c
Add some directory manipulation nodes
joeyballentine Jan 27, 2024
e656560
fix separator condition
joeyballentine Jan 27, 2024
0424eb4
make navi conditions for this
joeyballentine Jan 27, 2024
e3ae6ea
resolve combined dir
joeyballentine Jan 27, 2024
9b56b07
create if not exists
joeyballentine Jan 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions backend/src/packages/chaiNNer_standard/utility/directories/back_directory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
from __future__ import annotations

from pathlib import Path

from nodes.properties.inputs import DirectoryInput, NumberInput
from nodes.properties.outputs import DirectoryOutput

from .. import value_group


@value_group.register(
schema_id="chainner:utility:back_directory",
name="Back Directory",
description="Traverse up/back from a directory the specified number of times.",
icon="BsFolder",
inputs=[
DirectoryInput(
"Directory", must_exist=False, label_style="hidden", has_handle=True
),
NumberInput("Amount back", has_handle=True, minimum=1, precision=0),
],
outputs=[
DirectoryOutput("Directory"),
],
)
def back_directory_node(directory: Path, amt: int) -> Path:
result = directory
for _ in range(amt):
result = result.parent
return result
29 changes: 29 additions & 0 deletions backend/src/packages/chaiNNer_standard/utility/directories/directory_to_text.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
from __future__ import annotations

from pathlib import Path

from nodes.properties.inputs import DirectoryInput
from nodes.properties.outputs import TextOutput

from .. import value_group


@value_group.register(
schema_id="chainner:utility:directory_to_text",
name="Directory to Text",
description="Converts a directory path into usable text.",
icon="BsFolder",
inputs=[
DirectoryInput(
"Directory", must_exist=False, label_style="hidden", has_handle=True
),
],
outputs=[
TextOutput(
"Directory Text",
output_type="Input0.path",
),
],
)
def directory_to_text_node(directory: Path) -> str:
return str(directory)
35 changes: 35 additions & 0 deletions backend/src/packages/chaiNNer_standard/utility/directories/into_directory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
from __future__ import annotations

import sys
from pathlib import Path

from nodes.properties.inputs import DirectoryInput, TextInput
from nodes.properties.outputs import DirectoryOutput

from .. import value_group

separator = "/" if sys.platform == "linux" else r"\\"
RunDevelopment marked this conversation as resolved.
Show resolved Hide resolved


@value_group.register(
schema_id="chainner:utility:into_directory",
name="Into Directory",
RunDevelopment marked this conversation as resolved.
Show resolved Hide resolved
description="Goes forward into a directory.",
icon="BsFolder",
inputs=[
DirectoryInput(
"Directory", must_exist=False, label_style="hidden", has_handle=True
),
TextInput("Folder", has_handle=True),
],
outputs=[
DirectoryOutput(
"Directory",
output_type='Directory { path: string::concat(Input0.path, "'
+ separator
+ '", Input1) }',
joeyballentine marked this conversation as resolved.
Show resolved Hide resolved
),
],
)
def into_directory_node(directory: Path, folder: str) -> Path:
return directory / folder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because they are joining paths. They don't call these functions "go into directory".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Then let's support ... But I don't see how joining with absolute paths would be useful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I don't see how joining with absolute paths would be useful.

These are not absolute paths

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, then what do you suggest we do? restrict it to just single folder names?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading