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.

Sign up for GitHub

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

ensure workspace webhook configs can be persisted correctly #18034

Merged
merged 4 commits into from
Oct 16, 2022
Merged

ensure workspace webhook configs can be persisted correctly #18034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
395a7fa
ensure workspace webhook configs can be correctly passed between API …
mfsiega-airbyte Oct 16, 2022
3c37369
remove unnecessary logging
mfsiega-airbyte Oct 16, 2022
c4cdaa3
add unit tests to workspace webhook config handling
mfsiega-airbyte Oct 16, 2022
0760fcc
additional testing and style cleanup around workspace webhook config …
mfsiega-airbyte Oct 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions airbyte-api/src/main/openapi/config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2454,6 +2454,10 @@ components:
$ref: "#/components/schemas/Notification"
defaultGeography:
$ref: "#/components/schemas/Geography"
webhookConfigs:
davinchia marked this conversation as resolved.
Show resolved Hide resolved
type: array
items:
$ref: "#/components/schemas/WebhookConfigWrite"
WorkspaceGiveFeedback:
type: object
required:
Expand Down
4 changes: 3 additions & 1 deletion ...te-config/config-persistence/src/main/java/io/airbyte/config/persistence/DbConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,9 @@ public static StandardWorkspace buildStandardWorkspace(final Record record) {
.withFirstCompletedSync(record.get(WORKSPACE.FIRST_SYNC_COMPLETE))
.withFeedbackDone(record.get(WORKSPACE.FEEDBACK_COMPLETE))
.withDefaultGeography(
Enums.toEnum(record.get(WORKSPACE.GEOGRAPHY, String.class), Geography.class).orElseThrow());
Enums.toEnum(record.get(WORKSPACE.GEOGRAPHY, String.class), Geography.class).orElseThrow())
.withWebhookOperationConfigs(record.get(WORKSPACE.WEBHOOK_OPERATION_CONFIGS) == null ? null
: Jsons.deserialize(record.get(WORKSPACE.WEBHOOK_OPERATION_CONFIGS).data()));
}

public static SourceConnection buildSourceConnection(final Record record) {
Expand Down
6 changes: 5 additions & 1 deletion airbyte-config/config-persistence/src/test/java/io/airbyte/config/persistence/MockData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
import io.airbyte.config.StandardSyncState;
import io.airbyte.config.StandardWorkspace;
import io.airbyte.config.State;
import io.airbyte.config.WebhookConfig;
import io.airbyte.config.WebhookOperationConfigs;
import io.airbyte.config.WorkspaceServiceAccount;
import io.airbyte.protocol.models.AirbyteCatalog;
import io.airbyte.protocol.models.AuthSpecification;
Expand Down Expand Up @@ -156,7 +158,9 @@ public static List<StandardWorkspace> standardWorkspaces() {
.withNotifications(Collections.singletonList(notification))
.withFirstCompletedSync(true)
.withFeedbackDone(true)
.withDefaultGeography(Geography.AUTO);
.withDefaultGeography(Geography.AUTO)
.withWebhookOperationConfigs(Jsons.jsonNode(
new WebhookOperationConfigs().withWebhookConfigs(List.of(new WebhookConfig().withId(WEBHOOK_CONFIG_ID).withName("name")))));

final StandardWorkspace workspace2 = new StandardWorkspace()
.withWorkspaceId(WORKSPACE_ID_2)
Expand Down
52 changes: 42 additions & 10 deletions ...e-server/src/main/java/io/airbyte/server/converters/WorkspaceWebhookConfigsConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,42 +10,74 @@
import io.airbyte.commons.json.Jsons;
import io.airbyte.config.WebhookConfig;
import io.airbyte.config.WebhookOperationConfigs;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import java.util.UUID;
import java.util.function.Supplier;
import java.util.stream.Collectors;

// NOTE: we suppress this warning because PMD thinks it can be a foreach loop in toApiReads but the
// compiler disagrees.
@SuppressWarnings("PMD.ForLoopCanBeForeach")
public class WorkspaceWebhookConfigsConverter {

public static JsonNode toPersistenceWrite(List<WebhookConfigWrite> apiWebhookConfigs) {
public static JsonNode toPersistenceWrite(List<WebhookConfigWrite> apiWebhookConfigs, Supplier<UUID> uuidSupplier) {
if (apiWebhookConfigs == null) {
return Jsons.emptyObject();
}

final WebhookOperationConfigs configs = new WebhookOperationConfigs()
.withWebhookConfigs(apiWebhookConfigs.stream().map(WorkspaceWebhookConfigsConverter::toPersistenceConfig).collect(Collectors.toList()));
.withWebhookConfigs(apiWebhookConfigs.stream().map((item) -> toPersistenceConfig(uuidSupplier, item)).collect(Collectors.toList()));

return Jsons.jsonNode(configs);
}

public static List<WebhookConfigRead> toApiReads(List<WebhookConfig> persistenceConfig) {
if (persistenceConfig.isEmpty()) {
/**
* Extract the read-only properties from a set of persisted webhook operation configs.
* <p>
* Specifically, returns the id and name but excludes the secret auth token. Note that we "manually"
* deserialize the JSON tree instead of deserializing to our internal schema --
* WebhookOperationConfigs -- because the persisted JSON doesn't conform to that schema until we
* hydrate the secrets. Since we don't want to unnecessarily hydrate the secrets to read from the
* API, we do this instead.
* <p>
* TODO(mfsiega-airbyte): try find a cleaner way to handle this situation.
*
* @param persistedWebhookConfig - The JsonNode of the persisted webhook configs
* @return a list of (webhook id, name) pairs
*/
public static List<WebhookConfigRead> toApiReads(final JsonNode persistedWebhookConfig) {
if (persistedWebhookConfig == null) {
return Collections.emptyList();
}
return persistenceConfig.stream().map(WorkspaceWebhookConfigsConverter::toApiRead).collect(Collectors.toList());

// NOTE: we deserialize it "by hand" because the secrets aren't hydrated, so we can't deserialize it
// into the usual shape.
// TODO(mfsiega-airbyte): find a cleaner way to handle this situation.
List<WebhookConfigRead> configReads = new ArrayList<>();

final JsonNode configArray = persistedWebhookConfig.findPath("webhookConfigs");
Iterator<JsonNode> it = configArray.elements();
while (it.hasNext()) {
JsonNode webhookConfig = it.next();
configReads.add(toApiRead(webhookConfig));
}
return configReads;
}

private static WebhookConfig toPersistenceConfig(final WebhookConfigWrite input) {
private static WebhookConfig toPersistenceConfig(final Supplier<UUID> uuidSupplier, final WebhookConfigWrite input) {
return new WebhookConfig()
.withId(UUID.randomUUID())
.withId(uuidSupplier.get())
.withName(input.getName())
.withAuthToken(input.getAuthToken());
}

private static WebhookConfigRead toApiRead(final WebhookConfig persistenceConfig) {
private static WebhookConfigRead toApiRead(final JsonNode configJson) {
final var read = new WebhookConfigRead();
read.setId(persistenceConfig.getId());
read.setName(persistenceConfig.getName());
read.setId(UUID.fromString(configJson.findValue("id").asText()));
read.setName(configJson.findValue("name").asText());
davinchia marked this conversation as resolved.
Show resolved Hide resolved
return read;
}

Expand Down
30 changes: 14 additions & 16 deletions airbyte-server/src/main/java/io/airbyte/server/handlers/WorkspacesHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package io.airbyte.server.handlers;

import com.github.slugify.Slugify;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
import io.airbyte.analytics.TrackingClientSingleton;
Expand All @@ -24,9 +25,7 @@
import io.airbyte.api.model.generated.WorkspaceUpdate;
import io.airbyte.api.model.generated.WorkspaceUpdateName;
import io.airbyte.commons.enums.Enums;
import io.airbyte.commons.json.Jsons;
import io.airbyte.config.StandardWorkspace;
import io.airbyte.config.WebhookOperationConfigs;
import io.airbyte.config.persistence.ConfigNotFoundException;
import io.airbyte.config.persistence.ConfigRepository;
import io.airbyte.config.persistence.SecretsRepositoryWriter;
Expand All @@ -39,7 +38,6 @@
import io.airbyte.validation.json.JsonValidationException;
import java.io.IOException;
import java.util.List;
import java.util.Optional;
import java.util.UUID;
import java.util.function.Supplier;
import java.util.stream.Collectors;
Expand All @@ -66,12 +64,13 @@ public WorkspacesHandler(final ConfigRepository configRepository,
this(configRepository, secretsRepositoryWriter, connectionsHandler, destinationHandler, sourceHandler, UUID::randomUUID);
}

public WorkspacesHandler(final ConfigRepository configRepository,
final SecretsRepositoryWriter secretsRepositoryWriter,
final ConnectionsHandler connectionsHandler,
final DestinationHandler destinationHandler,
final SourceHandler sourceHandler,
final Supplier<UUID> uuidSupplier) {
@VisibleForTesting
WorkspacesHandler(final ConfigRepository configRepository,
final SecretsRepositoryWriter secretsRepositoryWriter,
final ConnectionsHandler connectionsHandler,
final DestinationHandler destinationHandler,
final SourceHandler sourceHandler,
final Supplier<UUID> uuidSupplier) {
this.configRepository = configRepository;
this.secretsRepositoryWriter = secretsRepositoryWriter;
this.connectionsHandler = connectionsHandler;
Expand Down Expand Up @@ -108,7 +107,7 @@ public WorkspaceRead createWorkspace(final WorkspaceCreate workspaceCreate)
.withTombstone(false)
.withNotifications(NotificationConverter.toConfigList(workspaceCreate.getNotifications()))
.withDefaultGeography(defaultGeography)
.withWebhookOperationConfigs(WorkspaceWebhookConfigsConverter.toPersistenceWrite(workspaceCreate.getWebhookConfigs()));
.withWebhookOperationConfigs(WorkspaceWebhookConfigsConverter.toPersistenceWrite(workspaceCreate.getWebhookConfigs(), uuidSupplier));
Copy link
Contributor

@davinchia davinchia Oct 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

one thought: can we rename the uuidSupplier variable to reflect this is specifically to provide a uuid for the webhook config objects? Not necessarily related to this change, however will help readability.

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 related to your change, can you add the @VisibleForTesting annotation on the construction at Line 66 so it's obvious this is for testing? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one thought: can we rename the uuidSupplier variable to reflect this is specifically to provide a uuid for the webhook config objects?

We do actually use it for other things -- e.g. generating new workspace ids -- so I think leaving it makes sense? (Though I'm still confused as to why we're doing that.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh interesting. Leaving it is good then!


if (!Strings.isNullOrEmpty(email)) {
workspace.withEmail(email);
Expand Down Expand Up @@ -269,12 +268,8 @@ private static WorkspaceRead buildWorkspaceRead(final StandardWorkspace workspac
.notifications(NotificationConverter.toApiList(workspace.getNotifications()))
.defaultGeography(Enums.convertTo(workspace.getDefaultGeography(), Geography.class));
// Add read-only webhook configs.
final Optional<WebhookOperationConfigs> persistedConfigs = Jsons.tryObject(
davinchia marked this conversation as resolved.
Show resolved Hide resolved
workspace.getWebhookOperationConfigs(),
WebhookOperationConfigs.class);
if (persistedConfigs.isPresent()) {
result.setWebhookConfigs(WorkspaceWebhookConfigsConverter.toApiReads(
persistedConfigs.get().getWebhookConfigs()));
if (workspace.getWebhookOperationConfigs() != null) {
result.setWebhookConfigs(WorkspaceWebhookConfigsConverter.toApiReads(workspace.getWebhookOperationConfigs()));
}
return result;
}
Expand Down Expand Up @@ -309,6 +304,9 @@ private void applyPatchToStandardWorkspace(final StandardWorkspace workspace, fi
workspace.setDefaultGeography(
Enums.convertTo(workspacePatch.getDefaultGeography(), io.airbyte.config.Geography.class));
}
if (workspacePatch.getWebhookConfigs() != null) {
mfsiega-airbyte marked this conversation as resolved.
Show resolved Hide resolved
workspace.setWebhookOperationConfigs(WorkspaceWebhookConfigsConverter.toPersistenceWrite(workspacePatch.getWebhookConfigs(), uuidSupplier));
}
}

private WorkspaceRead persistStandardWorkspace(final StandardWorkspace workspace) throws JsonValidationException, IOException {
Expand Down
34 changes: 27 additions & 7 deletions airbyte-server/src/test/java/io/airbyte/server/handlers/WorkspacesHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import com.fasterxml.jackson.databind.JsonNode;
import com.google.common.collect.Lists;
import io.airbyte.api.model.generated.ConnectionRead;
import io.airbyte.api.model.generated.ConnectionReadList;
Expand All @@ -22,6 +23,8 @@
import io.airbyte.api.model.generated.SlugRequestBody;
import io.airbyte.api.model.generated.SourceRead;
import io.airbyte.api.model.generated.SourceReadList;
import io.airbyte.api.model.generated.WebhookConfigRead;
import io.airbyte.api.model.generated.WebhookConfigWrite;
import io.airbyte.api.model.generated.WorkspaceCreate;
import io.airbyte.api.model.generated.WorkspaceGiveFeedback;
import io.airbyte.api.model.generated.WorkspaceIdRequestBody;
Expand All @@ -38,6 +41,7 @@
import io.airbyte.config.persistence.ConfigNotFoundException;
import io.airbyte.config.persistence.ConfigRepository;
import io.airbyte.config.persistence.SecretsRepositoryWriter;
import io.airbyte.config.persistence.split_secrets.SecretPersistence;
import io.airbyte.server.converters.NotificationConverter;
import io.airbyte.validation.json.JsonValidationException;
import java.io.IOException;
Expand All @@ -55,6 +59,11 @@ class WorkspacesHandlerTest {

public static final String FAILURE_NOTIFICATION_WEBHOOK = "http://airbyte.notifications/failure";
public static final String NEW_WORKSPACE = "new workspace";
public static final String TEST_NAME = "test-name";
private static final UUID WEBHOOK_CONFIG_ID = UUID.randomUUID();
private static final JsonNode PERSISTED_WEBHOOK_CONFIGS = Jsons.deserialize(
String.format("{\"webhookConfigs\": [{\"id\": \"%s\", \"name\": \"%s\", \"authToken\": {\"_secret\": \"a-secret_v1\"}}]}",
WEBHOOK_CONFIG_ID, TEST_NAME));
private ConfigRepository configRepository;
private SecretsRepositoryWriter secretsRepositoryWriter;
private ConnectionsHandler connectionsHandler;
Expand All @@ -72,12 +81,14 @@ class WorkspacesHandlerTest {
io.airbyte.api.model.generated.Geography.AUTO;
private static final io.airbyte.api.model.generated.Geography GEOGRAPHY_US =
io.airbyte.api.model.generated.Geography.US;
private SecretPersistence secretPersistence;

@SuppressWarnings("unchecked")
@BeforeEach
void setUp() {
configRepository = mock(ConfigRepository.class);
secretsRepositoryWriter = new SecretsRepositoryWriter(configRepository, Optional.empty(), Optional.empty());
secretPersistence = mock(SecretPersistence.class);
secretsRepositoryWriter = new SecretsRepositoryWriter(configRepository, Optional.of(secretPersistence), Optional.empty());
connectionsHandler = mock(ConnectionsHandler.class);
destinationHandler = mock(DestinationHandler.class);
sourceHandler = mock(SourceHandler.class);
Expand Down Expand Up @@ -121,6 +132,7 @@ private io.airbyte.api.model.generated.Notification generateApiNotification() {

@Test
void testCreateWorkspace() throws JsonValidationException, IOException, ConfigNotFoundException {
workspace.withWebhookOperationConfigs(PERSISTED_WEBHOOK_CONFIGS);
when(configRepository.getStandardWorkspaceNoSecrets(any(), eq(false))).thenReturn(workspace);

final UUID uuid = UUID.randomUUID();
Expand All @@ -135,7 +147,8 @@ void testCreateWorkspace() throws JsonValidationException, IOException, ConfigNo
.anonymousDataCollection(false)
.securityUpdates(false)
.notifications(List.of(generateApiNotification()))
.defaultGeography(GEOGRAPHY_US);
.defaultGeography(GEOGRAPHY_US)
.webhookConfigs(List.of(new WebhookConfigWrite().name(TEST_NAME).authToken("test-auth-token")));

final WorkspaceRead actualRead = workspacesHandler.createWorkspace(workspaceCreate);
final WorkspaceRead expectedRead = new WorkspaceRead()
Expand All @@ -151,7 +164,7 @@ void testCreateWorkspace() throws JsonValidationException, IOException, ConfigNo
.securityUpdates(false)
.notifications(List.of(generateApiNotification()))
.defaultGeography(GEOGRAPHY_US)
.webhookConfigs(Collections.emptyList());
.webhookConfigs(List.of(new WebhookConfigRead().id(uuid).name(TEST_NAME)));

assertEquals(expectedRead, actualRead);
}
Expand Down Expand Up @@ -275,6 +288,7 @@ void testListWorkspaces() throws JsonValidationException, IOException {

@Test
void testGetWorkspace() throws JsonValidationException, ConfigNotFoundException, IOException {
workspace.withWebhookOperationConfigs(PERSISTED_WEBHOOK_CONFIGS);
when(configRepository.getStandardWorkspaceNoSecrets(workspace.getWorkspaceId(), false)).thenReturn(workspace);

final WorkspaceIdRequestBody workspaceIdRequestBody = new WorkspaceIdRequestBody().workspaceId(workspace.getWorkspaceId());
Expand All @@ -291,7 +305,8 @@ void testGetWorkspace() throws JsonValidationException, ConfigNotFoundException,
.anonymousDataCollection(false)
.securityUpdates(false)
.notifications(List.of(generateApiNotification()))
.defaultGeography(GEOGRAPHY_AUTO);
.defaultGeography(GEOGRAPHY_AUTO)
.webhookConfigs(List.of(new WebhookConfigRead().id(WEBHOOK_CONFIG_ID).name(TEST_NAME)));

assertEquals(workspaceRead, workspacesHandler.getWorkspace(workspaceIdRequestBody));
}
Expand Down Expand Up @@ -330,7 +345,8 @@ void testUpdateWorkspace() throws JsonValidationException, ConfigNotFoundExcepti
.initialSetupComplete(true)
.displaySetupWizard(false)
.notifications(List.of(apiNotification))
.defaultGeography(GEOGRAPHY_US);
.defaultGeography(GEOGRAPHY_US)
.webhookConfigs(List.of(new WebhookConfigWrite().name(TEST_NAME).authToken("test-auth-token")));

final Notification expectedNotification = generateNotification();
expectedNotification.getSlackConfiguration().withWebhook("updated");
Expand All @@ -347,7 +363,10 @@ void testUpdateWorkspace() throws JsonValidationException, ConfigNotFoundExcepti
.withDisplaySetupWizard(false)
.withTombstone(false)
.withNotifications(List.of(expectedNotification))
.withDefaultGeography(Geography.US);
.withDefaultGeography(Geography.US)
.withWebhookOperationConfigs(PERSISTED_WEBHOOK_CONFIGS);

when(uuidSupplier.get()).thenReturn(WEBHOOK_CONFIG_ID);

when(configRepository.getStandardWorkspaceNoSecrets(workspace.getWorkspaceId(), false))
.thenReturn(workspace)
Expand All @@ -369,7 +388,8 @@ void testUpdateWorkspace() throws JsonValidationException, ConfigNotFoundExcepti
.anonymousDataCollection(true)
.securityUpdates(false)
.notifications(List.of(expectedNotificationRead))
.defaultGeography(GEOGRAPHY_US);
.defaultGeography(GEOGRAPHY_US)
.webhookConfigs(List.of(new WebhookConfigRead().name(TEST_NAME).id(WEBHOOK_CONFIG_ID)));

verify(configRepository).writeStandardWorkspaceNoSecrets(expectedWorkspace);

Expand Down
1 change: 1 addition & 0 deletions docs/reference/api/generated-api-html/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11578,6 +11578,7 @@ <h3><a name="WorkspaceUpdate"><code>WorkspaceUpdate</code> - </a> <a class="up"
<div class="param">securityUpdates (optional)</div><div class="param-desc"><span class="param-type"><a href="#boolean">Boolean</a></span> </div>
<div class="param">notifications (optional)</div><div class="param-desc"><span class="param-type"><a href="#Notification">array[Notification]</a></span> </div>
<div class="param">defaultGeography (optional)</div><div class="param-desc"><span class="param-type"><a href="#Geography">Geography</a></span> </div>
<div class="param">webhookConfigs (optional)</div><div class="param-desc"><span class="param-type"><a href="#WebhookConfigWrite">array[WebhookConfigWrite]</a></span> </div>
</div> <!-- field-items -->
</div>
<div class="model">
Expand Down