Skip to content
This repository has been archived by the owner on Oct 20, 2023. It is now read-only.

Adds support for returning the same error across multiple status codes #250

Merged
merged 2 commits into from
Mar 7, 2023
Merged

Adds support for returning the same error across multiple status codes #250

merged 2 commits into from
Mar 7, 2023

Conversation

alycklama
Copy link
Contributor

I noticed that there's an issue when returning the same error for multiple status codes like below:

openapi: 3.0.2
info:
  title: Single Error
  version: "1.0"
paths:
  /person:
    put:
      summary: Update an existing person
      description: Update an existing person by Id
      operationId: updatePerson
      responses:
        "400":
          description: Returns a 400
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/ErrorModel"
        "401":
          description: Returns a 401
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/ErrorModel"
components:
  schemas:
    ErrorModel:
      required:
        - msg
      type: object
      properties:
        msg:
          type: string

This will not compile, as it will try to extend the ErrorModel twice:

sealed trait GetRootGenericError
case class ErrorMessage(message: String)
    extends GetRootGenericError()
    with GetRootGenericError()

This PR fixes this issue by creating the coproducts based on the distinct error responses.

@alycklama
Copy link
Contributor Author

alycklama commented Feb 28, 2023

Doing a distinct on the $ref might be safer. However, that would also mean that one definition gets overwritten if there are differences. What would you prefer in this situation? A compile error, or that one of the definitions gets overwritten? The latter might be a lot harder to debug.

@alycklama
Copy link
Contributor Author

@ghostbuster91 I wanted to let you know that this PR is open for review. No need to rush, though! If there are any concerns on your side, please let me know.

@ghostbuster91
Copy link
Owner

@alycklama thanks, and no worries I saw it. I will review it today/tomorrow :) Looks good at the first glance 👍

@ghostbuster91
Copy link
Owner

ghostbuster91 commented Mar 4, 2023

I went through the PR, good job 👍

Doing a distinct on the $ref might be safer. However, that would also mean that one definition gets overwritten if there are differences.

You mean distincBy(_.$ref)? Is there any way where two instances of a coproduct created form the the same ref could differ? Or are we talking about a theoretical possibility of having a difference by using such a construct? I think that doing distinct is better here as by that time the model we operate on should be free from low-level open-api nuances. Which will be true once I rewrite the parser.

A compile error, or that one of the definitions gets overwritten?

In general a compile error is better than doing something silently behind users back. We could check here if: distinctBy(_.$ref) == distinct and fail if that doesn't hold. If anyone encounters that they know it immediately.

@alycklama
Copy link
Contributor Author

Or are we talking about a theoretical possibility of having a difference by using such a construct? I think that doing distinct is better here as by that time the model we operate on should be free from low-level open-api nuances. Which will be true once I rewrite the parser.

It's indeed a theoretical possibility. I noticed that the SafeSchema has classes that might not be comparable, but I didn't want to dive into the entire hierarchy to figure out this possibility (for example, jsonSchemaImpl is an Object).

I added an extra check that deals with this in a commit that I just pushed.

@ghostbuster91
Copy link
Owner

Makes sense 👍 Thanks for the PR!

@ghostbuster91 ghostbuster91 merged commit f9f2e66 into ghostbuster91:master Mar 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants