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

Servantify more of brig internal api #3346

Merged
merged 46 commits into from
Jul 17, 2023
Merged

Servantify more of brig internal api #3346

merged 46 commits into from
Jul 17, 2023

Conversation

fisx
Copy link
Contributor

@fisx fisx commented Jun 11, 2023

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

Checklist

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

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Jun 11, 2023
- `get "/i/users/:uid/contacts" (continue getContactListH)`
- `get "/i/users/activation-code" (continue getActivationCodeH)`
- `get "/i/users/password-reset-code" (continue getPasswordResetCodeH)`
fisx added 6 commits June 12, 2023 16:51
- `post "/i/users/revoke-identity" (continue revokeIdentityH)`
- `head "/i/users/blacklist" (continue checkBlacklistH)`
- `delete "/i/users/blacklist" (continue deleteFromBlacklistH)`
- `post "/i/users/blacklist" (continue addBlacklistH)`
- `get "/i/users/phone-prefixes/:prefix" (continue getPhonePrefixesH)`
- `delete "/i/users/phone-prefixes/:prefix" (continue deleteFromPhonePrefixH)`
- `post "/i/users/phone-prefixes" (continue addPhonePrefixH)`
- `put "/i/users/:uid/sso-id" (continue updateSSOIdH)`
- `delete "/i/users/:uid/sso-id" (continue deleteSSOIdH)`
- `put "/i/users/:uid/managed-by" (continue updateManagedByH)`
- `put "/i/users/:uid/rich-info" (continue updateRichInfoH)`
- `put "/i/users/:uid/handle" (continue updateHandleH)`
- `put "/i/users/:uid/name" (continue updateUserNameH)`
- `get "/i/users/:uid/rich-info" (continue getRichInfoH)`
- `get "/i/users/rich-info" (continue getRichInfoMultiH)`
- `head "/i/users/handles/:handle" (continue checkHandleInternalH)`
- `put "/i/connections/connection-update" (continue updateConnectionInternalH)`
- `post "/i/clients" (continue internalListClientsH)`
- `post "/i/clients/full" (continue internalListFullClientsH)`
- `post "/i/clients/:uid" (continue addClientInternalH)`
- `post "/i/clients/legalhold/:uid/request" (continue legalHoldClientRequestedH)`
- `delete "/i/clients/legalhold/:uid" (continue removeLegalHoldClientH)`
@fisx fisx marked this pull request as ready for review June 15, 2023 12:24
@fisx
Copy link
Contributor Author

fisx commented Jun 21, 2023

this is getting a little out of hand...

Tests
  User (types vs. aeson)
    [...]
    RichInfo Examples
      Empty rich info:                                                                              FAIL
        Error message: RichInfoMapAndList
        expected: Success (RichInfoMapAndList {richInfoMap = fromList [], richInfoAssocList = []})
         but got: Error "key \"richInfo\" not found"

        CallStack (from HasCallStack):
          assertFailure, called at ./Test/Tasty/HUnit/Orig.hs:86:32 in tasty-hunit-0.10.0.3-5gW5br1avVmDRoG1UA3lIa:Test.Tasty.HUnit.Orig
          assertEqual, called at test/unit/Test/Wire/API/User/RichInfo.hs:74:11 in main:Test.Wire.API.User.RichInfo
        Use -p '/RichInfo/&&/Empty rich info/' to rerun this test only.

i think the problem is that we used to treat object field names as case insensitive in the FromJSON instances, but it's unclear how to do that here. field and friends from schema-profunctor have Text baked in.

also failing:

make ci package=brig TASTY_PATTERN=/de-whitelisting/
make ci package=galley TASTY_PATTERN='/update conversation as member/'
make ci package=galley TASTY_PATTERN='/Teams LegalHold API (with flag whitelist-teams-and-implicit-consent).PUT \/teams\/{tid}\/legalhold\/approve/'
make ci package=galley TASTY_PATTERN='/Teams LegalHold API (with flag whitelist-teams-and-implicit-consent).DELETE \/teams\/{tid}\/legalhold\/{uid}/'

@fisx
Copy link
Contributor Author

fisx commented Jul 6, 2023

Another puzzle: is there a way to allow for the value of a json field to be either an object or an array, so two completely different parsers? I think that's what <|> is for, but I can't get it to work.

To be more specific, I have this test (reproduce with make c package=wire-api test=1 TASTY_PATTERN='/RichInfoMapAndList as only assoc list/'):

        testCase "RichInfoMapAndList as only assoc list" $ do
          let inputJSON =
                [aesonQQ|{
                                     "urn:wire:scim:schemas:profile:1.0" : {
                                        "richinfo": [{"type": "foo", "value": "bar"}]
                                     }
                                   }|]
          assertEqual "RichInfoMapAndList" (Aeson.Success $ mkRichInfo [RichField "foo" "bar"]) $ fromJSON inputJSON,

... which requires that richinfo can not only contain an object with version and fields, but also the fields without a surrounding object. both are legal and should be parsed successfully, but we don't handle this corner case with the new schema-profunctor instances.

So I tried this:

instance ToSchema RichInfoAssocList where
  schema =
    ciObject "RichInfoAssocList" richInfoAssocListSchema
      <|> richInfoAssocListSchemaLegacy
    where
      richInfoAssocListSchemaLegacy :: ValueSchema NamedSwaggerDoc RichInfoAssocList
      richInfoAssocListSchemaLegacy = array (schema @RichField)

      richInfoAssocListSchema :: CIObjectSchema SwaggerDoc RichInfoAssocList
      [...]

But ghc wasn't convinced:

$ make c package=wire-api test=1 TASTY_PATTERN='/RichInfoMapAndList as only assoc list/'
[...]
src/Wire/API/User/RichInfo.hs:280:39: error:
    • Couldn't match type ‘[RichField]’ with ‘RichInfoAssocList’
      Expected: ValueSchema NamedSwaggerDoc RichInfoAssocList
        Actual: ValueSchema NamedSwaggerDoc [RichField]
    • In the expression: array (schema @RichField)
      In an equation for ‘richInfoAssocListSchemaLegacy’:
          richInfoAssocListSchemaLegacy = array (schema @RichField)
      In an equation for ‘schema’:
          schema
            = ciObject "RichInfoAssocList" richInfoAssocListSchema
                <|> richInfoAssocListSchemaLegacy
            where
                richInfoAssocListSchemaLegacy ::
                  ValueSchema NamedSwaggerDoc RichInfoAssocList
                richInfoAssocListSchemaLegacy = array (schema @RichField)
                richInfoAssocListSchema ::
                  CIObjectSchema SwaggerDoc RichInfoAssocList
                richInfoAssocListSchema
                  = withParser
                      ((,) <$> const (0 :: Int) .= ciField "version" schema
                         <*> unRichInfoAssocList .= ciField "fields" (array schema))
                      $ \ (version, fields)
                          -> mkRichInfoAssocList <$> validateRichInfoAssocList version fields
    |
280 |       richInfoAssocListSchemaLegacy = array (schema @RichField)

When I implemented richInfoAssocListSchemaLegacy as undefined, another error surfaced on the calling side:

src/Wire/API/User/RichInfo.hs:277:7: error:
    • No instance for (Data.Schema.NearSemiRing S.NamedSchema)
        arising from a use of ‘<|>’
    • In the expression:
        ciObject "RichInfoAssocList" richInfoAssocListSchema
          <|> richInfoAssocListSchemaLegacy
      In an equation for ‘schema’:
          schema
            = ciObject "RichInfoAssocList" richInfoAssocListSchema
                <|> richInfoAssocListSchemaLegacy
            where
                richInfoAssocListSchemaLegacy ::
                  ValueSchema NamedSwaggerDoc RichInfoAssocList
                richInfoAssocListSchemaLegacy = undefined
                richInfoAssocListSchema ::
                  CIObjectSchema SwaggerDoc RichInfoAssocList
                richInfoAssocListSchema
                  = withParser
                      ((,) <$> const (0 :: Int) .= ciField "version" schema
                         <*> unRichInfoAssocList .= ciField "fields" (array schema))
                      $ \ (version, fields)
                          -> mkRichInfoAssocList <$> validateRichInfoAssocList version fields
      In the instance declaration for ‘ToSchema RichInfoAssocList’
    |
277 |       <|> richInfoAssocListSchemaLegacy

@@ -1065,8 +1064,6 @@ tests =
testObjects [(Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_1, "testObject_RichField_user_1.json"), (Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_2, "testObject_RichField_user_2.json"), (Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_3, "testObject_RichField_user_3.json"), (Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_4, "testObject_RichField_user_4.json"), (Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_5, "testObject_RichField_user_5.json"), (Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_6, "testObject_RichField_user_6.json"), (Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_7, "testObject_RichField_user_7.json"), (Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_8, "testObject_RichField_user_8.json"), (Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_9, "testObject_RichField_user_9.json"), (Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_10, "testObject_RichField_user_10.json"), (Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_11, "testObject_RichField_user_11.json"), (Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_12, "testObject_RichField_user_12.json"), (Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_13, "testObject_RichField_user_13.json"), (Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_14, "testObject_RichField_user_14.json"), (Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_15, "testObject_RichField_user_15.json"), (Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_16, "testObject_RichField_user_16.json"), (Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_17, "testObject_RichField_user_17.json"), (Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_18, "testObject_RichField_user_18.json"), (Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_19, "testObject_RichField_user_19.json"), (Test.Wire.API.Golden.Generated.RichField_user.testObject_RichField_user_20, "testObject_RichField_user_20.json")],
testGroup "Golden: RichInfoAssocList_user" $
testObjects [(Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_1, "testObject_RichInfoAssocList_user_1.json"), (Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_2, "testObject_RichInfoAssocList_user_2.json"), (Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_3, "testObject_RichInfoAssocList_user_3.json"), (Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_4, "testObject_RichInfoAssocList_user_4.json"), (Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_5, "testObject_RichInfoAssocList_user_5.json"), (Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_6, "testObject_RichInfoAssocList_user_6.json"), (Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_7, "testObject_RichInfoAssocList_user_7.json"), (Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_8, "testObject_RichInfoAssocList_user_8.json"), (Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_9, "testObject_RichInfoAssocList_user_9.json"), (Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_10, "testObject_RichInfoAssocList_user_10.json"), (Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_11, "testObject_RichInfoAssocList_user_11.json"), (Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_12, "testObject_RichInfoAssocList_user_12.json"), (Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_13, "testObject_RichInfoAssocList_user_13.json"), (Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_14, "testObject_RichInfoAssocList_user_14.json"), (Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_15, "testObject_RichInfoAssocList_user_15.json"), (Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_16, "testObject_RichInfoAssocList_user_16.json"), (Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_17, "testObject_RichInfoAssocList_user_17.json"), (Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_18, "testObject_RichInfoAssocList_user_18.json"), (Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_19, "testObject_RichInfoAssocList_user_19.json"), (Test.Wire.API.Golden.Generated.RichInfoAssocList_user.testObject_RichInfoAssocList_user_20, "testObject_RichInfoAssocList_user_20.json")],
testGroup "Golden: RichInfoMapAndList_user" $
Copy link
Contributor Author

Choose a reason for hiding this comment

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

RichInfoMapAndList doesn't have a Schema instance any more; it's only needed for implementing (de-)serializing RichInfo.

@@ -0,0 +1,94 @@
{-# LANGUAGE OverloadedStrings #-}
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 think this entire module just moved here.

@fisx fisx requested a review from pcapriotti July 7, 2023 13:44
Copy link
Contributor

@pcapriotti pcapriotti left a comment

Choose a reason for hiding this comment

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

LGTM

libs/wire-api/src/Wire/API/User/RichInfo.hs Outdated Show resolved Hide resolved
libs/wire-api/src/Wire/API/User/RichInfo.hs Outdated Show resolved Hide resolved
fisx and others added 4 commits July 10, 2023 11:17
Co-authored-by: Paolo Capriotti <paolo@capriotti.io>
Co-authored-by: Paolo Capriotti <paolo@capriotti.io>
(by running the same, more general handler over the union of the two
routes.)
@fisx fisx merged commit f33f1e0 into develop Jul 17, 2023
2 checks passed
@fisx fisx deleted the servantify-1 branch July 17, 2023 14:08
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.

3 participants