Skip to content

Validate swagger#4295

Merged
fisx merged 12 commits intodevelopfrom
validate-swagger
Oct 23, 2024
Merged

Validate swagger#4295
fisx merged 12 commits intodevelopfrom
validate-swagger

Conversation

@pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Oct 17, 2024

Fix openapi validation errors reported by https://editor.swagger.io/. There are some leftover failures related to DELETE endpoints with bodies, which AFAIU will not be considered errors anymore in openapi 3.1.

This PR also fixes issues reported by vacuum. We are running version 0.12.0. of the tool, because the latest version is crashing with a null pointer dereference error.

https://wearezeta.atlassian.net/browse/WPB-10314

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@echoes-hq echoes-hq bot added the echoes: unplanned Any work item that isn’t part of the product or technical roadmap. label Oct 17, 2024
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Oct 17, 2024
@pcapriotti pcapriotti marked this pull request as ready for review October 22, 2024 08:36
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

looks good to go to me!

i get 31 errors with vaccum locally now (maybe the nix derivation is older/newer than the docker image?), but that doesn't get in your way anywhere, you have to call a new make rule to see them.

so this can be fixed elsewhere.


instance (KnownSymbol a) => RenderableSymbol a where
renderSymbol = T.pack . show $ symbolVal (Proxy @a)
renderOperationId = T.pack $ symbolVal (Proxy @a)
Copy link
Contributor

Choose a reason for hiding this comment

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

is renderSymbol still used now? if not you could just change its behavior instead of adding another method to the class.

@fisx
Copy link
Contributor

fisx commented Oct 22, 2024

for the record:

/dev/fd/63:1:194116 | error    | paths are ambiguous with one another: `/conversations/one2one/{usr_domain}/{usr}...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{cnv_domain}/{cnv}/access']
/dev/fd/63:1:196356 | error    | paths are ambiguous with one another: `/conversations/one2one/{usr_domain}/{usr}...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{cnv_domain}/{cnv}/groupinfo']
/dev/fd/63:1:198131 | error    | paths are ambiguous with one another: `/conversations/one2one/{usr_domain}/{usr}...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{cnv_domain}/{cnv}/members']
/dev/fd/63:1:205789 | error    | paths are ambiguous with one another: `/conversations/one2one/{usr_domain}/{usr}...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{cnv_domain}/{cnv}/message-timer']
/dev/fd/63:1:207941 | error    | paths are ambiguous with one another: `/conversations/one2one/{usr_domain}/{usr}...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{cnv_domain}/{cnv}/name']
/dev/fd/63:1:214688 | error    | paths are ambiguous with one another: `/conversations/one2one/{usr_domain}/{usr}...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{cnv_domain}/{cnv}/protocol']
/dev/fd/63:1:217544 | error    | paths are ambiguous with one another: `/conversations/one2one/{usr_domain}/{usr}...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{cnv_domain}/{cnv}/receipt-mode']
/dev/fd/63:1:219742 | error    | paths are ambiguous with one another: `/conversations/one2one/{usr_domain}/{usr}...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{cnv_domain}/{cnv}/self']
/dev/fd/63:1:230362 | error    | paths are ambiguous with one another: `/conversations/one2one/{usr_domain}/{usr}...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{cnv_domain}/{cnv}/typing']
/dev/fd/63:1:241332 | error    | paths are ambiguous with one another: `/conversations/one2one/{usr_domain}/{usr}...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{cnv}/members/{usr}']
/dev/fd/63:1:241332 | error    | paths are ambiguous with one another: `/conversations/{cnv_domain}/{cnv}/access`...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{cnv}/members/{usr}']
/dev/fd/63:1:241332 | error    | paths are ambiguous with one another: `/conversations/{cnv_domain}/{cnv}/groupin...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{cnv}/members/{usr}']
/dev/fd/63:1:241332 | error    | paths are ambiguous with one another: `/conversations/{cnv_domain}/{cnv}/members...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{cnv}/members/{usr}']
/dev/fd/63:1:241332 | error    | paths are ambiguous with one another: `/conversations/{cnv_domain}/{cnv}/message...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{cnv}/members/{usr}']
/dev/fd/63:1:241332 | error    | paths are ambiguous with one another: `/conversations/{cnv_domain}/{cnv}/name` a...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{cnv}/members/{usr}']
/dev/fd/63:1:241332 | error    | paths are ambiguous with one another: `/conversations/{cnv_domain}/{cnv}/protoco...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{cnv}/members/{usr}']
/dev/fd/63:1:241332 | error    | paths are ambiguous with one another: `/conversations/{cnv_domain}/{cnv}/receipt...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{cnv}/members/{usr}']
/dev/fd/63:1:241332 | error    | paths are ambiguous with one another: `/conversations/{cnv_domain}/{cnv}/self` a...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{cnv}/members/{usr}']
/dev/fd/63:1:241332 | error    | paths are ambiguous with one another: `/conversations/{cnv_domain}/{cnv}/typing`...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{cnv}/members/{usr}']
/dev/fd/63:1:259109 | error    | paths are ambiguous with one another: `/conversations/one2one/{usr_domain}/{usr}...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{conv}/bots/{bot}']
/dev/fd/63:1:259109 | error    | paths are ambiguous with one another: `/conversations/{cnv_domain}/{cnv}/access`...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{conv}/bots/{bot}']
/dev/fd/63:1:259109 | error    | paths are ambiguous with one another: `/conversations/{cnv_domain}/{cnv}/groupin...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{conv}/bots/{bot}']
/dev/fd/63:1:259109 | error    | paths are ambiguous with one another: `/conversations/{cnv_domain}/{cnv}/members...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{conv}/bots/{bot}']
/dev/fd/63:1:259109 | error    | paths are ambiguous with one another: `/conversations/{cnv_domain}/{cnv}/message...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{conv}/bots/{bot}']
/dev/fd/63:1:259109 | error    | paths are ambiguous with one another: `/conversations/{cnv_domain}/{cnv}/name` a...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{conv}/bots/{bot}']
/dev/fd/63:1:259109 | error    | paths are ambiguous with one another: `/conversations/{cnv_domain}/{cnv}/protoco...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{conv}/bots/{bot}']
/dev/fd/63:1:259109 | error    | paths are ambiguous with one another: `/conversations/{cnv_domain}/{cnv}/receipt...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{conv}/bots/{bot}']
/dev/fd/63:1:259109 | error    | paths are ambiguous with one another: `/conversations/{cnv_domain}/{cnv}/self` a...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{conv}/bots/{bot}']
/dev/fd/63:1:259109 | error    | paths are ambiguous with one another: `/conversations/{cnv_domain}/{cnv}/typing`...                  | no-ambiguous-paths         | Operations   | $.paths['/conversations/{conv}/bots/{bot}']
/dev/fd/63:1:504929 | error    | paths are ambiguous with one another: `/users/handles/{handle}` and `/users/{uid}/email`             | no-ambiguous-paths         | Operations   | $.paths['/users/{uid}/email']
/dev/fd/63:1:505695 | error    | paths are ambiguous with one another: `/users/handles/{handle}` and `/users/{uid}/rich-info`         | no-ambiguous-paths         | Operations   | $.paths['/users/{uid}/rich-info']

@fisx
Copy link
Contributor

fisx commented Oct 22, 2024

i think those are because handle is also a valid {uid} in /users/handle and /users/{uid}, respectively. if i'm right, then that is a very legitimate point, but i'm not sure how we can fix this without overhauling the api.

@fisx
Copy link
Contributor

fisx commented Oct 22, 2024

sigh

$ vacuum version
v0.12.0
$ docker run --rm  dshanley/vacuum:v0.9.8 version
latest
$ docker run --rm  dshanley/vacuum:v0.12.0 version
latest

but i'm assuming that establishes that the newer version is more strict.

@pcapriotti
Copy link
Contributor Author

sigh

$ vacuum version
v0.12.0
$ docker run --rm  dshanley/vacuum:v0.9.8 version
latest
$ docker run --rm  dshanley/vacuum:v0.12.0 version
latest

but i'm assuming that establishes that the newer version is more strict.

Yes, we can switch to v0.12.0 and fix those errors. The overlap seems to be between /conversations/one-to-one and /conversation/:domain. We can probably rename the former to /one-to-one-conversations or similar in v7.

@pcapriotti
Copy link
Contributor Author

I fixed the overlapping paths reported by vacuum v0.12.0. These are breaking changes, so I versioned them under v7.

:> CanThrow 'InvalidHandle
:> CanThrow 'HandleNotFound
:> ZUser
:> "handles"
Copy link
Contributor

Choose a reason for hiding this comment

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

this will require nginz to be changed. what about changing /:uid/ into /uid/:uid/, so the old "check-user-handle" can stay?

@fisx
Copy link
Contributor

fisx commented Oct 23, 2024

/integration is failing with download asset returning 405. this endpoint is changed here, but i stared at the diff for a while and could not spot it.

@pcapriotti
Copy link
Contributor Author

/integration is failing with download asset returning 405. this endpoint is changed here, but i stared at the diff for a while and could not spot it.

It should be fixed in pcapriotti/validate-swagger. I can force-push it here if it's ok for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

echoes: unplanned Any work item that isn’t part of the product or technical roadmap. ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants