Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WPB-1103: Adding relative URLs to swagger docs. #3619

Merged
merged 3 commits into from
Sep 29, 2023
Merged

Conversation

lepsa
Copy link
Contributor

@lepsa lepsa commented Sep 28, 2023

Adding relative paths to the OpenApi generation code so that it matches with any URL that the server is using.
Adding basePath values to the older swagger files reflecting their versions. This is essentially the same as the OpenApi relative server URLs.

Checklist

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

@lepsa
Copy link
Contributor Author

lepsa commented Sep 28, 2023

This does add the version prefix onto every route, and as per the API versioning docs this is OK.
https://docs.wire.com/developer/developer/api-versioning.html

Some endpoints are unversioned. The backend will also accept versioned requests to them, but they all behave identically regardless of the version prefix. The unversioned endpoints are:
the /api-version endpoint itself; this is so that a client can dynamically determine which version to use based on the information returned by the backend;
the /access endpoint; this is so that access cookie paths can be set to the same value regardless of the version, which avoids invalidating logins across version upgrades.

The swagger files are compiled into the binary, and make can be fiddly about rebuilds. Local testing is best done by a make clean; make cr run, and then loading up http://localhost:9082/api/swagger-ui.

Each of the swagger routes for V0 through V4 should list a [ Base URL: /v$VERSION ] at the top of the file, and when "trying out" routes in swagger-ui, the curl example should also correctly list the version.
image

The V5 api docs use OpenApi 3, and are slightly different. The base path is shown in the server selection drop down.
image
The curl and request URLs are the same as the Swagger 2 versions.
image

@battermann battermann added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Sep 28, 2023
changelog.d/4-docs/WPB-1103 Outdated Show resolved Hide resolved
@fisx
Copy link
Contributor

fisx commented Sep 28, 2023

in brig:

      get /users/:domain/:uid - 422:                                                         FAIL
        Exception: Assertions failed:
         1: 422 =/= 500
         2: Just (String "/federation/api-version") =/= Nothing
         3: Just (String "invalid.example.com") =/= Nothing
        
        Response was:
        
        Response {responseStatus = Status {statusCode = 500, statusMessage = "Internal Server Error"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Thu, 28 Sep 2023 07:47:32 GMT"),("Server","Warp/3.3.23"),("Content-Encoding","gzip"),("Content-Type","application/json"),("Vary","Accept-Encoding")], responseBody = Just "{\"code\":500,\"label\":\"federation-not-available\",\"message\":\"Network.Socket.connect: <socket: 17>: does not exist (Connection refused)\"}", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}
        CallStack (from HasCallStack):
          error, called at src/Bilge/Assert.hs:91:5 in bilge-0.22.0-6thsNorikcyKuCfrAWNit5:Bilge.Assert
          <!!, called at src/Bilge/Assert.hs:109:19 in bilge-0.22.0-6thsNorikcyKuCfrAWNit5:Bilge.Assert
          !!!, called at test/integration/API/User/Account.hs:632:5 in main:API.User.Account
        Use -p '(!/turn/&&!/user.auth.cookies.limit/)&&/get \/users\/:domain\/:uid - 422/' to rerun this test only.

in integration:

----- AccessUpdate.testAccessUpdateGuestRemovedUnreachableRemotes FAIL (3.08 s) -----
assertion failure:
Actual:
533
Expected:
201

call stack: 
1. assertFailure at test/Testlib/Assertions.hs:60

2. shouldMatch at test/Testlib/Assertions.hs:106

=== (relevant) logs for failed brig-integration ===
      post /register - 201 existing activation:                                              OK (1.13s)
      post /register - 409 conflict:                                                         OK (0.95s)
      post /register - 400 invalid input:                                                    OK (0.18s)
      post /register - 403 blacklist:                                                        OK (3.06s)
      post /register - 400 external-SSO:                                                     OK (0.01s)
      post /register - 403 restricted user creation:                                         OK (1.69s)
      post /register - 403 too many members for legalhold:                                   OK (1.50s)
      post /activate - 200/204 + expiry:                                                     OK (10.39s)
      get /users/:uid - 404:                                                                 OK (0.20s)
      get /users/<localdomain>/:uid - 404:                                                   OK (0.53s)
      get /users/:domain/:uid - 422:                                                         FAIL
        Exception: Assertions failed:
         1: 422 =/= 500
         2: Just (String "/federation/api-version") =/= Nothing
         3: Just (String "invalid.example.com") =/= Nothing
        Response was:
        Response {responseStatus = Status {statusCode = 500, statusMessage = "Internal Server Error"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Thu, 28 Sep 2023 07:47:32 GMT"),("Server","Warp/3.3.23"),("Content-Encoding","gzip"),("Content-Type","application/json"),("Vary","Accept-Encoding")], responseBody = Just "{\"code\":500,\"label\":\"federation-not-available\",\"message\":\"Network.Socket.connect: <socket: 17>: does not exist (Connection refused)\"}", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}
        CallStack (from HasCallStack):
          error, called at src/Bilge/Assert.hs:91:5 in bilge-0.22.0-6thsNorikcyKuCfrAWNit5:Bilge.Assert
          <!!, called at src/Bilge/Assert.hs:109:19 in bilge-0.22.0-6thsNorikcyKuCfrAWNit5:Bilge.Assert
          !!!, called at test/integration/API/User/Account.hs:632:5 in main:API.User.Account

they both touch /federation/api-version, that smells like a clue?

@fisx
Copy link
Contributor

fisx commented Sep 28, 2023

did the tests pass locally @lepsa?

Co-authored-by: fisx <mf@zerobuzz.net>
@lepsa
Copy link
Contributor Author

lepsa commented Sep 28, 2023

They did. I

did the tests pass locally @lepsa?

I did, and all three suites passed with flying colours.

Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

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

Is changing Swagger files of finalised API versions necessary? It makes me wary. It's not something we've done so far.

@fisx
Copy link
Contributor

fisx commented Sep 29, 2023

Is changing Swagger files of finalised API versions necessary? It makes me wary. It's not something we've done so far.

if we don't, they are wrong, and the constructed curl commands won't work as expected (they will all call v0 no matter which version is intended).

so i'd say yes, we should fix them.

@fisx fisx merged commit fac705c into wireapp:develop Sep 29, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants