Implement an OpenAPIComparator

[abdelfetah18]

I've started implementing OpenAPIComparator based on the structure outlined in the other issue on the tapir project: softwaremill/tapir#3645 (comment)

I've made some progress, but I'd appreciate feedback to ensure I'm heading in the right direction.

[abdelfetah18]

@adamw, I was unable to add tests for reading .yaml or .json files because openapi-circe depends on openapi-model. This dependency prevents openapi-circe from being used within openapi-model, which means I don't have access to the necessary decoders.

[adamw]

@abdelfetah18 let's then maybe create an openapi-comparator-tests module (not published), which will depend on both model & circe, and contain only the tests? I think such specification-based tests will have most value

[abdelfetah18]

Can you review this? It's almost done, except for encoding, callbacks, and security. The tests are now using OpenAPI JSON files

[adamw]

Apart from the naming issues, I think what I'm missing is positive test cases (different schemas, but compatible).

Another thing: evolution of the request body, e.g. adding a new variant to a coproduct. Both in the request, and response bodies. Depending where the variant is added (client/server and response/request), we'll get compatible, or incompatible changes

[adamw]

Another missing test case: changing metadata - adding or removing examples, description itd - this should not influence compatibility

[adamw]

Yet another: adding an alternative media type to the response. E.g. there's an application/json, but now the server can also return XML. Or the client can send XML.

[adamw]

And one more: introducing new response codes. E.g. 200 was defined, now additionally 400 and 401 are defined

[adamw]

Is changing the parameter's name a compatible change? E.g. was /pet/{petId}, now is: /pet/{id}?

[abdelfetah18]

Is changing the parameter's name a compatible change? E.g. was /pet/{petId}, now is: /pet/{id}?

No, changing the parameter's name from /pet/{petId} to /pet/{id} is not a compatible change

[adamw]

ok, so we should test this as well

[abdelfetah18]

Yet another: adding an alternative media type to the response. E.g. there's an application/json, but now the server can also return XML. Or the client can send XML.

And one more: introducing new response codes. E.g. 200 was defined, now additionally 400 and 401 are defined

I've already tested them. one is missing response content media type and the other is missing response.

I think I should rename the test cases because they’re unclear.

[adamw]

we definitely shouldn't have mutable state that's shared among a couple of method invocations. This makes an OpenAPIComparator instance not re-usable. We either need to pass the state explicitly, or create a private mutable comparator instance which will be single-use

[adamw]

Let's add a test that you can reuse the comparator instance

[abdelfetah18]

I have updated the test file names and resolved all issues except for the following two:

we definitely shouldn't have mutable state that's shared among a couple of method invocations. This makes an OpenAPIComparator instance not re-usable. We either need to pass the state explicitly, or create a private mutable comparator instance which will be single-use

Let's add a test that you can reuse the comparator instance

I cannot make the OpenAPIComparator instance reusable because the schemas map is stored in (client/server)OpenApi.components.schemas. To access it, I need the (server/client)OpenApi instance. If I pass the (server/client)OpenApi instance through the compare method, I won't be able to access the schemas in the checkSchemas method unless I explicitly pass them down during method invocation, which doesn't seem ideal.

Apart from the naming issues, I think what I'm missing is positive test cases (different schemas, but compatible).

Another thing: evolution of the request body, e.g. adding a new variant to a coproduct. Both in the request, and response bodies. Depending where the variant is added (client/server and response/request), we'll get compatible, or incompatible changes

Could you provide an example of a positive test case? Currently, we have the identical test case, and other tests where swapping the order of client/server results in no issues.

[adamw]

hm so here I'm wondering if the hierarchy is correct - we've got Issues and SubIssues, which are also Issues. Can IncompatibleParameter be used as a top-level issue, or only as a sub-issue of IncompatibleRequiredParameter? Should SubIssue extend Issue?

[abdelfetah18]

This is a naming issue—my apologies. What I intended to convey is that the required value is set to true on the client side and false on the server side. However, this concept isn't limited to parameters. it can also apply to other components, such as RequestBody and Header.

I will generalize the class for use in different contexts, similar to how the style attribute is handled.

case class IncompatibleRequiredValue(
    clientValue: Boolean,
    serverValue: Boolean
) extends OpenAPICompatibilityIssue {
  def description: String =
    s"required value mismatch: client has $clientValue, but server has $serverValue"
}

The IncompatibleRequiredValue will simply be a sub-issue of IncompatibleParameter.

[adamw]

so IncompatibleRequiredValue can be used as a top-level issue as well? That is, it can be used in two contexts: as a top-level and sub-level issue?

[adamw]

isn't it an incompatibility if one schema is provided, and the other is not?

[abdelfetah18]

Should I use MissingClientSchema and MissingServerSchema, or should I simply check if the schema is missing from the server and use just MissingSchema?

[adamw]

I guess just MissingSchema is enough

[ghik]

OpenAPI requires that a Parameter or Header object defines either a schema or content with exactly one entry. I guess it could technically be possible for two parameters/headers to be compatible when one of them has a schema while the other one has a content?

[adamw]

@abdelfetah18 thanks, this looks good, there's a couple of TODOs left in code still, though?

[abdelfetah18]

@abdelfetah18 thanks, this looks good, there's a couple of TODOs left in code still, though?

Yes, I'll take care of those TODOs and put them in a separate commit to make it easier for you to review.

[abdelfetah18]

I’ve started working on encoding compatibility issues and will push the changes tomorrow once it’s done, including callbacks and security compatibility. For now, I’ve fixed all the previous issues.

Regarding IncompatibleRequiredValue, I noticed I hadn't written tests for the required value. I'll add those tests tomorrow as well.

[abdelfetah18]

I’m done except for the callbacks. I’m encountering an issue where the callback path items are not being loaded correctly.

I’ve declared some callbacks in the OpenAPI definition, but when I try to print them, the callback path items are missing. It seems like this might be an encoding issue, causing the paths not to be properly parsed or included when the file is read.

Here’s my test file for reference:

{
  "openapi": "3.0.3",
  "info": {
    "title": "Simple Pet Store API",
    "version": "1.0.0"
  },
  "paths": {
    "/pets": {
      "post": {
        "summary": "Add a new pet",
        "requestBody": {
          "required": true,
          "content": {
            "application/json": {
              "schema": {
                "type": "object",
                "properties": {
                  "id": {
                    "type": "integer"
                  },
                  "name": {
                    "type": "string"
                  },
                  "tag": {
                    "type": "string"
                  }
                }
              }
            }
          }
        },
        "responses": {
          "201": {
            "description": "Pet created successfully."
          }
        },
        "callbacks": {
          "onPetStatusChange": {
            "{$request.body#/callbackUrl}": {
              "post": {
                "summary": "Notify about pet status change",
                "description": "This callback is triggered whenever the status of a pet changes.",
                "requestBody": {
                  "required": true,
                  "content": {
                    "application/json": {
                      "schema": {
                        "type": "object",
                        "properties": {
                          "status": {
                            "type": "string",
                            "description": "The new status of the pet.",
                            "enum": [
                              "available",
                              "pending",
                              "sold"
                            ]
                          },
                          "timestamp": {
                            "type": "string",
                            "format": "date-time",
                            "description": "The time when the status change occurred."
                          },
                          "petId": {
                            "type": "integer",
                            "description": "The unique ID of the pet whose status changed."
                          }
                        },
                        "required": [
                          "status",
                          "timestamp",
                          "petId"
                        ]
                      }
                    }
                  }
                },
                "responses": {
                  "200": {
                    "description": "Callback successfully processed."
                  },
                  "400": {
                    "description": "Invalid payload provided.",
                    "content": {
                      "application/json": {
                        "schema": {
                          "type": "object",
                          "properties": {
                            "error": {
                              "type": "string",
                              "description": "Details about the error."
                            }
                          }
                        }
                      }
                    }
                  }
                }
              }
            }
          }
        }
      }
    }
  }
}

[adamw]

@abdelfetah18 maybe the circe decoder is incorrect? let's create an issue to fix this (both the decoder & the comparator), and skip callbacks for now

[adamw]

Almost done, thanks! :) I needed to increase the memory for the tests to pass (which is weird ... but probably another issue: #197)