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

refactor: ポート変更時にEngineInfoの情報を書き換えないようにする。 #2282

Merged
merged 4 commits into from
Oct 8, 2024

Conversation

sabonerune
Copy link
Contributor

内容

#2272 (comment) で提案されたリファクタリングです。

@sabonerune sabonerune requested a review from a team as a code owner October 4, 2024 08:51
@sabonerune sabonerune requested review from Hiroshiba and removed request for a team October 4, 2024 08:51
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

ほぼLGTMです!!!!!
完璧です、ありがとうございます!!!

ちょっと細かい部分でコメントさせていただきました!
もしよければ!

RuntimeInfoの方にも代替ポート情報渡さないとなの抜けていました。。。。
ありがとうございます 🙇 🙇 🙇

src/type/preload.ts Outdated Show resolved Hide resolved
@@ -105,7 +105,7 @@ export type Command = {
export type EngineState = "STARTING" | "FAILED_STARTING" | "ERROR" | "READY";

// ポートが塞がれていたときの代替ポート情報
export type AltPortInfos = Record<EngineId, { from: number; to: number }>;
export type AltPortInfos = Record<EngineId, string>;
Copy link
Member

Choose a reason for hiding this comment

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

(ただのコメントです)

仮にdefaultPortがundefinableになっても、ここは必ずstringになりそう!

const engineHostInfo: HostInfo = {
protocol: engineInfo.protocol,
hostname: engineInfo.hostname,
port: Number(engineInfo.defaultPort),
Copy link
Member

Choose a reason for hiding this comment

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

HostInfo.port、Numberじゃなくてstringでも良さそうな気がしますね!

まあとはいえEngineManifestのportがnumberなので全部stringにはできないですね。
まあどこからstringにするかというだけですが、このエディタ上では大体stringで統一しておくと良さそうかも?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

portHelper周りがNumberで処理しているのでここがNumberとStringを切り替える部分だと思いました。

メモ: portHelper周りをStringに書き換える場合portが数字であることを確認しないとOSコマンドインジェクション脆弱性を引き起こすかもしれない。

Copy link
Member

@Hiroshiba Hiroshiba Oct 7, 2024

Choose a reason for hiding this comment

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

なるほどです!!

portHelper周りもstringで良い気もしました!
けどまあ、既存コードになじませる意図だとNumberのが良さそうなので、このプルリクエストはこの形が良さそうだなと感じました!!
ということでresolve!

メモ: portHelper周りをStringに書き換える場合portが数字であることを確認しないとOSコマンドインジェクション脆弱性を引き起こすかもしれない。

現状、engineManifestの読み込みは型のバリデーションも含むzodで行っているので、必ず数値しか入らないと思われます。


ちょっとこのプルリクとは外れますが、危ないのは事実ですね・・・。
一番危ないのはたぶんコマンド実行している部分execFileSyncがshell: trueで動いてることだと思います。hostnameとかでいたずらされるかもしれない。
shell: falseにすれば問題ないはず・・・。

src/backend/electron/manager/RuntimeInfoManager.ts Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member

色々書いていますがあまり変更に気乗りじゃなかったらご連絡ください!! 🙏
細かい部分の調整はこっちでやっちゃうので、もしよかったらリファクタリングまたお願いできるとめちゃくちゃ助かります!!!

@sabonerune
Copy link
Contributor Author

ちょっとURLAPIの仕様確認に手間取っています。
(ポート番号が空文字(http://example.com:/engine)の場合正常に動作するとは限らないような気がしてきて調べているのですがはっきりしない。)

@sabonerune
Copy link
Contributor Author

一通り修正しました。

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!

完璧なリファクタリングありがとうございました!!
もしよかったらまたぜひ力をお借りしたいです・・・!(このPR内でも出てたshell: trueをfalseにしていくとか)


リファクタリング協力してもいいよ〜って感じでしたら、またお願いさせていただいても良いでしょうか 🙇
ちょっとmain.tsの解体をしていければとか思っているけど手が足りず・・・。

次はテキストファイルの受け渡しをしている部分だけをmain.tsから切り出す、とかどうでしょう・・・?

ここでimportして

import {
ContactTextFileName,
HowToUseTextFileName,
OssCommunityInfosFileName,
OssLicensesJsonFileName,
PolicyTextFileName,
PrivacyPolicyTextFileName,
QAndATextFileName,
UpdateInfosJsonFileName,
} from "@/type/staticResources";

ここで読んで
// 使い方テキストの読み込み
const howToUseText = fs.readFileSync(
path.join(__static, HowToUseTextFileName),
"utf-8",
);
// OSSコミュニティ情報の読み込み
const ossCommunityInfos = fs.readFileSync(
path.join(__static, OssCommunityInfosFileName),
"utf-8",
);
// 利用規約テキストの読み込み
const policyText = fs.readFileSync(
path.join(__static, PolicyTextFileName),
"utf-8",
);
// OSSライセンス情報の読み込み
const ossLicenses = JSON.parse(
fs.readFileSync(path.join(__static, OssLicensesJsonFileName), {
encoding: "utf-8",
}),
) as Record<string, string>[];
// 問い合わせの読み込み
const contactText = fs.readFileSync(
path.join(__static, ContactTextFileName),
"utf-8",
);
// Q&Aの読み込み
const qAndAText = fs.readFileSync(
path.join(__static, QAndATextFileName),
"utf-8",
);
// アップデート情報の読み込み
const updateInfos = JSON.parse(
fs.readFileSync(path.join(__static, UpdateInfosJsonFileName), {
encoding: "utf-8",
}),
) as UpdateInfo[];
const privacyPolicyText = fs.readFileSync(
path.join(__static, PrivacyPolicyTextFileName),
"utf-8",
);

ここでGET関数を定義してますが、
https://github.com/VOICEVOX/voicevox/blob/a1ace7836753ecdb2b2dea0f304383642e668776/src/backend/electron/main.ts#L546C3-L580
全部読み込んでobjectを返し、あとは読み出し関数を作れば100行くらい減る気がしています。

こんな感じのkey型作って

type AssetTextType = "Contact" | "HowToUseText" | ...

あとはtextAsset.tsなどを作って(名前は適当)↓の関数作って

function loadTextAsset() {
  return {
    "Contact": ...,
    "HowToUseText": ...,
  }
}

main.tsでconstに代入しておく。
でipc関数にGET_HOW_TO_USE_TEXTなどがいっぱい生えてるので、ひとまとめに

GET_ASSET_TEXT: (textType: AssetTextType) => {
}

とか定義して・・・みたいな!

結構typescirptの型パズルがややこしい気がしてます。
難しそうだったら適当にdisableしちゃってdraftPR作っていただくとかで・・・・!!!
もしよければ・・・!!!

@Hiroshiba Hiroshiba merged commit 755d84b into VOICEVOX:main Oct 8, 2024
8 checks passed
@sabonerune
Copy link
Contributor Author

テキストファイルの受け渡しの方をやってみます。

@sabonerune sabonerune deleted the refactor/engineInfo branch October 8, 2024 14:36
@Hiroshiba
Copy link
Member

おーぜひぜひ!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants