Skip to content

Conversation

@opqdonut
Copy link
Member

No description provided.

it is an old feature, but didn't have a test, so it was broken by #715

also add a test so we don't break it again
(ring/router
["/foo" {:post {:responses {200 {:content {:default {:schema schema-200}}}
201 {:content {"application/edn" {:schema schema-200}}}
202 {:description "status code explicitly mentioned, but no :content map"}
Copy link
Member Author

Choose a reason for hiding this comment

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

The interaction here between :content and :default will probably mislead users.

  • If a status code is listed, but has no :content, the coercion for the :default status will apply
  • If a response code has a :content, it implicitly means non-mentioned content-types will have no coercion at all

It would be clearer, IMO, to have the :default response only apply when the status code isn't listed. That is, in this case, status 202 should have no response coercion at all, for any content type.

Choose a reason for hiding this comment

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

Yeah, I agree that the :default response should only apply if there is an unlisted status code.

As in #691 , it would be good to explicitly and separately specify and document the recommended way of specifying that an empty response body is expected.

@opqdonut
Copy link
Member Author

opqdonut commented Apr 11, 2025

This is currently an ok fix for the feature we broke in #715, but I think there's an opportunity to clarify even more stuff here.

We could also address #691

@lispyclouds
Copy link

Hey @opqdonut would there be a plan to release this? Hoping to get to update to the new reitit on some of my projects, not urgent but great to have it! Thanks again!

@opqdonut
Copy link
Member Author

I'll try to have some time to look at this, latest next week. Feel free to ping me if you don't hear from me.

@opqdonut
Copy link
Member Author

opqdonut commented Apr 24, 2025

Jotting down some notes for myself.

Lookup order for coercions should be:

  1. find status code in :responses, or if not present, look at :responses :default
  2. from there, find the content-type in :content, or look at :content :default, or :body
  3. perform the coercion specified by :schema (or, in the case of :body, the value of :body). if there is no schema, do no coercion.

However, we should still be able to disable coercion with something like (reitit.coercion.malli/create {:enabled false}). This works currently by returning nil for the coercer, so nil means "don't coerce". However, other places use -identity-coercer to mean "don't coerce."

The OpenAPI semantics however use a lack of :schema to mean "no response allowed", so that's in conflict with reitit, which uses it to mean "anything goes".

@bombaywalla
Copy link

In Reitit, how do I specify that the :body must be empty? (in contrast to ":body may be anything").

@opqdonut
Copy link
Member Author

Currently, the only way to do that is to use a schema that admits only nil. So for example {:schema nil?} works for malli & spec.

@opqdonut opqdonut marked this pull request as ready for review April 28, 2025 07:05
@opqdonut
Copy link
Member Author

I think I managed to cover all the corners. The default resolution order is now like I planned in my message above (see also: doc changes in the commits)

@lispyclouds
Copy link

Looks great to me, thanks a lot for the effort!

@opqdonut opqdonut merged commit 7a77c9f into master Apr 29, 2025
12 checks passed
@opqdonut opqdonut deleted the response-default branch April 29, 2025 06:30
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.

5 participants