Skip to content

Add valibot for parameter validation in /post#539

Draft
SnO2WMaN wants to merge 2 commits into
masterfrom
valibot-api-payload
Draft

Add valibot for parameter validation in /post#539
SnO2WMaN wants to merge 2 commits into
masterfrom
valibot-api-payload

Conversation

@SnO2WMaN
Copy link
Copy Markdown
Collaborator

@SnO2WMaN SnO2WMaN commented Apr 16, 2026

方針としてはこのようなvalidatorを入れてsearchParamsをチェックする.段階的にやるので,とりあえず1ページのみ.

@SnO2WMaN SnO2WMaN marked this pull request as ready for review April 16, 2026 00:51
@SnO2WMaN SnO2WMaN requested a review from lachrymaLF April 16, 2026 00:53
@lachrymaLF
Copy link
Copy Markdown
Member

I'd like to see if there's a way to solve this together inside #538. Let us discuss a little more before proceeding with this because doing the same validation twice feels very wrong to me.

@lachrymaLF lachrymaLF marked this pull request as draft April 17, 2026 00:06
@SnO2WMaN
Copy link
Copy Markdown
Collaborator Author

SnO2WMaN commented Apr 17, 2026

「2回やる」の意味がわからない,このPRは単に params のバリデーションをしているだけで,#538 とは関係がないし,一緒にやる理由も無いと思う.

@SnO2WMaN SnO2WMaN marked this pull request as ready for review April 17, 2026 01:24
@lachrymaLF
Copy link
Copy Markdown
Member

lachrymaLF commented Apr 17, 2026

It's being validated another time when it arrives on the backend (which is where it actually matters) -- at which point the error should be able to be forwarded if needed (hence this ties into #538). In 100% of the cases the logic is the exact same. I currently think we should not be introducing more code for this in the metaframework. There are areas where we still might want to do more checks such as display params, so let's spend a bit more time discussing this before going forward.

@lachrymaLF lachrymaLF marked this pull request as draft April 17, 2026 01:29
@SnO2WMaN
Copy link
Copy Markdown
Collaborator Author

今回はそのケースではないが,私見では以下を修正していきたい.

  • 現状でasなどによる無理やりな型合わせがあり,パラメータのチェックは結構杜撰な感じになっている.server.tsでそれが完結しているならそれでも良いが,いくつかのパラメータはpage.tsにも直接渡されているため安全ではないと感じる.
  • APIリクエストをする前に論外な入力は弾いておきたい.これはパフォーマンス的な話で,ここまで神経質になる必要はないと思うが.

@lachrymaLF
Copy link
Copy Markdown
Member

  • I'm not entirely certain what sort of safety you are referring to when the code is passing from server.ts to page.svelte, since the types are transparently carried over in data. I would be happy if you can elaborate on this.
  • Otherwise it seems to me that validating values before sending requests to backend does not solve any real problem but comes with a massive downside:
    1. My issue with this is less about performance since when the data even reaches the metaframework it would already have made its way through external HTTP.
    2. It's more so about code duplication and the additional necessity to synchronize the same constraints from the API spec. You should see that covers an extremely large code surface.
      • This is a lot of burden for someone trying to implement any new feature since now they need to write validation logic twice, which is a huge downside.
      • If an API is changed, the validation logic needs to be changed in the endpoint as well as at every callsite.
    3. Since they don't introduce anything useful (the exact same checks must occur on the backend to begin with), the existence of pre-flight checks is completely transparent to any end user. In other words, it is quite literally pointless, so the benefit of doing this is...?

@SnO2WMaN
Copy link
Copy Markdown
Collaborator Author

今回の修正のケースではないが,例えばのケースとしてcategoryのIDが"-420"みたいな値であったとしてもparseIntを通すだけで未チェックみたいなケースがあったはずで,ちゃんとCategoryIdとして1 から 6であることをserver.tsで確かめておいてほしい,みたいなケースが想定される.
サーバー側のpayloadとして一回確かめておけばいいだけの話なのでどちらかというとpage.tsで一々そのことを確かめる処理を消したい,みたいなモチベーションだ.

バックエンド側の制約条件と厳密に合わせる必要はなく,型として有用な程度の最小限の制約条件で良いのでそれほどコストはかからないと思う.

@lachrymaLF
Copy link
Copy Markdown
Member

I'm going to veto this for now. The complexity is low for that kind of check and doesn't warrant an entire validation library. Two or three helpers our own can make this ergonomic enough. I'm still not convinced about pre-flight checks.

@lachrymaLF lachrymaLF closed this Apr 17, 2026
@lachrymaLF lachrymaLF deleted the valibot-api-payload branch April 18, 2026 13:53
@SnO2WMaN SnO2WMaN restored the valibot-api-payload branch April 25, 2026 23:33
@SnO2WMaN SnO2WMaN reopened this Apr 25, 2026
@SnO2WMaN
Copy link
Copy Markdown
Collaborator Author

SnO2WMaN commented Apr 25, 2026

そもそも型以前の問題としてJSとPythonで意味論が違う以上,2x-checkだとしても最低限のvalidationは入れるべきだと思い直してきた.
ヘルパー関数を導入するのも良いが,もう既にライブラリとして使いやすいものがある以上zodかvalibotを使っていくのが良いと思う.

@lachrymaLF
Copy link
Copy Markdown
Member

I don't agree. I suggest that you talk to us about this before working on this since it would otherwise be vetoed again.

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