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

コントロールパネルの全般ページでサーバ情報を編集・保存するとnullな項目が空文字に置き換わる #15075

Open
1 task
samunohito opened this issue Nov 30, 2024 · 13 comments
Labels
🐛Bug Unexpected behavior [Feat] ControlPanel Issues related to existing functionality, such as bugs or adding small features. packages/frontend Client side specific issue/PR

Comments

@samunohito
Copy link
Member

💡 Summary

Image
取得時は↑のような内容なのですが、shortName(画面上ではサーバ略称)だけを空欄にして保存ボタンを押すと

Image
Image
↑空文字が送信され、nullだった項目が空文字となってしまいました。

🥰 Expected Behavior

ClientServerService.tsなど、フィールドがnullであることを期待する処理はいくつかあるので、空文字の場合はnullとして登録するのが望ましい挙動と考えます。

🤬 Actual Behavior

nullだったフィールドがフロントエンドを経由することで空文字として登録される

📝 Steps to Reproduce

  1. サーバ情報などがnullなサーバを用意する
  2. コントロールパネルの全般ページでサーバ情報を更新する

💻 Frontend Environment

* Model and OS of the device(s): any
* Browser: any
* Server URL: any
* Misskey: 2024.11.0

🛰 Backend Environment (for server admin)

* Installation Method or Hosting Service: any
* Misskey: 2024.11.0
* Node: any
* PostgreSQL: any
* Redis: any
* OS and Architecture: any

Do you want to address this bug yourself?

  • Yes, I will patch the bug myself and send a pull request
@samunohito samunohito added [Feat] ControlPanel Issues related to existing functionality, such as bugs or adding small features. packages/frontend Client side specific issue/PR 🐛Bug Unexpected behavior labels Nov 30, 2024
@samunohito
Copy link
Member Author

samunohito commented Nov 30, 2024

サーバ側の実装を見るとnullも受け付けることから、

  • nullである場合はnullをそのまま送り返す
  • 空文字である場合はnullとする
  • (可能であれば)変更のないフィールドはリクエストに含めない

のような仕組みをフロントエンド側に設けるのが良いと考えます

@syuilo
Copy link
Member

syuilo commented Nov 30, 2024

バックエンド側でnullと空文字が区別される状態になっているのが諸悪の根源だわね

@samunohito
Copy link
Member Author

(変えてないはずの値を別の値にして送り返してる…という所に対して違和感を感じるのだけど、僕だけかしら…)

@syuilo
Copy link
Member

syuilo commented Nov 30, 2024

別の値にしてしまうのはバックエンドがおかしな値を送ってきてるせいだからフロントエンドの実装にそこまで違和感はないわね

@samunohito
Copy link
Member Author

本来、meta.nameなどで「空の値」として想定されるのは空文字であるということです…?
(だとすると、テーブル定義はなぜnullableに…?という話に)

@syuilo
Copy link
Member

syuilo commented Nov 30, 2024

本来空の場合は空文字にしたい
nullableは歴史的経緯だわね

@syuilo
Copy link
Member

syuilo commented Nov 30, 2024

まあ空文字で持つとしてもAPIのレスポンス的にはnullにしたい可能性があるからその場合はフロントエンドで何かしらのケアは要るわね

@syuilo
Copy link
Member

syuilo commented Nov 30, 2024

今からカラム定義を変えるのは現実的ではないからバックエンド的には空文字を許さない方向にして、フロントエンドから空文字が飛んできたらnullとみなすようにするのが良さそう

@syuilo
Copy link
Member

syuilo commented Nov 30, 2024

本来空の場合は空文字にしたい nullableは歴史的経緯だわね

理由: 型が簡潔になり、空判定も簡潔になる

@samunohito
Copy link
Member Author

フロントエンドから空文字が飛んできたらnullとみなすようにするのが良さそう

undefinedがきた -> その値は変えない
null -> nullにする
空文字 -> nullにする

なら、まあ…いいのかな…?

@fruitriin
Copy link
Contributor

fruitriin commented Nov 30, 2024

明示的にnullになることに依存した実装がなければ、BEでnullを空文字にして保存、DBのnullを空文字変換、非nullableかにマイグレーションするほうが今度の展開的によい気がする


追記:
カラム定義変更が難しい理由ってあるっけ(DBのマイグレーションに時間がかかるのはダウンタイムを予告しておけばいいのではないかなあ)

@sakuhanight
Copy link
Contributor

カラム定義変更が難しい理由

ClientServerService.tsなど、フィールドがnullであることを期待する処理を全て対応させる必要があるのではないでしょうか。

@sakuhanight
Copy link
Contributor

関連すると思われるので貼っておきます。#15097

全体的に空文字の取り回しを見直す必要がある気がしています。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛Bug Unexpected behavior [Feat] ControlPanel Issues related to existing functionality, such as bugs or adding small features. packages/frontend Client side specific issue/PR
Projects
None yet
Development

No branches or pull requests

4 participants