From 5baa3a7ccf9beea6aa8c5468655073f05f3e265c Mon Sep 17 00:00:00 2001 From: Igor Vinokur Date: Tue, 28 Nov 2023 18:09:57 +0200 Subject: [PATCH] Align the unreachable devfile exception message (#615) Add a common exception message and use it for cases when: The devfile location is unavailable. The devfile content is is not valid: e.g the response content is an html content of an scm authorisation page. --- .ci/openshift-ci/common.sh | 9 +++++++++ .../test-azure-no-pat-oauth-flow.sh | 3 +-- ...bucket-no-pat-oauth-flow-raw-devfile-url.sh | 2 +- ...github-no-pat-oauth-flow-raw-devfile-url.sh | 2 +- ...gitlab-no-pat-oauth-flow-raw-devfile-url.sh | 2 +- .../scm/AuthorizingFileContentProvider.java | 3 ++- .../scm/exception/ExceptionMessages.java | 18 ++++++++++++++++++ .../server/urlfactory/URLFactoryBuilder.java | 5 +++-- .../urlfactory/URLFactoryBuilderTest.java | 5 +++-- 9 files changed, 39 insertions(+), 10 deletions(-) create mode 100644 wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/scm/exception/ExceptionMessages.java diff --git a/.ci/openshift-ci/common.sh b/.ci/openshift-ci/common.sh index fc354957816..038acd18cc5 100644 --- a/.ci/openshift-ci/common.sh +++ b/.ci/openshift-ci/common.sh @@ -101,6 +101,15 @@ testFactoryResolverNoPatOAuth() { echo "[INFO] Check factory resolver for public repository with NO PAT/OAuth setup" testFactoryResolverResponse $1 200 + echo "[INFO] Check factory resolver for private repository with NO PAT/OAuth setup" + testFactoryResolverResponse $2 500 +} + +# check that raw devfile url factory resolver returns correct value without any PAT/OAuth setup +testFactoryResolverNoPatOAuthRaw() { + echo "[INFO] Check factory resolver for public repository with NO PAT/OAuth setup" + testFactoryResolverResponse $1 200 + echo "[INFO] Check factory resolver for private repository with NO PAT/OAuth setup" testFactoryResolverResponse $2 400 } diff --git a/.ci/openshift-ci/test-azure-no-pat-oauth-flow.sh b/.ci/openshift-ci/test-azure-no-pat-oauth-flow.sh index a6268176ca9..58c51470f45 100644 --- a/.ci/openshift-ci/test-azure-no-pat-oauth-flow.sh +++ b/.ci/openshift-ci/test-azure-no-pat-oauth-flow.sh @@ -26,8 +26,7 @@ source "${SCRIPT_DIR}"/common.sh trap "catchFinish" EXIT SIGINT setupTestEnvironment ${OCP_NON_ADMIN_USER_NAME} -# due to the issue https://github.com/eclipse/che/issues/22469 -# testFactoryResolverNoPatOAuth ${PUBLIC_REPO_URL} ${PRIVATE_REPO_URL} +testFactoryResolverNoPatOAuth ${PUBLIC_REPO_URL} ${PRIVATE_REPO_URL} testCloneGitRepoProjectShouldExists ${PUBLIC_REPO_WORKSPACE_NAME} ${PUBLIC_PROJECT_NAME} ${PUBLIC_REPO_URL} ${USER_CHE_NAMESPACE} deleteTestWorkspace ${PUBLIC_REPO_WORKSPACE_NAME} ${USER_CHE_NAMESPACE} testCloneGitRepoNoProjectExists ${PRIVATE_REPO_WORKSPACE_NAME} ${PRIVATE_PROJECT_NAME} ${PRIVATE_REPO_URL} ${USER_CHE_NAMESPACE} diff --git a/.ci/openshift-ci/test-bitbucket-no-pat-oauth-flow-raw-devfile-url.sh b/.ci/openshift-ci/test-bitbucket-no-pat-oauth-flow-raw-devfile-url.sh index 240b20183ef..ce8651bebb1 100644 --- a/.ci/openshift-ci/test-bitbucket-no-pat-oauth-flow-raw-devfile-url.sh +++ b/.ci/openshift-ci/test-bitbucket-no-pat-oauth-flow-raw-devfile-url.sh @@ -26,7 +26,7 @@ source "${SCRIPT_DIR}"/common.sh trap "catchFinish" EXIT SIGINT setupTestEnvironment ${OCP_NON_ADMIN_USER_NAME} -testFactoryResolverNoPatOAuth ${PUBLIC_REPO_RAW_PATH_URL} ${PRIVATE_REPO_RAW_PATH_URL} +testFactoryResolverNoPatOAuthRaw ${PUBLIC_REPO_RAW_PATH_URL} ${PRIVATE_REPO_RAW_PATH_URL} testCloneGitRepoProjectShouldExists ${PUBLIC_REPO_WORKSPACE_NAME} ${PUBLIC_PROJECT_NAME} ${PUBLIC_REPO_RAW_PATH_URL} ${USER_CHE_NAMESPACE} deleteTestWorkspace ${PUBLIC_REPO_WORKSPACE_NAME} ${USER_CHE_NAMESPACE} diff --git a/.ci/openshift-ci/test-github-no-pat-oauth-flow-raw-devfile-url.sh b/.ci/openshift-ci/test-github-no-pat-oauth-flow-raw-devfile-url.sh index a4bddfe32c2..747cb9c5d37 100644 --- a/.ci/openshift-ci/test-github-no-pat-oauth-flow-raw-devfile-url.sh +++ b/.ci/openshift-ci/test-github-no-pat-oauth-flow-raw-devfile-url.sh @@ -26,7 +26,7 @@ source "${SCRIPT_DIR}"/common.sh trap "catchFinish" EXIT SIGINT setupTestEnvironment ${OCP_NON_ADMIN_USER_NAME} -testFactoryResolverNoPatOAuth ${PUBLIC_REPO_RAW_PATH_URL} ${PRIVATE_REPO_RAW_PATH_URL} +testFactoryResolverNoPatOAuthRaw ${PUBLIC_REPO_RAW_PATH_URL} ${PRIVATE_REPO_RAW_PATH_URL} testCloneGitRepoNoProjectExists ${PUBLIC_REPO_WORKSPACE_NAME} ${PUBLIC_PROJECT_NAME} ${PUBLIC_REPO_RAW_PATH_URL} ${USER_CHE_NAMESPACE} deleteTestWorkspace ${PUBLIC_REPO_WORKSPACE_NAME} ${USER_CHE_NAMESPACE} diff --git a/.ci/openshift-ci/test-gitlab-no-pat-oauth-flow-raw-devfile-url.sh b/.ci/openshift-ci/test-gitlab-no-pat-oauth-flow-raw-devfile-url.sh index b12a21369ef..265390055ec 100644 --- a/.ci/openshift-ci/test-gitlab-no-pat-oauth-flow-raw-devfile-url.sh +++ b/.ci/openshift-ci/test-gitlab-no-pat-oauth-flow-raw-devfile-url.sh @@ -26,7 +26,7 @@ source "${SCRIPT_DIR}"/common.sh trap "catchFinish" EXIT SIGINT setupTestEnvironment ${OCP_NON_ADMIN_USER_NAME} -testFactoryResolverNoPatOAuth ${PUBLIC_REPO_RAW_PATH_URL} ${PRIVATE_REPO_RAW_PATH_URL} +testFactoryResolverNoPatOAuthRaw ${PUBLIC_REPO_RAW_PATH_URL} ${PRIVATE_REPO_RAW_PATH_URL} testCloneGitRepoNoProjectExists ${PUBLIC_REPO_WORKSPACE_NAME} ${PUBLIC_PROJECT_NAME} ${PUBLIC_REPO_RAW_PATH_URL} ${USER_CHE_NAMESPACE} deleteTestWorkspace ${PUBLIC_REPO_WORKSPACE_NAME} ${USER_CHE_NAMESPACE} diff --git a/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/scm/AuthorizingFileContentProvider.java b/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/scm/AuthorizingFileContentProvider.java index 1bb4fbcfa19..a4d3a457428 100644 --- a/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/scm/AuthorizingFileContentProvider.java +++ b/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/scm/AuthorizingFileContentProvider.java @@ -13,6 +13,7 @@ import static com.google.common.base.Strings.isNullOrEmpty; import static org.eclipse.che.api.factory.server.scm.PersonalAccessTokenFetcher.OAUTH_2_PREFIX; +import static org.eclipse.che.api.factory.server.scm.exception.ExceptionMessages.getDevfileConnectionErrorMessage; import java.io.FileNotFoundException; import java.io.IOException; @@ -123,7 +124,7 @@ protected String fetchContentWithoutToken(String requestURL) } } throw new DevfileException( - "Could not reach devfile at " + "`" + exception.getMessage() + "`", exception); + getDevfileConnectionErrorMessage(exception.getMessage()), exception); } } diff --git a/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/scm/exception/ExceptionMessages.java b/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/scm/exception/ExceptionMessages.java new file mode 100644 index 00000000000..6035050dbd7 --- /dev/null +++ b/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/scm/exception/ExceptionMessages.java @@ -0,0 +1,18 @@ +/* + * Copyright (c) 2012-2023 Red Hat, Inc. + * This program and the accompanying materials are made + * available under the terms of the Eclipse Public License 2.0 + * which is available at https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * Red Hat, Inc. - initial API and implementation + */ +package org.eclipse.che.api.factory.server.scm.exception; + +public class ExceptionMessages { + public static String getDevfileConnectionErrorMessage(String location) { + return String.format("Could not reach devfile at %s", location); + } +} diff --git a/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/urlfactory/URLFactoryBuilder.java b/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/urlfactory/URLFactoryBuilder.java index 648d7697803..cd95fb12f42 100644 --- a/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/urlfactory/URLFactoryBuilder.java +++ b/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/urlfactory/URLFactoryBuilder.java @@ -13,6 +13,7 @@ import static com.google.common.base.Strings.isNullOrEmpty; import static org.eclipse.che.api.factory.server.ApiExceptionMapper.toApiException; +import static org.eclipse.che.api.factory.server.scm.exception.ExceptionMessages.getDevfileConnectionErrorMessage; import static org.eclipse.che.api.factory.shared.Constants.CURRENT_VERSION; import static org.eclipse.che.api.workspace.server.devfile.Constants.CURRENT_API_VERSION; import static org.eclipse.che.api.workspace.shared.Constants.WORKSPACE_TOOLING_EDITOR_ATTRIBUTE; @@ -130,7 +131,7 @@ public Optional createFactoryFromDevfile( continue; } catch (DevfileException e) { LOG.debug("Unexpected devfile exception: {}", e.getMessage()); - throw toApiException(e, location); + throw new ApiException(e.getMessage()); } if (isNullOrEmpty(devfileYamlContent)) { return Optional.empty(); @@ -142,7 +143,7 @@ public Optional createFactoryFromDevfile( try { devfileVersionDetector.devfileVersion(parsedDevfile); } catch (DevfileException e) { - throw new ApiException("Failed to fetch devfile"); + throw new ApiException(getDevfileConnectionErrorMessage(devfileLocation)); } return Optional.of( createFactory(parsedDevfile, overrideProperties, fileContentProvider, location)); diff --git a/wsmaster/che-core-api-factory/src/test/java/org/eclipse/che/api/factory/server/urlfactory/URLFactoryBuilderTest.java b/wsmaster/che-core-api-factory/src/test/java/org/eclipse/che/api/factory/server/urlfactory/URLFactoryBuilderTest.java index 324eb6fa791..1c9ea46bb25 100644 --- a/wsmaster/che-core-api-factory/src/test/java/org/eclipse/che/api/factory/server/urlfactory/URLFactoryBuilderTest.java +++ b/wsmaster/che-core-api-factory/src/test/java/org/eclipse/che/api/factory/server/urlfactory/URLFactoryBuilderTest.java @@ -441,7 +441,8 @@ public String location() { } })); - when(fileContentProvider.fetchContent(anyString())).thenThrow(new DevfileException("", cause)); + when(fileContentProvider.fetchContent(anyString())) + .thenThrow(new DevfileException(expectedMessage, cause)); // when try { @@ -457,7 +458,7 @@ public String location() { @Test( expectedExceptions = ApiException.class, - expectedExceptionsMessageRegExp = "Failed to fetch devfile") + expectedExceptionsMessageRegExp = "Could not reach devfile at location") public void shouldThrowErrorOnUnsupportedDevfileContent() throws ApiException, DevfileException, IOException { JsonNode jsonNode = mock(JsonNode.class);