Skip to content

Comments

Federation API versioning#2040

Closed
pcapriotti wants to merge 11 commits intodevelopfrom
pcapriotti/fed-api-versioning
Closed

Federation API versioning#2040
pcapriotti wants to merge 11 commits intodevelopfrom
pcapriotti/fed-api-versioning

Conversation

@pcapriotti
Copy link
Contributor

This is the first step towards implementing the API versioning scheme outlined in https://github.com/wireapp/wire-server/blob/develop/docs/developer/api-versioning.md for the federation API.

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.
  • If HTTP endpoint paths have been added or renamed, the endpoint / config-flag checklist (see Wire-employee only backend wiki page) has been followed.
  • 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/fed-api-versioning branch from 1bae681 to 3e8b983 Compare January 17, 2022 14:30
@pcapriotti pcapriotti force-pushed the pcapriotti/fed-api-versioning branch from 3e8b983 to e7611af Compare January 17, 2022 14:49
@pcapriotti pcapriotti force-pushed the pcapriotti/fed-api-versioning branch from 37a002e to 5b6c0fe Compare February 2, 2022 07:50
@pcapriotti pcapriotti added the bit-rot PRs that have bit-rotted label Mar 2, 2022
@fisx
Copy link
Contributor

fisx commented Mar 18, 2022

this has been stuck for a while, but i have another weird idea. i've written this down in anger, so please ask me to elaborate if you're getting to this.

the simple case

we provide a new servant client function generator vclient that
takes a dynamic version V16 and a static version V19. V19 is used to
call client and shape the request; V16 is put into the path of the
result of calling client, overwriting V19.

V16 is any supported version, V19 is the most recent supported
version, or one that is identical to the most recent supported one.

for all supported versions Vx, we test that Vx of the route has the
same type as V19. (this can be done by the type checker, right?)

but what if types change?

assume that starting in V19, the request body contains an different
type B V19 /= B V18.

then, where we previously called vclient to construct a client
function c that could handle all supported versions, we now call
vclient twice, once for V18 and once for V19. the resulting
functions c1, c2 are called by a manually writte c, chosen based
on whether the dynamic version is <19 or not. all the marshalling
that cannot be automated happens inside c.

to provide type safety the list of suppored versions needs to be
passed to vclient explicitly, either in the form of a set or in the
form of lower and upper bounds. that would allow (a) making sure
vclient does not replace a good version in the path with a bad one,
leading to dynamic errors that may not be caught by our tests; and (b)
generating the types to witness that all supported versions from the
list / range are compatible behind the scenes.

what does this mean?

the developer doesn't have to do anything for any clients whose route
does not change between versions.

if the version changes, the developer has to generate the client
function for the new version and introduce / modify a wrapper to
contain one more case switch for the new range of compatible versions
just introduced. some manual work in this situation is unavoidable,
and the overhead to obtain type safety should be constant and
reasonable.

@fisx
Copy link
Contributor

fisx commented Mar 18, 2022

other alternatives:

  • just do clients all manually, but servers statically!
  • extend routing table with a case where instead of the type-level
    Version, we pass a Version on the value level (this breaks the
    constraint in the routing table, but i think that's fine). the type
    families of the types that change over versions have a case for
    Int that maps on Aeson.Value. so, still some manual work to be
    done here, but not as much as if we do everything manually (we just
    need to touch the stuff that changes in the range of supported
    versions).

@pcapriotti
Copy link
Contributor Author

This is not going to be very constructive feedback, but I'm afraid this approach would still result in a lot of boilerplate and manual wiring. We should probably go back to the drawing board and specify more precisely what we are looking for here in terms of type safety. Because if we decide we need to rely on tests anyway to have any meaningful guarantee that all this versioning system is actually ensuring some form of compatibility (and I think it's very likely), then we might as well scrap the whole typed mess and implement something at the Wai level instead.

@fisx
Copy link
Contributor

fisx commented Mar 18, 2022

This is not going to be very constructive feedback, but I'm afraid this approach would still result in a lot of boilerplate and manual wiring. We should probably go back to the drawing board and specify more precisely what we are looking for here in terms of type safety. Because if we decide we need to rely on tests anyway to have any meaningful guarantee that all this versioning system is actually ensuring some form of compatibility (and I think it's very likely), then we might as well scrap the whole typed mess and implement something at the Wai level instead.

Yes, we could make the version number dynamic, but I think the outcome would be worse:

  • no swagger docs that are version-specific and correct by construction
  • the trivial cases which I suspect represent the overwhelming majority (no change between versions; body json object in req or resp has new fields or old fields get deleted) are handled nicely, and the code that contains the semantics of the change can be encapsulated in one place which will make it easier to test.

IMO the first point alone justifies all the typed mess.

@pcapriotti
Copy link
Contributor Author

Closing in favour of #2297.

@pcapriotti pcapriotti closed this Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bit-rot PRs that have bit-rotted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants