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

Validation only when body content is present #290

Closed
wants to merge 3 commits into from

Conversation

koriym
Copy link
Member

@koriym koriym commented Jan 20, 2024

bodyの値が[]の時にJsonSchemaによるチェックを行わない。

/item?id=100として該当のアイテムがなければ[]が変えるがJsonSchemaを使って空配列を許容するのはOneOfを使う必要があり煩雑になるため。

代替案としてはJsonSchemaアトリビュートに空配列を許すようなパラメータの導入。例えばallow_empty:nullなど。

Copy link

codecov bot commented Jan 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (246016b) 100.00% compared to head (09fa110) 100.00%.
Report is 18 commits behind head on 1.x.

Additional details and impacted files
@@             Coverage Diff              @@
##                 1.x      #290    +/-   ##
============================================
  Coverage     100.00%   100.00%            
- Complexity       509       614   +105     
============================================
  Files             85        85            
  Lines           1362      1539   +177     
============================================
+ Hits            1362      1539   +177     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@koriym koriym requested review from jingu and NaokiTsuchiya January 20, 2024 08:10
@koriym koriym marked this pull request as ready for review January 20, 2024 08:10
@NaokiTsuchiya
Copy link
Contributor

/item?id=100として該当のアイテムがなければ[]が変えるがJsonSchemaを使って空配列を許容するのはOneOfを使う必要があり煩雑になるため。

もう少し具体的な例はありますかね?

今の実装だと、 code が 200 or 201 の場合にのみJsonSchemaによるチェックが行われますが、/item?id=100 としたときに、body が [] でないケースと [] となるケースが混在する状況があまりピンときていません。

@koriym
Copy link
Member Author

koriym commented Mar 20, 2024

説明が明瞭じゃないですね。反省。思い出しながら説明するに

ResourceObject::bodyはデフォルトがnullです。DBをセレクトして結果がある時だけセットするとすれば、バリデーションする時にアイテム1つ以上の配列かnullのどちらかという、"OneOf"を書かないといけなくてならなくて煩雑かと考えました。

@NaokiTsuchiya
Copy link
Contributor

説明が明瞭じゃないですね。反省。思い出しながら説明するに

ResourceObject::bodyはデフォルトがnullです。DBをセレクトして結果がある時だけセットするとすれば、バリデーションする時にアイテム1つ以上の配列かnullのどちらかという、"OneOf"を書かないといけなくてならなくて煩雑かと考えました。

なるほど。
なんとなく理解できました。

この変更で、以下が明確になるテストがあると良さそうに思いました。

  • これまでエラーになっていたが、エラーにならなくなるもの
  • これまでエラーにならなかったが、エラーになるもの

@koriym
Copy link
Member Author

koriym commented Mar 22, 2024

空だったら404にしてバリデーション回避する運用の話を聞き一旦クローズします。

@koriym koriym closed this Mar 22, 2024
@koriym koriym deleted the no_json_validate_on_empty branch March 22, 2024 16:23
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