Servantify get users by unqualified ids or handles #1291

akshaymankar · 2020-12-16T16:07:51Z

No description provided.

jschaul

Looks reasonable, though I see 5-6 TODOs still in here, I assume you still want to work on them during this PR?

jschaul · 2020-12-29T22:31:58Z

The wire-api unittests fail with a mismatch between user profile JSON and swagger.

akshaymankar · 2021-01-04T13:43:16Z

The wire-api unittests fail with a mismatch between user profile JSON and swagger.

Yes, this should've been a draft.

* Create a newtype for CommaSeparatedList * Write FromHttpApiData instance for Range

We no longer use 'foo@bar.com' in the backend. One exception is while printing errors for federation not implemented. Maybe this should also not use this format.

mheinzel

It seems a bit weird to convert the unqualified IDs to qualified ones, just to then partition into local (unqualified) and remote ones again, but I think it makes sense in this situation.

There's one point about mutually exclusive parameters that would be good to address soon, but that could also be in a follow-up PR.

mheinzel · 2021-01-04T19:16:46Z

services/brig/src/Brig/API/Public.hs

+  case (mUids, mHandles) of
+    (Just uids, _) -> listUsersByIdsOrHandles self (Public.ListUsersByIds ((`Qualified` domain) <$> fromCommaSeparatedList uids))
+    (_, Just handles) ->
+      let normalRangedList = fromCommaSeparatedList $ fromRange handles
+          qualifiedList = (`Qualified` domain) <$> normalRangedList
+          -- Use of unsafeRange here is ok only because we know that 'handles'
+          -- is valid for 'Range 1 4'. However, we must not forget to keep this
+          -- annotation here otherwise a change in 'Public.ListUsersByHandles'
+          -- could cause this code to break.
+          qualifiedRangedList :: Range 1 4 [Qualified Handle] = unsafeRange qualifiedList
+       in listUsersByIdsOrHandles self (Public.ListUsersByHandles qualifiedRangedList)
+    (Nothing, Nothing) -> throwStd $ badRequest "at least one ids or handles must be provided"


So in case both are provided, we just silently ignore the provided handles? Would be nicer to fail, maybe reducing the boilderplate by using some kind of combinator Maybe a -> Maybe b -> Handler (Either a b) or even API combinator.

I decided to not do this for backwards compatibility. Currently nothing breaks if a client sends both of these and handles get ignored if uids are present. So, maybe it is ok to live with this in the old endpoint.
That being said I agree that it would be nicer to throw an error and since we're making a new endpoint, I will just make the parseJSON for the input fail if both are present.

Ah, you're right. Didn't know it behaved that way, but agree that it's best to keep it for the deprecated endpoint.

jschaul · 2021-01-04T23:21:18Z

Compile issue:

[4 of 8] Compiling Federator.Impl
                       
/home/user/Documents/git/wire-server/services/federator/src/Federator/Impl.hs:35:9: error:
    • No instance for (http-api-data-0.4.1.1:Web.Internal.HttpApiData.FromHttpApiData
                         (Data.Qualified.Qualified Data.Id.UserId))
        arising from a use of ‘serve’
    • In the expression: serve api (mock api (Proxy @'[]))
      In an equation for ‘app’:
          app _        
            = serve api (mock api (Proxy @'[]))
            where      
                api = Proxy @(ToServantApi API.Api)
   |                   
35 | app _ = serve api (mock api (Proxy @'[]))
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Also, perhaps it's worthwhile to rebase this PR on current develop.

Use separate domain and id so we don't have to parse the '@'

It was a little clunky to fit this in swagger, hopefully this will be better in swagger3.

akshaymankar · 2021-01-05T09:54:11Z

Alright, I think stuff should just work now 🤞
Could one of you please have a look at b06813b and let me know if the approval still stands?

mheinzel

New commit looks good. It still seems like some mutuallyExclusive combinator could make the instances clearer, especially if the pattern shows up in multiple places, but I'm fine with it for now.

libs/wire-api/src/Wire/API/User.hs

Co-authored-by: Matthias Heinzel <mheinzel@posteo.de>

jschaul

Minor typos for the next PR.

jschaul · 2021-01-05T09:57:12Z

libs/wire-api/src/Wire/API/User.hs

+      case (mUids, mHandles) of
+        (Just uids, Nothing) -> pure uids
+        (Nothing, Just handles) -> pure handles
+        (_, _) -> fail "exactly one of qualifie_ids or qualified_handles must be provided."


Suggested change

(_, _) -> fail "exactly one of qualifie_ids or qualified_handles must be provided."

(_, _) -> fail "exactly one of qualified_ids or qualified_handles must be provided."

jschaul · 2021-01-05T09:57:22Z

libs/wire-api/src/Wire/API/User.hs

+      NamedSchema (Just "ListUsersQuery") $
+        mempty
+          & type_ ?~ SwaggerObject
+          & description ?~ "exactly one of qualifie_ids or qualified_handles must be provided."


Suggested change

& description ?~ "exactly one of qualifie_ids or qualified_handles must be provided."

& description ?~ "exactly one of qualified_ids or qualified_handles must be provided."

jschaul · 2021-01-05T09:58:18Z

libs/wire-api/src/Wire/API/User.hs

+-- NB: It is not possible to specific mutually exclusive fields in swagger2, so
+-- here we write it in description and modify the example to have the correct


Suggested change

-- NB: It is not possible to specific mutually exclusive fields in swagger2, so

-- here we write it in description and modify the example to have the correct

-- NB: It is not possible to specify mutually exclusive fields in swagger2, so

-- here we write it in the description and modify the example to have the correct

jschaul · 2021-01-05T09:59:55Z

services/brig/src/Brig/API/Public.hs

+          -- could cause this code to break.
+          qualifiedRangedList :: Range 1 4 [Qualified Handle] = unsafeRange qualifiedList
+       in listUsersByIdsOrHandles self (Public.ListUsersByHandles qualifiedRangedList)
+    (Nothing, Nothing) -> throwStd $ badRequest "at least one ids or handles must be provided"


Suggested change

(Nothing, Nothing) -> throwStd $ badRequest "at least one ids or handles must be provided"

(Nothing, Nothing) -> throwStd $ badRequest "One of 'ids' or 'handles' as query parameter must be provided"

jschaul · 2021-01-05T10:00:35Z

services/brig/src/Brig/API/Public.hs

+      us <- getIds localHandles
+      filterHandleResults self =<< byIds us
+  case foundUsers of
+    [] -> throwStd $ notFound "None of the specified ids or handles match any users"


Suggested change

[] -> throwStd $ notFound "None of the specified ids or handles match any users"

[] -> throwStd $ notFound "None of the specified ids or handles matches any users"

jschaul force-pushed the akshaymankar/get-users branch from e9530df to def05b4 Compare December 29, 2020 20:23

jschaul reviewed Dec 29, 2020

View reviewed changes

akshaymankar and others added 4 commits December 29, 2020 23:05

WIP: Servantify get users by unqualified ids or handles

30e2964

Finish servantifying get users by unqualified ids or handles

2c86b20

Hastily implement GetUsersByQualifiedIdsOrHandles 🎄

3c38888

Hi CI

1982e3d

jschaul force-pushed the akshaymankar/get-users branch from 9e51019 to 1982e3d Compare December 29, 2020 22:05

Hi CI

cf2d37d

akshaymankar added 6 commits January 4, 2021 14:54

Fix ToSchema instance for Qualified ID

23260cd

Move orphans out of Brig.API.Public

13236a8

* Create a newtype for CommaSeparatedList * Write FromHttpApiData instance for Range

Brig.API.Public: Add comment about using 'unsafeRange'

c22664a

Brig/listUsers: Return 404 if no users found

bfd603f

Data.Qualified: Delete parser/reder code for string serialization

b9eb838

We no longer use 'foo@bar.com' in the backend. One exception is while printing errors for federation not implemented. Maybe this should also not use this format.

Test.Qualified: Add missing imports

b6eaf11

mheinzel approved these changes Jan 4, 2021

View reviewed changes

akshaymankar added 3 commits January 5, 2021 09:34

Merge branch 'develop' into akshaymankar/get-users

714ccf3

Remove qualified Ids from federator API

26ee12d

Use separate domain and id so we don't have to parse the '@'

Expect exactly one of ids or handles while listing users

b06813b

It was a little clunky to fit this in swagger, hopefully this will be better in swagger3.

mheinzel approved these changes Jan 5, 2021

View reviewed changes

mheinzel reviewed Jan 5, 2021

View reviewed changes

libs/wire-api/src/Wire/API/User.hs Outdated Show resolved Hide resolved

Wire.API.User: Fix typo

811b39b

Co-authored-by: Matthias Heinzel <mheinzel@posteo.de>

akshaymankar merged commit 9d89976 into develop Jan 5, 2021

akshaymankar deleted the akshaymankar/get-users branch January 5, 2021 11:11

jschaul reviewed Jan 5, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Servantify get users by unqualified ids or handles #1291

Servantify get users by unqualified ids or handles #1291

akshaymankar commented Dec 16, 2020

jschaul left a comment

jschaul commented Dec 29, 2020

akshaymankar commented Jan 4, 2021

mheinzel left a comment

mheinzel Jan 4, 2021

akshaymankar Jan 5, 2021

mheinzel Jan 5, 2021

jschaul commented Jan 4, 2021

akshaymankar commented Jan 5, 2021

mheinzel left a comment

jschaul left a comment

jschaul Jan 5, 2021

jschaul Jan 5, 2021

jschaul Jan 5, 2021

jschaul Jan 5, 2021

jschaul Jan 5, 2021

	(_, _) -> fail "exactly one of qualifie_ids or qualified_handles must be provided."
	(_, _) -> fail "exactly one of qualified_ids or qualified_handles must be provided."

	& description ?~ "exactly one of qualifie_ids or qualified_handles must be provided."
	& description ?~ "exactly one of qualified_ids or qualified_handles must be provided."

		-- NB: It is not possible to specific mutually exclusive fields in swagger2, so
		-- here we write it in description and modify the example to have the correct

	(Nothing, Nothing) -> throwStd $ badRequest "at least one ids or handles must be provided"
	(Nothing, Nothing) -> throwStd $ badRequest "One of 'ids' or 'handles' as query parameter must be provided"

	[] -> throwStd $ notFound "None of the specified ids or handles match any users"
	[] -> throwStd $ notFound "None of the specified ids or handles matches any users"

Servantify get users by unqualified ids or handles #1291

Servantify get users by unqualified ids or handles #1291

Conversation

akshaymankar commented Dec 16, 2020

jschaul left a comment

Choose a reason for hiding this comment

jschaul commented Dec 29, 2020

akshaymankar commented Jan 4, 2021

mheinzel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jschaul commented Jan 4, 2021

akshaymankar commented Jan 5, 2021

mheinzel left a comment

Choose a reason for hiding this comment

jschaul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment