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 brig properties endpoints #2244

Merged
merged 11 commits into from
Apr 7, 2022

Conversation

pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Mar 30, 2022

This PR servantifies brig's properties endpoints.

A couple of things worth noting:

  • One of the servantified endpoints (namely GET /properties) was used to test /i/metrics. This role has now been taken by /onboarding/v3 (arbitrarily).
  • Since property values are validated in a way that requires knowledge of the settings, it cannot simply be done at the level of the Servant type classes. Therefore, I introduced a RawPropertyValue wrapper around a ByteString, which leaves it to the handler to do the actual parsing and validation.

As a side note, it is a bit strange that we are parsing property values at all, since we just convert them back to bytestrings immediately to save them in the database, and never look inside again. In principle, we could make them completely opaque bytestrings and avoid having to deal with all of these complications. However, it is hard to do without changing the API somehow, since these values are used inside other JSON objects.

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • changelog.d contains the following bits of information (details):
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

@pcapriotti pcapriotti force-pushed the pcapriotti/servantify-brig-properties branch from 5634f2f to 614adc0 Compare March 31, 2022 15:04
@pcapriotti pcapriotti marked this pull request as ready for review March 31, 2022 15:10
@pcapriotti pcapriotti force-pushed the pcapriotti/servantify-brig-properties branch from c0c5b61 to 094c40d Compare April 6, 2022 13:24
@pcapriotti pcapriotti temporarily deployed to cachix April 6, 2022 13:24 Inactive
@pcapriotti pcapriotti force-pushed the pcapriotti/servantify-brig-properties branch from 094c40d to fc7cdcc Compare April 6, 2022 13:26
@pcapriotti pcapriotti temporarily deployed to cachix April 6, 2022 13:26 Inactive
Copy link
Member

@akshaymankar akshaymankar left a comment

Choose a reason for hiding this comment

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

🚀

libs/wire-api/src/Wire/API/Properties.hs Show resolved Hide resolved
@pcapriotti pcapriotti merged commit e88f91f into develop Apr 7, 2022
@pcapriotti pcapriotti deleted the pcapriotti/servantify-brig-properties branch April 7, 2022 07:47
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.

None yet

3 participants