Skip to content

fix: Support for multiple response content-types see #436#439

Open
mryellow wants to merge 4 commits intoMermade:mainfrom
MomsFriendlyDevCo:feat-multiresponse
Open

fix: Support for multiple response content-types see #436#439
mryellow wants to merge 4 commits intoMermade:mainfrom
MomsFriendlyDevCo:feat-multiresponse

Conversation

@mryellow
Copy link
Copy Markdown

It could be this simple.

However code and description are in the outer object and don't require calculation each loop.

I'll push another commit which does the logic on those first then reuses the result for the inner loop.

@mryellow
Copy link
Copy Markdown
Author

May as well include here a patch for #437 which uses your patterns for safeType in getParameters.


let ctSchema = contentType.schema;
if (ctSchema) {
entry.type = ctSchema.type;
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Superfluous.

I guess is nice having format as well but seeing we're using ctSchema.type below this point and entry.originalType is being set, there is no need to assign entry.type.

@MikeRalphson
Copy link
Copy Markdown
Contributor

Thanks, looking forward to seeing the final version. Where (say) a JSON and XML response contentType refer to the same schema, I'd rather not duplicate it in the output markdown if possible...

@mryellow
Copy link
Copy Markdown
Author

JSON and XML response contentType refer to the same schema

The schema is a child of the content-types.

path -> method -> response code -> content-type -> schema

In this inline example the schema are both type: 'object' but definitely separate instances with other potential differences in properties or descriptions inside that object.

{
	"paths": {
		"/api/endpoint": {
			"post": {
				"description": "",
				"responses": {
					"200": {
						"description": "",
						"content": {
							"application/xml": {
								"schema": {
									"type": "object"
								}
							},
							"application/json": {
								"schema": {
									"type": "object"
								}
							}
						}
					}
				}
			}
		}
	}
}

If they were the same you'd probably link them to the same $ref right?

"content": {
	"application/xml": {
		"schema": {
			"$ref": "#/components/schemas/Custom"
		}
	},
	"application/json": {
		"schema": {
			"$ref": "#/components/schemas/Custom"
		}
	}
}

I'm looking at an endpoint where one response is an inline PDF (string(binary)) and the other a component JSON schema.

"content": {
	"application/pdf": {
		"schema": {
			"type": "string",
			"format": "binary"
		}
	},
	"application/json": {
		"schema": {
			"$ref": "#/components/schemas/Custom"
		}
	}
}

@mryellow
Copy link
Copy Markdown
Author

If they were the same you'd probably link them to the same $ref right?

Perhaps you're wishing to hide the contentType column whenever the schema inside every item has the same $ref value specified?

That would probably take 2 tables in templates/openapi3/responses.def with a flag to switch between them.

Might be a bridge too far, sounds like a visual improvement which someone else might wish to add.

I'm just wishing to fix the bugs:

  • It only displays the last key in the responses.
  • The format is dropped.

It just needs to display stuff which meets OpenAPI spec, prettifying it by removing keys which are considered noisy is something else.

@MikeRalphson
Copy link
Copy Markdown
Contributor

The schema is a child of the content-types.
path -> method -> response code -> content-type -> schema

Yes, I know how the OAS is structured 😄

Perhaps you're wishing to hide the contentType column whenever the schema inside every item has the same $ref value specified?

Exactly this. Merge the contentTypes but show multiple (two in this case) examples in the code blocks (which get put in the right-hand column in Slate).

Might be a bridge too far, sounds like a visual improvement which someone else might wish to add.
I'm just wishing to fix the bugs:

* It only displays the last key in the responses.
* The format is dropped.

That's fair.

It just needs to display stuff which meets OpenAPI spec, prettifying it by removing keys which are considered noisy is something else.

Agreed, we can iterate on top of this PR, and keep it focussed.

@MikeRalphson MikeRalphson self-assigned this Apr 14, 2021
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