Validation breaks when schema is a reference

[dafeder]

Package version
0.12

Describe the bug
I have a spec whose relevant section looks like this:

    "/api/1/metastore/schemas/dataset/items/{identifier}": {
      "get": {
        "operationId": "dataset-get-item",
        "summary": "Get a single dataset.",
        "tags": [
          "Metastore: dataset"
        ],
        "parameters": [
          {
            "$ref": "#/components/parameters/datasetUuid"
          },
          {
            "$ref": "#/components/parameters/showReferenceIds"
          }
        ],
        "responses": {
          "200": {
            "description": "Full dataset item.",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/dataset"
                }
              }
            }
          },
          "404": {
            "$ref": "#/components/responses/404IdNotFound"
          }
        }
      },

When I upgrade to v0.12, the SchemaValidator breaks with:

TypeError: League\OpenAPIValidation\Schema\SchemaValidator::validate(): Argument #2 ($schema) must be of type cebe\openapi\spec\Schema, cebe\openapi\spec\Reference given, called in /var/www/html/vendor/league/openapi-psr7-validator/src/PSR7/Validators/BodyValidator/UnipartValidator.php on line 58

I believe this is because the schema is a reference, and in #52 the ValidatorBuilder now passes a specObject from Reader instead of.. I'm not sure what, but whatever it was doing before. Now validate() fails because it's getting a Reference, which previously must have somehow been resolving to a Schema.

To Reproduce
Sorry I'm not sure the best way to tell you to how to reproduce it.

Expected behaviour
Specs that provide response schemas via references continue to work for validation.

[osteel]

Hi, thank you for reporting.

Sorry I'm not sure the best way to tell you to how to reproduce it

What would be amazing, if you've got time, is if you could open a PR updating this file to include something that fails with v0.12 as per your use case, but passes with v0.11 (this file is used for this package's unit tests). And I'll take it from there.

I'll try and have a look some time next week

[osteel]

Just ran a quick test by moving some of the OpenAPI definition's components to references (a property and an entire response's schema), and I'm not encountering any issue so far. That's all the time I can dedicate to this right now I'm afraid – like I said, if you could provide me with a failing test case by updating example.json in a PR so I can have a look next week, it'd be amazing 🙏

[dafeder]

Thanks for the response, I'll get to this early next week.

[dafeder]

OK still debugging here, it looks like you are right, in a very isolated test with the same spec I don't get an error. Sorry, I should have tested more before submitting an issue. Will update when I figure it out.

[dafeder]

@osteel ok I see the issue. Now that a specObject is instantiated with cebe\openapi\Reader::readFromJson[File]() before building the validator, it's inheriting a behavior from that library where references are resolved when reading JSON files, but not when reading simple JSON strings. You can see this here: https://github.com/cebe/php-openapi/blob/1.8.0/src/Reader.php#L95

Now that I get exactly what's happening, it seems fairly simple to understand, but on the way I created a little repo with an integration test that demonstrates this, and how the behavior changes from v0.11 to v0.12 if you'd like to look:

https://github.com/dafeder/osteel-openapi-test

My use case is maybe a bit of an edge, as our spec is not a static file but dynamically generated. I wonder if you think it would be reasonable to call $specObject->resolveReferences() in your ValidatorBuilder just in case, or if this feels out of scope and it's on me to, say, write my spec to a temporary file as part of my test before building a validator.

[osteel]

Hi @dafeder, this is great, thank you. If you can keep that repo open for now, I'll try and have a look in the next few days and I'll let you know. At first glance though, as this is a breaking change, however rare it might be, it'd make sense for me to fix it

[dafeder]

Thinking about it some more, I think the reason the cebe library works like this is that if there's no path or URI for the spec, some relative references could be unresolveable. In my case the references are all just relative to the document root, not the file, but JSON refs can have relative paths to other files. So there could be unwanted effects of always resolving refs in the context of a string. OTOH one could say that it's up to the downstream user of this (the osteel) library not to pass a JSON string with relative filesystem references 🤷🏽

[osteel]

Hey @dafeder, not sure you saw this?

Let me know if you really don't have time – I'm pretty sure it fixes the issue, so could probably just merge it and release it. I'm just being a bit paranoid 🙂

[dafeder]

Hi @osteel, sorry just getting back from Thanksgiving break here in the US. This looks like it works for us!