Skip to content

Commit

Permalink
fix: Fail Early on Invalid Connector Key
Browse files Browse the repository at this point in the history 
We observed, that the connector key is not or not always validated as
expected. Instead, for example when trying to retrieve a submission by
ID with an invalid connector key, it just returned "no submission found"
instead of complaining.

This caused, for example, endless retries when performing the manual
test for a failed cancelation communication. For the initial request
for a submission by ID, our facade instead returned some "mocked"
submission in state "OTHER", which subsequently is not handled by any
means within the workflow definition.
  • Loading branch information
@mmichaelis
mmichaelis committed Jan 16, 2025
1 parent 6eb0909 commit 4382622
Show file tree
Hide file tree
Showing 2 changed files with 127 additions and 74 deletions.
156 changes: 88 additions & 68 deletions .../src/main/java/com/coremedia/labs/translation/gcc/facade/def/DefaultGCExchangeFacade.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.gs4tr.gcc.restclient.dto.GCResponse;
import org.gs4tr.gcc.restclient.dto.MessageResponse;
import org.gs4tr.gcc.restclient.dto.PageableResponseData;
import org.gs4tr.gcc.restclient.model.Connector;
import org.gs4tr.gcc.restclient.model.ContentLocales;
import org.gs4tr.gcc.restclient.model.GCSubmission;
import org.gs4tr.gcc.restclient.model.GCTask;
Expand Down Expand Up @@ -111,19 +112,38 @@ public class DefaultGCExchangeFacade implements GCExchangeFacade {
gcConfig.setConnectorKey(connectorKey);
// Redirect logging to SLF4j.
gcConfig.setLogger(SLF4JHandler.getLogger(GCExchange.class));
gcConfig.getLogger().fine("JUL Logging redirection to SLF4J: OK");
LOG.debug("JUL Logging redirected to SLF4J.");
gcConfig.getLogger().finest("JUL Logging redirection to SLF4J: OK");
LOG.trace("JUL Logging redirected to SLF4J.");
delegate = new GCExchange(gcConfig);
validateConnectorKey(delegate);
} catch (RuntimeException e) {
throw new GCFacadeCommunicationException(e, "Failed to connect to GCC at %s.", apiUrl);
} catch (IllegalAccessError e) {
throw new GCFacadeAccessException(e, "Cannot authenticate with API key.");
}
fileTypeSupplier = Suppliers.memoize(() -> getSupportedFileType(
String.valueOf(config.get(GCConfigProperty.KEY_FILE_TYPE)))
String.valueOf(config.get(GCConfigProperty.KEY_FILE_TYPE)))
);
}

/**
* GCC backend does not validate the connector key during connection setup.
* Not validating it upfront may lead to unexpected states, such as that we
* get just no submission for a given ID instead of any failure signal.
* <p>
* This validation is meant to fail-fast and to prevent unexpected, hard to
* debug states.
*
* @param gcExchange GCC exchange instance to validate
*/
private static void validateConnectorKey(@NonNull GCExchange gcExchange) {
String configuredKey = gcExchange.getConfig().getConnectorKey();
List<String> availableConnectorKeys = gcExchange.getConnectors().stream().map(Connector::getConnectorKey).toList();
if (!availableConnectorKeys.contains(configuredKey)) {
throw new IllegalStateException("Connector key is unavailable in GCC (url=%s).".formatted(gcExchange.getConfig().getApiUrl()));
}
}

@VisibleForTesting
DefaultGCExchangeFacade(GCExchange delegate, String fileType) {
this.delegate = delegate;
Expand All @@ -135,7 +155,7 @@ private static String requireNonNullConfig(Map<String, Object> config, String ke
Object value = config.get(key);
if (value == null) {
throw new GCFacadeConfigException("Configuration for %s is missing. Configuration (values hidden): %s", key, config.entrySet().stream()
.collect(toMap(Map.Entry::getKey, e -> GCConfigProperty.KEY_URL.equals(e.getKey()) ? e.getValue() : "*****"))
.collect(toMap(Map.Entry::getKey, e -> GCConfigProperty.KEY_URL.equals(e.getKey()) ? e.getValue() : "*****"))
);
}
return String.valueOf(value);
Expand Down Expand Up @@ -168,21 +188,21 @@ public long submitSubmission(@Nullable String subject, @Nullable String comment,
Map<String, List<Locale>> contentMap) {

List<ContentLocales> contentLocalesList = contentMap.entrySet().stream()
.map(e ->
new ContentLocales(
e.getKey(),
e.getValue().stream().map(Locale::toLanguageTag).collect(toList())))
.collect(toList());
.map(e ->
new ContentLocales(
e.getKey(),
e.getValue().stream().map(Locale::toLanguageTag).collect(toList())))
.collect(toList());

SubmissionSubmitRequest request = new SubmissionSubmitRequest(
createSubmissionName(subject, sourceLocale, contentMap),
// REST API documents using UTC, Java REST Client API (v3.1.3)
// uses local time zone instead. This may cause an
// `IllegalArgumentException` if the due date is set to today with
// only some hours offset.
GCUtil.toUnixDateUtc(dueDate),
sourceLocale.toLanguageTag(),
contentLocalesList
createSubmissionName(subject, sourceLocale, contentMap),
// REST API documents using UTC, Java REST Client API (v3.1.3)
// uses local time zone instead. This may cause an
// `IllegalArgumentException` if the due date is set to today with
// only some hours offset.
GCUtil.toUnixDateUtc(dueDate),
sourceLocale.toLanguageTag(),
contentLocalesList
);
if (comment != null) {
request.setInstructions(comment);
Expand All @@ -199,7 +219,7 @@ public long submitSubmission(@Nullable String subject, @Nullable String comment,
return response.getSubmissionId();
} catch (RuntimeException e) {
throw new GCFacadeCommunicationException(e, "Failed to create submission: subject=%s, source-locale=%s",
subject, sourceLocale.toLanguageTag());
subject, sourceLocale.toLanguageTag());
}
}

Expand All @@ -223,11 +243,11 @@ public int cancelSubmission(long submissionId) {

private static String gcResponseToString(GCResponse response) {
return com.google.common.base.MoreObjects.toStringHelper(response)
.add("status", response.getStatus())
.add("statusCode", response.getStatusCode())
.add("error", response.getError())
.add("message", response.getMessage())
.toString();
.add("status", response.getStatus())
.add("statusCode", response.getStatusCode())
.add("error", response.getError())
.add("message", response.getMessage())
.toString();
}

/**
Expand All @@ -250,19 +270,19 @@ private static String createSubmissionName(@Nullable String subject,
} else {
String truncatedSubject = trimmedSubject.substring(0, SUBMISSION_NAME_MAX_LENGTH);
LOG.warn("Given subject exceeds maximum length of {}. Will truncate subject and skip adding further information: {} → {}",
SUBMISSION_NAME_MAX_LENGTH,
trimmedSubject,
truncatedSubject);
SUBMISSION_NAME_MAX_LENGTH,
trimmedSubject,
truncatedSubject);
trimmedSubject = truncatedSubject;
}
return trimmedSubject;
}

String allTargetLocales = contentMap.entrySet().stream()
.flatMap(e -> e.getValue().stream())
.distinct()
.map(Locale::toLanguageTag)
.collect(joining(", "));
.flatMap(e -> e.getValue().stream())
.distinct()
.map(Locale::toLanguageTag)
.collect(joining(", "));

if (trimmedSubject.isEmpty()) {
trimmedSubject = Instant.now().toString();
Expand Down Expand Up @@ -316,16 +336,16 @@ private void downloadTask(GCTaskModel task, BiPredicate<? super InputStream, ? s
@Override
public void confirmCancelledTasks(long submissionId) {
Map<TaskStatus, Set<GCTaskModel>> tasksByState =
getTasksByState(submissionId,
// Ignore tasks which got already confirmed as being cancelled.
r -> r.setIsCancelConfirmed(0),
Cancelled
);
getTasksByState(submissionId,
// Ignore tasks which got already confirmed as being cancelled.
r -> r.setIsCancelConfirmed(0),
Cancelled
);
List<GCTaskModel> tasks = new ArrayList<>(tasksByState.getOrDefault(Cancelled, emptySet()));

List<Long> taskIds = tasks.stream()
.map(GCTaskModel::getTaskId)
.collect(toList());
.map(GCTaskModel::getTaskId)
.collect(toList());

LOG.debug("Canceling Task IDs of submission {}: {}", submissionId, taskIds);
confirmTaskCancellations(taskIds);
Expand Down Expand Up @@ -380,8 +400,8 @@ private Map<TaskStatus, Set<GCTaskModel>> getTasksByState(long submissionId,
Map<TaskStatus, Set<GCTaskModel>> tasksByState = new EnumMap<>(TaskStatus.class);

GCUtil.processAllPages(
() -> createTaskListRequestBase(submissionId, requestPreProcessor, taskStates),
r -> executeRequest(r, tasksByState)
() -> createTaskListRequestBase(submissionId, requestPreProcessor, taskStates),
r -> executeRequest(r, tasksByState)
);

return tasksByState;
Expand Down Expand Up @@ -435,10 +455,10 @@ private PageableResponseData executeRequest(TaskListRequest request, Map<TaskSta
Locale localeFromGCCTask = new Locale.Builder().setLanguageTag(t.getTargetLocale().getLocale()).build();
GCTaskModel gcTaskModel = new GCTaskModel(t.getTaskId(), localeFromGCCTask);
tasksByState.merge(TaskStatus.valueOf(t.getState()), Sets.newHashSet(gcTaskModel),
(oldValue, newValue) -> {
oldValue.addAll(newValue);
return oldValue;
}
(oldValue, newValue) -> {
oldValue.addAll(newValue);
return oldValue;
}
);
} catch (IllformedLocaleException exception) {
LOG.error("Failed to convert LanguageTag tag from GCCTask with ID {}", t.getTaskId());
Expand Down Expand Up @@ -498,10 +518,10 @@ public GCSubmissionModel getSubmission(long submissionId) {
}
}
return GCSubmissionModel.builder(submissionId)
.pdSubmissionIds(submission.getPdSubmissionIds().keySet())
.state(state)
.submitter(submission.getSubmitter())
.build();
.pdSubmissionIds(submission.getPdSubmissionIds().keySet())
.state(state)
.submitter(submission.getSubmitter())
.build();
}

/**
Expand All @@ -515,23 +535,23 @@ public GCSubmissionModel getSubmission(long submissionId) {
private boolean areAllSubmissionTasksDone(long submissionId) {
AtomicBoolean allDone = new AtomicBoolean(true);
GCUtil.processAllPages(
() -> createTaskListRequestBase(submissionId),
r -> executeRequest(r, t -> {
TaskStatus status = TaskStatus.valueOf(t.getState());
LOG.debug("Retrieved status \"{}\" of task {} of submission {}", status.text(), t.getTaskId(), submissionId);
switch (status) {
case Delivered:
break;
case Cancelled:
LOG.debug("Verifying cancelation of task {} got confirmed -> {}", t.getTaskId(), t.getIsCancelConfirmed());
// Logical AND: Only use confirmed state, if value
// is still true. Otherwise, keep false state.
allDone.compareAndSet(true, t.getIsCancelConfirmed());
break;
default:
allDone.set(false);
}
})
() -> createTaskListRequestBase(submissionId),
r -> executeRequest(r, t -> {
TaskStatus status = TaskStatus.valueOf(t.getState());
LOG.debug("Retrieved status \"{}\" of task {} of submission {}", status.text(), t.getTaskId(), submissionId);
switch (status) {
case Delivered:
break;
case Cancelled:
LOG.debug("Verifying cancelation of task {} got confirmed -> {}", t.getTaskId(), t.getIsCancelConfirmed());
// Logical AND: Only use confirmed state, if value
// is still true. Otherwise, keep false state.
allDone.compareAndSet(true, t.getIsCancelConfirmed());
break;
default:
allDone.set(false);
}
})
);

return allDone.get();
Expand Down Expand Up @@ -562,9 +582,9 @@ private GCSubmission getSubmissionById(long submissionId) {
}
if (responseData.getTotalResultPagesCount() > 1L) {
LOG.warn(
"More than one submission ({}) returned for the same ID {}. Will choose the first one.",
responseData.getTotalResultPagesCount(),
submissionId
"More than one submission ({}) returned for the same ID {}. Will choose the first one.",
responseData.getTotalResultPagesCount(),
submissionId
);
}
return submissions.get(0);
Expand Down Expand Up @@ -593,7 +613,7 @@ private String getSupportedFileType(String configuredFileType) {
result = configuredFileType;
} else {
throw new GCFacadeFileTypeConfigException("Configured file type '%s' not in supported ones %s for GlobalLink " +
"connection at %s", configuredFileType, supportedFileTypes, apiUrl);
"connection at %s", configuredFileType, supportedFileTypes, apiUrl);
}

LOG.info("Using file type '{}' for uploading data to GlobalLink at {}", result, apiUrl);
Expand Down
45 changes: 39 additions & 6 deletions ...va/com/coremedia/labs/translation/gcc/facade/def/DefaultGCExchangeFacadeContractTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.coremedia.labs.translation.gcc.facade.GCConfigProperty;
import com.coremedia.labs.translation.gcc.facade.GCExchangeFacade;
import com.coremedia.labs.translation.gcc.facade.GCFacadeAccessException;
import com.coremedia.labs.translation.gcc.facade.GCFacadeCommunicationException;
import com.coremedia.labs.translation.gcc.facade.GCSubmissionState;
import com.coremedia.labs.translation.gcc.facade.GCTaskModel;
Expand Down Expand Up @@ -52,6 +53,7 @@
import static java.util.Collections.singletonList;
import static java.util.Collections.singletonMap;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;
import static org.awaitility.Awaitility.await;
import static org.slf4j.LoggerFactory.getLogger;

Expand Down Expand Up @@ -99,12 +101,43 @@ class DefaultGCExchangeFacadeContractTest {
private static final long TRANSLATION_TIMEOUT_MINUTES = 30L;
private static final long SUBMISSION_VALID_TIMEOUT_MINUTES = 2L;

@Test
@DisplayName("Validate that login works.")
void login(Map<String, Object> gccProperties) {
LOG.info("Properties: {}", gccProperties);
GCExchangeFacade facade = new DefaultGCExchangeFacade(gccProperties);
assertThat(facade.getDelegate()).isNotNull();
@Nested
@DisplayName("Tests for login")
class Login {
@Test
@DisplayName("Validate that login works.")
void shouldLoginSuccessfully(@NonNull Map<String, Object> gccProperties) {
LOG.info("Properties: {}", gccProperties);
assertThatCode(() -> new DefaultGCExchangeFacade(gccProperties))
.doesNotThrowAnyException();
}

@Test
@DisplayName("Validate that invalid login is denied.")
void shouldFailToLoginWithInvalidApiKey(@NonNull Map<String, Object> gccProperties) {
Map<String, Object> patchedProperties = new HashMap<>(gccProperties);
patchedProperties.put("apiKey", "invalid");
LOG.info("Properties: {} patched to {}", gccProperties, patchedProperties);
assertThatCode(() -> new DefaultGCExchangeFacade(patchedProperties))
.isInstanceOf(GCFacadeAccessException.class)
.hasCauseInstanceOf(IllegalAccessError.class);
}

/**
* We cannot trust GCC to validate the connector key in every situation.
* To prevent unexpected, hard to handle results, that do not expose an
* issue with the connector key (such as when trying to retrieve a
* submission by ID), we validate the connector key initially instead.
*/
@Test
void shouldValidateConnectorKeyInitially(@NonNull Map<String, Object> gccProperties) {
Map<String, Object> patchedProperties = new HashMap<>(gccProperties);
patchedProperties.put("key", "invalid");
LOG.info("Properties: {} patched to {}", gccProperties, patchedProperties);
assertThatCode(() -> new DefaultGCExchangeFacade(patchedProperties))
.isInstanceOf(GCFacadeCommunicationException.class)
.hasCauseInstanceOf(IllegalStateException.class);
}
}

@Nested
Expand Down

0 comments on commit 4382622

Please sign in to comment.