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

fix: throw if response status is not int #715

Merged
merged 1 commit into from
Feb 25, 2025

Conversation

filipesilva
Copy link
Contributor

Fix #667

@filipesilva
Copy link
Contributor Author

@ikitommi lmk if you want this somewhere else, or using another approach. I put the test in reitit.ring.middleware.exception-test because that's the place that actually uses reitit.coercion/response-coercers.

@ikitommi ikitommi requested a review from opqdonut February 16, 2025 10:35
@ikitommi
Copy link
Member

proposing @opqdonut as reviewer.

@@ -125,7 +125,7 @@
(ring/router
["/api"
["/plus/:e"
{:get {:responses {"200" {}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was actually using the wrong format.

Copy link
Member

@opqdonut opqdonut Feb 25, 2025

Choose a reason for hiding this comment

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

This was actually a test case that was meant to throw an error due to reitit.ring.spec/validate. Now it throws your new error because it seems the response coercion compilation happens before the :validate.

I propose you remove your latest commit and change this test case so that it fails in :validate, for example:

{:get {:responses {200 {:description 1}}
       :handler identity}}

(edit: fixed the suggestion)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used your suggestion.

But it does beg the question, am I fixing the wrong thing? If reitit.ring.spec/validate already caught the non-int response codes, maybe that's what I should be using in the first place. But then why is the validation optional?

Copy link
Member

Choose a reason for hiding this comment

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

Good question re: validation being optional. I think this added assert is a good addition anyway, though you are right that it is a bit redundant.

Copy link
Member

@opqdonut opqdonut left a comment

Choose a reason for hiding this comment

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

The fix looks good, but the test needs work.

@@ -125,7 +125,7 @@
(ring/router
["/api"
["/plus/:e"
{:get {:responses {"200" {}}
Copy link
Member

@opqdonut opqdonut Feb 25, 2025

Choose a reason for hiding this comment

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

This was actually a test case that was meant to throw an error due to reitit.ring.spec/validate. Now it throws your new error because it seems the response coercion compilation happens before the :validate.

I propose you remove your latest commit and change this test case so that it fails in :validate, for example:

{:get {:responses {200 {:description 1}}
       :handler identity}}

(edit: fixed the suggestion)

@opqdonut opqdonut merged commit d5d46d5 into metosin:master Feb 25, 2025
7 checks passed
@vemv
Copy link

vemv commented Feb 25, 2025

Thanks, all!

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.

reitit.coercion.malli - invalid keys for :responses are accepted
4 participants