Backstage renovate bot failing upgrade to 0.53

[sennyeya]

Describe the bug When default response is defined and a 500 error is sent by the server, Optic notes some fake query parameters that don't exist on the schema. Example logs, https://github.com/backstage/backstage/actions/runs/6981202604/job/18997968192?pr=21543 and the relevant schema, https://github.com/backstage/backstage/blob/542d19100dc054168561ca704ce287018932f5ea/plugins/search-backend/src/schema/openapi.yaml#L72.

To Reproduce see above branch

Expected behavior This should not throw and should retain the same behavior as 0.50.10.

[acunniffe]

Taking a look at this -- very strange behavior. Which ones are "fake"?

[sennyeya]

@acunniffe Thanks! All of them, here's the test, https://github.com/backstage/backstage/blob/542d19100dc054168561ca704ce287018932f5ea/plugins/search-backend/src/service/router.test.ts#L82, no query parameters defined at all.

[acunniffe]

@sennyeya Here's what I know so far. The 'phantom' query parameters aren't hardcoded anywhere in our project. That rules out some test code leaking them into production differ.

I'm also seeing the query parameters that Optic says are not documented appearing https://github.com/backstage/backstage/blob/542d19100dc054168561ca704ce287018932f5ea/plugins/search-backend/src/service/router.test.ts#L105

I think what's happening is that Optic is picking up all those query parameters because they appear in some of the tests, even though they are not in the one you shared. Maybe tou can test that theory by running just this test

    it.only('throws meaningful query errors', async () => {

[sennyeya]

@acunniffe 🤦 that's my bad, I should have looked through the rest of the tests. Using it.only works.

I think this is an issue with my spec, https://github.com/backstage/backstage/blob/542d19100dc054168561ca704ce287018932f5ea/plugins/search-backend/src/service/router.test.ts#L217 looks really weird to me. Give me a little while to update the spec and retry, I don't think this is an issue with Optic and actually a nice catch by it!

[sennyeya]

@acunniffe A little more looking later,

1. The way the Backstage library is creating query parameters and denoting them is really non-standard, using a[0]=x&a[1]=y, which is supported by the qs library but not by OpenAPI. I will adjust this to be more standard, as currently this is odd behavior. Using a=x&a=y makes the query parameter recognized by Optic.
2. Related to the above, I think Optic isn't handing the [] notation as expected though, for example,

- name: filters
          in: query
          required: false
          style: deepObject
          explode: true
          schema:
            type: object
            additionalProperties: true

is one of the query parameters defined for the endpoint and ideally would support any filters[val]=test or filter[prop]=test, but Optic outputs [query parameter] filters[prop] is not documented which seems a little odd. Similarly with

- name: filters
          in: query
          required: false
          style: deepObject
          explode: true
          schema:
            type: object
            properties:
              prop:
                type: string

3. How would I go about denoting variable query parameters? This feels painful from a spec definition perspective, but basically we're supporting additional undefined query parameters to this endpoint, https://github.com/backstage/backstage/blob/542d19100dc054168561ca704ce287018932f5ea/plugins/search-backend/src/service/router.ts#L156. I found this Stack Overflow article that mentions it, https://stackoverflow.com/questions/49582559/how-to-document-dynamic-query-parameter-names-in-openapi-swagger, but that seems rough for validation and Optic currently doesn't support it. Thoughts? Is this something that could be added to Optic?

[sennyeya]

Should also add that this isn't super time sensitive, I'm happy to keep using 0.50.10 for the time being 😁

[acunniffe]

Hey @sennyeya -- oh yeah that is definitely a new one for me. I've seen the [] notation for arrays, but never mixed with a variable. I think we could look into supporting explode style. We probably need a few days to check with some of the larger users on what they do. If we make changes here we want to make sure it supports all the current / (many) future cases here.

What changed from 0.50.10 is that we shipped query parameter and header support 👍 -- which is a big step forward for the tool's validation / generation abilities. If it's not quite ready for you yet, feel free to stay pinned. Can I circle back next week w/ more once we talk to other users?

[sennyeya]

@acunniffe Ah, that's a huge step forward for the tool 😁 ! Hadn't realized that was what changed 🤦 . For sure, take your time, I'm happy with the current state of things and this would be nice to have on top.