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

Add schema migration for new tables #1485

Merged
merged 42 commits into from
May 17, 2021

Conversation

jschaul
Copy link
Member

@jschaul jschaul commented May 4, 2021

  • schema migration for remote users in local conversations; and users -> remote conversations
  • partially implements adding a remote user to a local conversation: cassandra queries are made use of (data is actually inserted), however checks are missing and that endpoint is still hidden from actual usage by using a /i/ prefix.
  • augments OtherMember to also give back a qualified user Id; this means that querying for a conversation (get /conversations/:cnv) supports showing remote users. For this, golden tests had to be adjusted, which accounts for some of the noise of this PR.
  • removing a remote member is implemented at the cql query level but never actually used nor tested. That is a futurework.
  • There may be some minor conflicts with the work in Port instances to schemas library #1482 (we needed some instances to make servant work); use the instances of Port instances to schemas library #1482 when resolving conflicts.
  • Adds scaffolding for internal endpoints in galley with servant.
  • Adds lots of FUTUREWORKs.

See the comment in https://github.com/wireapp/wire-server/pull/1485/files#diff-259be1b8909f30421ab02e60765d6e90ae68426b95c3aa8303c57945dc84dd37R467-R481 for some next steps that can be implemented once this PR is merged.

@jschaul jschaul force-pushed the add-remote-users-to-conversation-schema branch from ab29660 to f44cee9 Compare May 4, 2021 21:11
@jschaul jschaul force-pushed the add-remote-users-to-conversation-schema branch from 7a24bb9 to c9ffc15 Compare May 11, 2021 23:06
@akshaymankar akshaymankar changed the title [WIP] [skip ci] Add schema migration for new tables [WIP] Add schema migration for new tables May 12, 2021
@jschaul jschaul changed the title [WIP] Add schema migration for new tables Add schema migration for new tables May 12, 2021
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.

I have taken the liberty to merge develop, and then remove JSON instances that were covered by schemas.

Looks good to me. I've added a few minor inline comments.

libs/types-common/src/Data/Qualified.hs Show resolved Hide resolved
libs/types-common/src/Data/Qualified.hs Show resolved Hide resolved
libs/api-bot/src/Network/Wire/Bot/Assert.hs Show resolved Hide resolved
services/galley/schema/src/V49_ReAddRemoteIdentifiers.hs Outdated Show resolved Hide resolved
services/galley/src/Galley/Data.hs Outdated Show resolved Hide resolved
services/galley/src/Galley/Data/Queries.hs Outdated Show resolved Hide resolved
services/galley/test/integration/API.hs Outdated Show resolved Hide resolved
Comment on lines +155 to +158
-- FUTUREWORK: Make a PR to the servant-swagger package with this instance
instance ToSchema Servant.NoContent where
declareNamedSchema _ = declareNamedSchema (Proxy @())

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not normally necessary, because servant-swagger has a more specific instance for Verb ... NoContent. Here we need it because NoContent is inside UVerb, but I'm not sure this is the best way.

I think this is fine for now, but for the purposes of submitting a PR, I think the proper way to support NoContent in UVerb is to have some more specific instances for ToSwagger that do not necessarily require ToSchema. I have done something similar for HasServer and HasClient in my servant PR.

libs/wire-api/src/Wire/API/Event/Conversation.hs Outdated Show resolved Hide resolved
@jschaul jschaul merged commit 7682515 into develop May 17, 2021
@jschaul jschaul deleted the add-remote-users-to-conversation-schema branch May 17, 2021 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants