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.

Sign up for GitHub

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-10324] Drop legacy notification endpoints #4363

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

[WPB-10324] Drop legacy notification endpoints #4363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f428562
Introduce client API versions V8 and V9
mdimjasevic Dec 5, 2024
eeaa83d
Remove the target endpoints from V9
mdimjasevic Dec 5, 2024
1158d2e
Add a changelog
mdimjasevic Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/1-api-changes/WPB-10324
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Drop legacy notification endpoints from client API at version 9
Copy link
Contributor

Choose a reason for hiding this comment

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

did you also create the confluence page and add it there? seems like a good idea to me, especially since we might all completely forget by the time this becomes relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't yet, as I am still not 100% sure what version this should be reflected in. The latest discussion in the ticket suggests this won't be done in version 9, but in a much later version and perhaps this PR can be closed, but let's wait and see how the discussion evolves.

2 changes: 1 addition & 1 deletion integration/test/Test/Swagger.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import Testlib.Prelude
import UnliftIO.Temporary

existingVersions :: Set Int
existingVersions = Set.fromList [0, 1, 2, 3, 4, 5, 6, 7, 8]
existingVersions = Set.fromList [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]

internalApis :: Set String
internalApis = Set.fromList ["brig", "cannon", "cargohold", "cannon", "spar"]
Expand Down
14 changes: 10 additions & 4 deletions integration/test/Test/Version.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ instance TestCases Versioned' where
MkTestCase "[version=versioned]" (Versioned' Versioned),
MkTestCase "[version=v1]" (Versioned' (ExplicitVersion 1)),
MkTestCase "[version=v3]" (Versioned' (ExplicitVersion 3)),
MkTestCase "[version=v6]" (Versioned' (ExplicitVersion 6))
MkTestCase "[version=v6]" (Versioned' (ExplicitVersion 6)),
MkTestCase "[version=v7]" (Versioned' (ExplicitVersion 7)),
MkTestCase "[version=v8]" (Versioned' (ExplicitVersion 8))
]

-- | Used to test endpoints that have changed after version 5
Expand Down Expand Up @@ -43,9 +45,11 @@ testVersion (Versioned' v) = withModifiedBackend
domain <- resp.json %. "domain" & asString
federation <- resp.json %. "federation" & asBool

-- currently there is only one development version
-- it is however theoretically possible to have multiple development versions
length dev `shouldMatchInt` 1
-- currently there are two development versions
--
-- it is however possible to have a different number of development
-- versions
length dev `shouldMatchInt` 2
domain `shouldMatch` dom
federation `shouldMatch` True

Expand Down Expand Up @@ -77,6 +81,8 @@ testVersionDisabled = withModifiedBackend
void $ getSelfWithVersion (ExplicitVersion 4) user >>= getJSON 200
void $ getSelfWithVersion (ExplicitVersion 5) user >>= getJSON 200
void $ getSelfWithVersion (ExplicitVersion 6) user >>= getJSON 200
void $ getSelfWithVersion (ExplicitVersion 7) user >>= getJSON 200
void $ getSelfWithVersion (ExplicitVersion 8) user >>= getJSON 200
void $ getSelfWithVersion Unversioned user >>= getJSON 200

testVersionDisabledNotAdvertised :: App ()
Expand Down
2 changes: 2 additions & 0 deletions libs/wire-api/src/Wire/API/Routes/Public/Cannon.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,15 @@ import Servant
import Wire.API.Routes.API
import Wire.API.Routes.Named
import Wire.API.Routes.Public (ZConn, ZUser)
import Wire.API.Routes.Version
import Wire.API.Routes.WebSocket

type CannonAPI =
Named
"await-notifications"
( Summary "Establish websocket connection"
-- Description "This is the legacy variant of \"consume-events\""
:> Until 'V9
:> "await"
:> ZUser
:> ZConn
Expand Down
3 changes: 3 additions & 0 deletions libs/wire-api/src/Wire/API/Routes/Public/Gundeck.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ type NotificationAPI =
Named
"get-notification-by-id"
( Summary "Fetch a notification by ID"
:> Until 'V9
:> ZUser
:> "notifications"
:> Capture' '[Description "Notification ID"] "id" NotificationId
Expand All @@ -83,6 +84,7 @@ type NotificationAPI =
:<|> Named
"get-last-notification"
( Summary "Fetch the last notification"
:> Until 'V9
:> ZUser
:> "notifications"
:> "last"
Expand Down Expand Up @@ -116,6 +118,7 @@ type NotificationAPI =
"get-notifications"
( Summary "Fetch notifications"
:> From 'V3
:> Until 'V9
:> ZUser
:> "notifications"
:> QueryParam' [Optional, Strict, Description "Only return notifications more recent than this"] "since" NotificationId
Expand Down
4 changes: 3 additions & 1 deletion libs/wire-api/src/Wire/API/Routes/Version.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ import Wire.Arbitrary (Arbitrary, GenericUniform (GenericUniform))
-- and 'developmentVersions' stay in sync; everything else here should keep working without
-- change. See also documentation in the *docs* directory.
-- https://docs.wire.com/developer/developer/api-versioning.html#version-bump-checklist
data Version = V0 | V1 | V2 | V3 | V4 | V5 | V6 | V7 | V8
data Version = V0 | V1 | V2 | V3 | V4 | V5 | V6 | V7 | V8 | V9
deriving stock (Eq, Ord, Bounded, Enum, Show, Generic)
deriving (FromJSON, ToJSON) via (Schema Version)
deriving (Arbitrary) via (GenericUniform Version)
Expand All @@ -101,6 +101,7 @@ versionInt V5 = 5
versionInt V6 = 6
versionInt V7 = 7
versionInt V8 = 8
versionInt V9 = 9
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 conflict with #4356

but maybe that's fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's totally fine. I am aware of this. There's no way to avoid conflicts if we work in parallel on related stuff. I see the PR has been merged in the PR so I'll rebase this PR to resolve the conflicts.


supportedVersions :: [Version]
supportedVersions = [minBound .. maxBound]
Expand Down Expand Up @@ -213,6 +214,7 @@ isDevelopmentVersion V5 = False
isDevelopmentVersion V6 = False
isDevelopmentVersion V7 = False
isDevelopmentVersion V8 = True
isDevelopmentVersion V9 = True

developmentVersions :: [Version]
developmentVersions = filter isDevelopmentVersion supportedVersions
Expand Down
16 changes: 16 additions & 0 deletions services/brig/src/Brig/API/Public.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,22 @@ internalEndpointsSwaggerDocsAPIs =
--
-- Dual to `internalEndpointsSwaggerDocsAPI`.
versionedSwaggerDocsAPI :: Servant.Server VersionedSwaggerDocsAPI
versionedSwaggerDocsAPI (Just (VersionNumber V9)) =
swaggerSchemaUIServer $
( serviceSwagger @VersionAPITag @'V9
<> serviceSwagger @BrigAPITag @'V9
<> serviceSwagger @GalleyAPITag @'V9
<> serviceSwagger @SparAPITag @'V9
<> serviceSwagger @CargoholdAPITag @'V9
<> serviceSwagger @CannonAPITag @'V9
<> serviceSwagger @GundeckAPITag @'V9
<> serviceSwagger @ProxyAPITag @'V9
<> serviceSwagger @OAuthAPITag @'V9
)
& S.info . S.title .~ "Wire-Server API"
& S.info . S.description ?~ $(embedText =<< makeRelativeToProject "docs/swagger.md")
& S.servers .~ [S.Server ("/" <> toUrlPiece V9) Nothing mempty]
& cleanupSwagger
versionedSwaggerDocsAPI (Just (VersionNumber V8)) =
swaggerSchemaUIServer $
( serviceSwagger @VersionAPITag @'V8
Expand Down
4 changes: 2 additions & 2 deletions services/brig/test/integration/API/User/Auth.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ testNginz b n = do
post (unversioned . n . path "/access" . cookie c . header "Authorization" ("Bearer " <> toByteString' t)) <!! do
const 200 === statusCode
-- ensure regular user tokens can fetch notifications
get (n . path "/notifications" . header "Authorization" ("Bearer " <> toByteString' t)) !!! const 200 === statusCode
get (apiVersion "v8" . n . path "/notifications" . header "Authorization" ("Bearer " <> toByteString' t)) !!! const 200 === statusCode

testNginzLegalHold :: Brig -> Galley -> Nginz -> Http ()
testNginzLegalHold b g n = do
Expand Down Expand Up @@ -282,7 +282,7 @@ testNginzLegalHold b g n = do
get (n . path "/clients" . header "Authorization" ("Bearer " <> toByteString' t)) !!! const 403 === statusCode
get (n . path "/self" . header "Authorization" ("Bearer " <> toByteString' t)) !!! const 403 === statusCode
-- ensure legal hold tokens can fetch notifications
get (n . path "/notifications" . header "Authorization" ("Bearer " <> toByteString' t)) !!! const 200 === statusCode
get (apiVersion "v8" . n . path "/notifications" . header "Authorization" ("Bearer " <> toByteString' t)) !!! const 200 === statusCode

get (apiVersion "v1" . n . paths ["legalhold", "conversations", toByteString' (qUnqualified qconv)] . header "Authorization" ("Bearer " <> toByteString' t)) !!! const 200 === statusCode

Expand Down
Loading