Add Workspace and Connection Geography Support to API #17650

pmossman · 2022-10-05T23:35:47Z

What

Addresses https://github.com/airbytehq/airbyte-cloud/issues/2278

How

Adds new endpoint v1/web_backend/geographies/list which returns available Geographies
- Will be just ['auto'] in OSS, and ['auto', 'us', 'eu'] in Cloud
Modifies /v1/workspaces/update to support a new property called defaultGeography
- This endpoint is already PATCH-style, which means:
  - If this property is present on the update request, will set the workspace's defaultGeography column to the value
  - If this property is not present on the update request, will leave the workspace's current defaultGeography unmodified
Modifies the /v1/connections/create and /v1/web_backend/connections/create endpoint to support a new optional property called geography
- If present on the create request, will set the Connection's geography column to the value
- If not present, the Connection will use whatever is set on its workspace's defaultGeography.
Modifies the /v1/connections/update and /v1/web_backend/connections/update endpoint to support a new optional property called geography
- This endpoint is already PATCH-style, which means:
  - If this property is present, will set the Connection's 'geography' column to the value
  - If not present, will leave the Connection's 'geography' unmodified

Recommended reading order

Start with the various .yaml files that define the Geography type and various schema updates
WebBackendGeographiesHandler.java
ConnectionsHandler.java
WorkspacesHandler.java
Other non-test changes, all relatively minor
Tests

davinchia · 2022-10-06T23:41:30Z

airbyte-bootloader/src/main/java/io/airbyte/bootloader/BootloaderApp.java

@@ -286,7 +287,8 @@ private static void createWorkspaceIfNoneExists(final ConfigRepository configRep
        .withSlug(workspaceId.toString())
        .withInitialSetupComplete(false)
        .withDisplaySetupWizard(true)
-        .withTombstone(false);
+        .withTombstone(false)
+        .withDefaultGeography(Geography.AUTO);


airbyte-server/src/main/java/io/airbyte/server/handlers/WebBackendGeographiesHandler.java

terencecho

lgtm, left a couple of comments. I think it's probably worth waiting for someone on FE to give another approval before merging.

airbyte-api/src/main/openapi/config.yaml

ambirdsall

The API contracts look reasonable to me. I like that it seems the frontend can use the same code path for OSS and cloud, and will simply fetch different geography lists based on context (hiding a geography selection UI if the only geography is auto is a reasonable piece of busines logic, and cleaner than hardcoding distinct OSS/cloud behaviors); easy to work with and extend in various directions as the product evolves.

My only issue is fetching data with a POST verb, but that's a sufficiently well-established convention that it wouldn't be appropriate to add in an ad hoc one-off GET endpoint.

davinchia · 2022-10-10T18:55:28Z

airbyte-server/src/main/java/io/airbyte/server/handlers/WebBackendGeographiesHandler.java

+  /**
+   * Only called by the wrapped Cloud API to enable multi-cloud
+   */
+  public WebBackendGeographiesListResult listGeographiesCloud() {


remind me: why/how does the Cloud api call this?

Is the idea to explicitly call this method in the wrapped server in Cloud?

Yeah exactly, in the Cloud wrapped server, we'd call this specific endpoint instead of the OSS-specific endpoint. I think this is a short-term solution that we'll iterate on once we have more concrete needs around specialized OSS use-cases. I think it should serve our needs for now and should be pretty easy to adjust in the future

davinchia

Looks sane to me!

One question to make sure I understand how we want to call this in Cloud.

pmossman · 2022-10-10T20:33:46Z

My only issue is fetching data with a POST verb, but that's a sufficiently well-established convention that it wouldn't be appropriate to add in an ad hoc one-off GET endpoint.

@ambirdsall definitely agree with this - we have some lower-level server code that assumes every API call is a POST for logging and redaction of sensitive parameters, so that's the convention we use for now. I think there's growing consensus that this is something we'll want to change eventually but for now I think following the established convention makes sense

* progress on adding geography throughout api * fix workspace handler test * more progress * implement workspace defaulting and add/update more tests * fix bootloader tests * set defaultGeography in missing places * add Geography column when reading Connection record from DB * fix pmd * add more comments/description * format * description

pmossman requested review from timroes, terencecho, ambirdsall and davinchia October 5, 2022 23:35

github-actions bot added area/api Related to the api area/documentation Improvements or additions to documentation area/platform issues related to the platform area/server labels Oct 5, 2022

pmossman force-pushed the parker/geograph-api-endpoints branch from d19b471 to 909f342 Compare October 5, 2022 23:53

pmossman temporarily deployed to more-secrets October 5, 2022 23:54 Inactive

pmossman temporarily deployed to more-secrets October 6, 2022 00:04 Inactive

pmossman temporarily deployed to more-secrets October 6, 2022 17:51 Inactive

pmossman force-pushed the parker/geograph-api-endpoints branch from 8c492df to 1b4d45c Compare October 6, 2022 18:11

pmossman temporarily deployed to more-secrets October 6, 2022 18:13 Inactive

pmossman force-pushed the parker/geograph-api-endpoints branch from 1b4d45c to e5afee7 Compare October 6, 2022 19:10

pmossman temporarily deployed to more-secrets October 6, 2022 19:12 Inactive

davinchia reviewed Oct 6, 2022

View reviewed changes

airbyte-server/src/main/java/io/airbyte/server/handlers/WebBackendGeographiesHandler.java Outdated Show resolved Hide resolved

terencecho approved these changes Oct 7, 2022

View reviewed changes

airbyte-api/src/main/openapi/config.yaml Outdated Show resolved Hide resolved

airbyte-api/src/main/openapi/config.yaml Show resolved Hide resolved

pmossman force-pushed the parker/geograph-api-endpoints branch from e5afee7 to b68ed71 Compare October 7, 2022 23:42

pmossman temporarily deployed to more-secrets October 7, 2022 23:45 Inactive

pmossman added 9 commits October 10, 2022 08:39

progress on adding geography throughout api

c1952c0

fix workspace handler test

9bab3ea

more progress

933b5cf

implement workspace defaulting and add/update more tests

f462b13

fix bootloader tests

4df5b4f

set defaultGeography in missing places

5297015

add Geography column when reading Connection record from DB

3ce8928

fix pmd

718bb01

add more comments/description

cd62540

pmossman added 2 commits October 10, 2022 08:39

format

7d8de3f

description

020edce

pmossman force-pushed the parker/geograph-api-endpoints branch from b68ed71 to 020edce Compare October 10, 2022 15:54

pmossman temporarily deployed to more-secrets October 10, 2022 15:56 Inactive

ambirdsall approved these changes Oct 10, 2022

View reviewed changes

davinchia reviewed Oct 10, 2022

View reviewed changes

davinchia approved these changes Oct 10, 2022

View reviewed changes

pmossman merged commit fb9efb3 into master Oct 10, 2022

pmossman deleted the parker/geograph-api-endpoints branch October 10, 2022 20:34

This was referenced Oct 13, 2022

Bump Airbyte version from 0.40.14 to 0.40.15 #17961

Closed

Bump Airbyte version from 0.40.14 to 0.40.15 #17970

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Workspace and Connection Geography Support to API #17650

Add Workspace and Connection Geography Support to API #17650

pmossman commented Oct 5, 2022

davinchia Oct 6, 2022

terencecho left a comment

ambirdsall left a comment

davinchia Oct 10, 2022

davinchia Oct 10, 2022

pmossman Oct 10, 2022

davinchia left a comment

pmossman commented Oct 10, 2022

Add Workspace and Connection Geography Support to API #17650

Add Workspace and Connection Geography Support to API #17650

Conversation

pmossman commented Oct 5, 2022

What

How

Recommended reading order

davinchia Oct 6, 2022

Choose a reason for hiding this comment

terencecho left a comment

Choose a reason for hiding this comment

ambirdsall left a comment

Choose a reason for hiding this comment

davinchia Oct 10, 2022

Choose a reason for hiding this comment

davinchia Oct 10, 2022

Choose a reason for hiding this comment

pmossman Oct 10, 2022

Choose a reason for hiding this comment

davinchia left a comment

Choose a reason for hiding this comment

pmossman commented Oct 10, 2022