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.

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

整理: Setting をモジュール内に限局 #1397

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions run.py
Original file line number Diff line number Diff line change
Expand Up @@ -297,19 +297,19 @@ def main() -> None:
)

setting_loader = SettingHandler(args.setting_file)
settings = setting_loader.load()
setting_cors_policy_mode, setting_allow_origin_raw = setting_loader.load()

# 複数方式で指定可能な場合、優先度は上から「引数」「環境変数」「設定ファイル」「デフォルト値」

root_dir = select_first_not_none([voicevox_dir, engine_root()])

cors_policy_mode = select_first_not_none(
[arg_cors_policy_mode, settings.cors_policy_mode]
[arg_cors_policy_mode, setting_cors_policy_mode]
)

setting_allow_origin = None
if settings.allow_origin is not None:
setting_allow_origin = settings.allow_origin.split(" ")
if setting_allow_origin_raw is not None:
setting_allow_origin = setting_allow_origin_raw.split(" ")
allow_origin = select_first_not_none_or_none(
[arg_allow_origin, setting_allow_origin]
)
Expand Down
58 changes: 32 additions & 26 deletions test/unit/setting/test_setting.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,85 +4,91 @@
from pydantic import ValidationError

from voicevox_engine.setting.model import CorsPolicyMode
from voicevox_engine.setting.setting_manager import Setting, SettingHandler
from voicevox_engine.setting.setting_manager import SettingHandler, _Setting


def test_setting_handler_load_not_exist_file() -> None:
"""`SettingHandler` に存在しない設定ファイルのパスを渡すとデフォルト値になる。"""
# Inputs
setting_loader = SettingHandler(Path("not_exist.yaml"))
settings = setting_loader.load()
# Expects
true_setting = {"allow_origin": None, "cors_policy_mode": CorsPolicyMode.localapps}
true_cors_policy_mode = CorsPolicyMode.localapps
true_allow_origin = None
# Outputs
setting = settings.model_dump()
cors_policy_mode, allow_origin = setting_loader.load()
# Test
assert true_setting == setting
assert true_cors_policy_mode == cors_policy_mode
assert true_allow_origin == allow_origin


def test_setting_handler_load_exist_file_1() -> None:
"""`SettingHandler` に設定ファイルのパスを渡すとその値を読み込む。"""
# Inputs
setting_path = Path("test/unit/setting/setting-test-load-1.yaml")
setting_loader = SettingHandler(setting_path)
settings = setting_loader.load()
# Expects
true_setting = {"allow_origin": None, "cors_policy_mode": CorsPolicyMode.localapps}
true_cors_policy_mode = CorsPolicyMode.localapps
true_allow_origin = None
# Outputs
setting = settings.model_dump()
cors_policy_mode, allow_origin = setting_loader.load()
# Test
assert true_setting == setting
assert true_cors_policy_mode == cors_policy_mode
assert true_allow_origin == allow_origin


def test_setting_handler_load_exist_file_2() -> None:
"""`SettingHandler` に設定ファイルのパスを渡すとその値を読み込む。"""
# Inputs
setting_path = Path("test/unit/setting/setting-test-load-2.yaml")
setting_loader = SettingHandler(setting_path)
settings = setting_loader.load()
cors_policy_mode, allow_origin = setting_loader.load()
# Expects
true_setting = {"allow_origin": None, "cors_policy_mode": "all"}
true_cors_policy_mode = CorsPolicyMode.all
true_allow_origin = None
# Outputs
setting = settings.model_dump()
cors_policy_mode, allow_origin = setting_loader.load()
# Test
assert true_setting == setting
assert true_cors_policy_mode == cors_policy_mode
assert true_allow_origin == allow_origin


def test_setting_handler_load_exist_file_3() -> None:
"""`SettingHandler` に設定ファイルのパスを渡すとその値を読み込む。"""
# Inputs
setting_path = Path("test/unit/setting/setting-test-load-3.yaml")
setting_loader = SettingHandler(setting_path)
settings = setting_loader.load()
cors_policy_mode, allow_origin = setting_loader.load()
# Expects
true_setting = {
"allow_origin": "192.168.254.255 192.168.255.255",
"cors_policy_mode": CorsPolicyMode.localapps,
}
true_cors_policy_mode = CorsPolicyMode.localapps
true_allow_origin = "192.168.254.255 192.168.255.255"
# Outputs
setting = settings.model_dump()
cors_policy_mode, allow_origin = setting_loader.load()
# Test
assert true_setting == setting
assert true_cors_policy_mode == cors_policy_mode
assert true_allow_origin == allow_origin


def test_setting_handler_save(tmp_path: Path) -> None:
"""`SettingHandler.save()` で設定値を保存できる。"""
# Inputs
setting_path = tmp_path / "setting-test-dump.yaml"
setting_loader = SettingHandler(setting_path)
new_setting = Setting(cors_policy_mode=CorsPolicyMode.localapps)
new_setting_cors = CorsPolicyMode.localapps
new_setting_origin = None
# Expects
true_setting = {"allow_origin": None, "cors_policy_mode": CorsPolicyMode.localapps}
true_cors_policy_mode = CorsPolicyMode.localapps
true_allow_origin = None
# Outputs
setting_loader.save(new_setting)
setting_loader.save(new_setting_cors, new_setting_origin)
# NOTE: `.load()` の正常動作を前提とする
setting = setting_loader.load().model_dump()
cors_policy_mode, allow_origin = setting_loader.load()
# Test
assert true_setting == setting
assert true_cors_policy_mode == cors_policy_mode
assert true_allow_origin == allow_origin


def test_setting_invalid_input() -> None:
"""`Setting` は不正な入力に対してエラーを送出する。"""
# Test
with pytest.raises(ValidationError) as _:
Setting(cors_policy_mode="invalid_value", allow_origin="*")
_Setting(cors_policy_mode="invalid_value", allow_origin="*")
19 changes: 4 additions & 15 deletions voicevox_engine/app/routers/setting.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

from voicevox_engine.engine_manifest import BrandName
from voicevox_engine.setting.model import CorsPolicyMode
from voicevox_engine.setting.setting_manager import Setting, SettingHandler
from voicevox_engine.setting.setting_manager import SettingHandler
from voicevox_engine.utility.path_utility import resource_root

from ..dependencies import check_disabled_mutable_api
Expand All @@ -31,10 +31,7 @@ def setting_get(request: Request) -> Response:
"""
設定ページを返します。
"""
settings = setting_loader.load()

cors_policy_mode = settings.cors_policy_mode
allow_origin = settings.allow_origin
cors_policy_mode, allow_origin = setting_loader.load()

if allow_origin is None:
allow_origin = ""
Expand All @@ -56,15 +53,7 @@ def setting_post(
cors_policy_mode: Annotated[CorsPolicyMode, Form()],
allow_origin: Annotated[str | SkipJsonSchema[None], Form()] = None,
) -> None:
"""
設定を更新します。
"""
settings = Setting(
cors_policy_mode=cors_policy_mode,
allow_origin=allow_origin,
)

# 更新した設定へ上書き
setting_loader.save(settings)
"""設定を更新します。"""
setting_loader.save(cors_policy_mode, allow_origin)

return router
33 changes: 18 additions & 15 deletions voicevox_engine/setting/setting_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,22 @@

from enum import Enum
from pathlib import Path
from typing import TypeAlias

import yaml
from pydantic import BaseModel, Field
from pydantic import BaseModel

from ..utility.path_utility import get_save_dir
from .model import CorsPolicyMode

_AllowOrigin: TypeAlias = str | None

class Setting(BaseModel):
"""
エンジンの設定情報
"""

cors_policy_mode: CorsPolicyMode = Field(title="リソース共有ポリシー")
allow_origin: str | None = Field(default=None, title="許可するオリジン")
class _Setting(BaseModel):
"""エンジンの設定情報"""

cors_policy_mode: CorsPolicyMode # リソース共有ポリシー
allow_origin: _AllowOrigin # 許可するオリジン


USER_SETTING_PATH: Path = get_save_dir() / "setting.yml"
Expand All @@ -33,24 +34,26 @@ def __init__(self, setting_file_path: Path) -> None:
"""
self.setting_file_path = setting_file_path

def load(self) -> Setting:
def load(self) -> tuple[CorsPolicyMode, _AllowOrigin]:
"""設定値をファイルから読み込む。"""
if not self.setting_file_path.is_file():
# 設定ファイルが存在しないためデフォルト値を取得
setting = {"allow_origin": None, "cors_policy_mode": "localapps"}
else:
# 指定された設定ファイルから値を取得
# FIXME: 型チェックと例外処理を追加する
# FIXME: 例外処理を追加する
setting = yaml.safe_load(self.setting_file_path.read_text(encoding="utf-8"))

return Setting(
cors_policy_mode=setting["cors_policy_mode"],
allow_origin=setting["allow_origin"],
)
setting_obj = _Setting.model_validate(setting)
return setting_obj.cors_policy_mode, setting_obj.allow_origin
Comment on lines -46 to +48
Copy link
Member

@Hiroshiba Hiroshiba Jun 18, 2024

Choose a reason for hiding this comment

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

これはちょっと避けたいですね・・・。
どれがどれかわからず、フラグが2つ追加された時点でわからなくなり、またSetting構造体に戻ると思います。

内部型であってもrouterに露出するのは問題ないはずです。
例えばデータ変換のときとかは必ず渡さないとなので。
Settingをやり取りする形のがメンテナンス性高そうに思いました!

BaseModelじゃなくても良さそうには思います。


def save(self, settings: Setting) -> None:
def save(
self, cors_policy_mode: CorsPolicyMode, allow_origin: _AllowOrigin
) -> None:
"""設定値をファイルへ書き込む。"""
settings_dict = settings.model_dump()
settings_dict = _Setting(
cors_policy_mode=cors_policy_mode, allow_origin=allow_origin
).model_dump()

if isinstance(settings_dict["cors_policy_mode"], Enum):
settings_dict["cors_policy_mode"] = settings_dict["cors_policy_mode"].value
Expand Down