From 0980c7678e42940c1c24eaf68ea55cb45ee37ffa Mon Sep 17 00:00:00 2001 From: ksrinath Date: Tue, 2 Jul 2024 19:42:49 +0530 Subject: [PATCH 1/3] fix(backend/restli): warn instead of error when aspect not found --- .../metadata/filter/RestliLoggingFilter.java | 5 ++- .../resources/entity/AspectResource.java | 5 +-- .../linkedin/metadata/restli/RestliUtil.java | 38 +++++++++++++++++++ 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/filter/RestliLoggingFilter.java b/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/filter/RestliLoggingFilter.java index edd8270e87210e..b90d62584c5b41 100644 --- a/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/filter/RestliLoggingFilter.java +++ b/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/filter/RestliLoggingFilter.java @@ -1,5 +1,6 @@ package com.linkedin.metadata.filter; +import com.linkedin.metadata.restli.RestliUtil; import com.linkedin.restli.common.HttpMethod; import com.linkedin.restli.common.HttpStatus; import com.linkedin.restli.server.filter.Filter; @@ -33,7 +34,9 @@ public CompletableFuture onError( final FilterRequestContext requestContext, final FilterResponseContext responseContext) { logResponse(requestContext, responseContext); - log.error("Rest.li error: ", th); + if (!(th instanceof RestliUtil.NoLogRestLiServiceException)) { + log.error("Rest.li error: ", th); + } return CompletableFuture.completedFuture(null); } diff --git a/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/resources/entity/AspectResource.java b/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/resources/entity/AspectResource.java index 0d9a49d583b57a..49b3c16d0758ef 100644 --- a/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/resources/entity/AspectResource.java +++ b/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/resources/entity/AspectResource.java @@ -153,9 +153,8 @@ public Task get( final VersionedAspect aspect = _entityService.getVersionedAspect(opContext, urn, aspectName, version); if (aspect == null) { - throw RestliUtil.resourceNotFoundException( - String.format( - "Did not find urn: %s aspect: %s version: %s", urn, aspectName, version)); + log.warn("Did not find urn: {} aspect: {} version: {}", urn, aspectName, version); + throw RestliUtil.noLogResourceNotFoundException(); } return new AnyRecord(aspect.data()); }, diff --git a/metadata-utils/src/main/java/com/linkedin/metadata/restli/RestliUtil.java b/metadata-utils/src/main/java/com/linkedin/metadata/restli/RestliUtil.java index 737f79dc1c4417..a633ace952a9cf 100644 --- a/metadata-utils/src/main/java/com/linkedin/metadata/restli/RestliUtil.java +++ b/metadata-utils/src/main/java/com/linkedin/metadata/restli/RestliUtil.java @@ -6,6 +6,7 @@ import com.linkedin.parseq.Task; import com.linkedin.restli.common.HttpStatus; import com.linkedin.restli.server.RestLiServiceException; +import com.linkedin.restli.server.errors.ServiceError; import java.util.Optional; import java.util.function.Supplier; import javax.annotation.Nonnull; @@ -74,11 +75,48 @@ public static Task toTaskFromOptional(@Nonnull Supplier> supp return toTask(() -> supplier.get().orElseThrow(RestliUtil::resourceNotFoundException)); } + public static class NoLogRestLiServiceException extends RestLiServiceException { + + public NoLogRestLiServiceException(HttpStatus status) { + super(status); + } + + public NoLogRestLiServiceException(HttpStatus status, String message) { + super(status, message); + } + + public NoLogRestLiServiceException(HttpStatus status, Throwable cause) { + super(status, cause); + } + + public NoLogRestLiServiceException(HttpStatus status, String message, Throwable cause) { + super(status, message, cause); + } + + public NoLogRestLiServiceException( + HttpStatus status, String message, Throwable cause, boolean writableStackTrace) { + super(status, message, cause, writableStackTrace); + } + + public NoLogRestLiServiceException(ServiceError serviceError) { + super(serviceError); + } + + public NoLogRestLiServiceException(ServiceError serviceError, Throwable cause) { + super(serviceError, cause); + } + } + @Nonnull public static RestLiServiceException resourceNotFoundException() { return resourceNotFoundException(null); } + @Nonnull + public static RestLiServiceException noLogResourceNotFoundException() { + return new NoLogRestLiServiceException(HttpStatus.S_404_NOT_FOUND); + } + @Nonnull public static RestLiServiceException resourceNotFoundException(@Nullable String message) { return new RestLiServiceException(HttpStatus.S_404_NOT_FOUND, message); From ff1b2a42e6c82f6a7573e260ad0b1e2acb5f4bcb Mon Sep 17 00:00:00 2001 From: ksrinath Date: Wed, 3 Jul 2024 08:31:06 +0530 Subject: [PATCH 2/3] fix(backend/restli): refactoring, javadocs --- .../metadata/filter/RestliLoggingFilter.java | 4 +- .../resources/entity/AspectResource.java | 4 +- .../restli/NonExceptionHttpErrorResponse.java | 45 +++++++++++++++++++ .../linkedin/metadata/restli/RestliUtil.java | 37 +-------------- 4 files changed, 50 insertions(+), 40 deletions(-) create mode 100644 metadata-utils/src/main/java/com/linkedin/metadata/restli/NonExceptionHttpErrorResponse.java diff --git a/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/filter/RestliLoggingFilter.java b/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/filter/RestliLoggingFilter.java index b90d62584c5b41..eee64b3f24bb9a 100644 --- a/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/filter/RestliLoggingFilter.java +++ b/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/filter/RestliLoggingFilter.java @@ -1,6 +1,6 @@ package com.linkedin.metadata.filter; -import com.linkedin.metadata.restli.RestliUtil; +import com.linkedin.metadata.restli.NonExceptionHttpErrorResponse; import com.linkedin.restli.common.HttpMethod; import com.linkedin.restli.common.HttpStatus; import com.linkedin.restli.server.filter.Filter; @@ -34,7 +34,7 @@ public CompletableFuture onError( final FilterRequestContext requestContext, final FilterResponseContext responseContext) { logResponse(requestContext, responseContext); - if (!(th instanceof RestliUtil.NoLogRestLiServiceException)) { + if (!(th instanceof NonExceptionHttpErrorResponse)) { log.error("Rest.li error: ", th); } return CompletableFuture.completedFuture(null); diff --git a/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/resources/entity/AspectResource.java b/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/resources/entity/AspectResource.java index 49b3c16d0758ef..cbca464d569a83 100644 --- a/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/resources/entity/AspectResource.java +++ b/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/resources/entity/AspectResource.java @@ -13,7 +13,6 @@ import com.codahale.metrics.MetricRegistry; import com.datahub.authentication.Authentication; import com.datahub.authentication.AuthenticationContext; -import com.datahub.authorization.EntitySpec; import com.datahub.plugins.auth.authorization.Authorizer; import com.google.common.annotations.VisibleForTesting; import com.linkedin.aspect.GetTimeseriesAspectValuesResponse; @@ -21,7 +20,6 @@ import com.linkedin.common.urn.Urn; import com.linkedin.metadata.aspect.EnvelopedAspectArray; import com.linkedin.metadata.aspect.VersionedAspect; -import com.linkedin.metadata.authorization.Disjunctive; import com.linkedin.metadata.authorization.PoliciesConfig; import com.linkedin.metadata.entity.EntityService; import com.linkedin.metadata.entity.IngestResult; @@ -154,7 +152,7 @@ public Task get( _entityService.getVersionedAspect(opContext, urn, aspectName, version); if (aspect == null) { log.warn("Did not find urn: {} aspect: {} version: {}", urn, aspectName, version); - throw RestliUtil.noLogResourceNotFoundException(); + throw RestliUtil.nonExceptionResourceNotFound(); } return new AnyRecord(aspect.data()); }, diff --git a/metadata-utils/src/main/java/com/linkedin/metadata/restli/NonExceptionHttpErrorResponse.java b/metadata-utils/src/main/java/com/linkedin/metadata/restli/NonExceptionHttpErrorResponse.java new file mode 100644 index 00000000000000..9a319cdddac479 --- /dev/null +++ b/metadata-utils/src/main/java/com/linkedin/metadata/restli/NonExceptionHttpErrorResponse.java @@ -0,0 +1,45 @@ +package com.linkedin.metadata.restli; + +import com.linkedin.restli.common.HttpStatus; +import com.linkedin.restli.server.RestLiServiceException; +import com.linkedin.restli.server.errors.ServiceError; + +/** + * Captures an error response (e.g. 404-not-found) that is not to be regarded as an + * exception within the server.
+ *
+ * Restli apparently requires http-error-responses to be represented by {@link + * RestLiServiceException}; thus this class is used to specify an error response that isn't + * really an exception (in the context of the server). + */ +public class NonExceptionHttpErrorResponse extends RestLiServiceException { + + public NonExceptionHttpErrorResponse(HttpStatus status) { + super(status); + } + + public NonExceptionHttpErrorResponse(HttpStatus status, String message) { + super(status, message); + } + + public NonExceptionHttpErrorResponse(HttpStatus status, Throwable cause) { + super(status, cause); + } + + public NonExceptionHttpErrorResponse(HttpStatus status, String message, Throwable cause) { + super(status, message, cause); + } + + public NonExceptionHttpErrorResponse( + HttpStatus status, String message, Throwable cause, boolean writableStackTrace) { + super(status, message, cause, writableStackTrace); + } + + public NonExceptionHttpErrorResponse(ServiceError serviceError) { + super(serviceError); + } + + public NonExceptionHttpErrorResponse(ServiceError serviceError, Throwable cause) { + super(serviceError, cause); + } +} diff --git a/metadata-utils/src/main/java/com/linkedin/metadata/restli/RestliUtil.java b/metadata-utils/src/main/java/com/linkedin/metadata/restli/RestliUtil.java index a633ace952a9cf..c9b1d5a8a82de5 100644 --- a/metadata-utils/src/main/java/com/linkedin/metadata/restli/RestliUtil.java +++ b/metadata-utils/src/main/java/com/linkedin/metadata/restli/RestliUtil.java @@ -6,7 +6,6 @@ import com.linkedin.parseq.Task; import com.linkedin.restli.common.HttpStatus; import com.linkedin.restli.server.RestLiServiceException; -import com.linkedin.restli.server.errors.ServiceError; import java.util.Optional; import java.util.function.Supplier; import javax.annotation.Nonnull; @@ -75,46 +74,14 @@ public static Task toTaskFromOptional(@Nonnull Supplier> supp return toTask(() -> supplier.get().orElseThrow(RestliUtil::resourceNotFoundException)); } - public static class NoLogRestLiServiceException extends RestLiServiceException { - - public NoLogRestLiServiceException(HttpStatus status) { - super(status); - } - - public NoLogRestLiServiceException(HttpStatus status, String message) { - super(status, message); - } - - public NoLogRestLiServiceException(HttpStatus status, Throwable cause) { - super(status, cause); - } - - public NoLogRestLiServiceException(HttpStatus status, String message, Throwable cause) { - super(status, message, cause); - } - - public NoLogRestLiServiceException( - HttpStatus status, String message, Throwable cause, boolean writableStackTrace) { - super(status, message, cause, writableStackTrace); - } - - public NoLogRestLiServiceException(ServiceError serviceError) { - super(serviceError); - } - - public NoLogRestLiServiceException(ServiceError serviceError, Throwable cause) { - super(serviceError, cause); - } - } - @Nonnull public static RestLiServiceException resourceNotFoundException() { return resourceNotFoundException(null); } @Nonnull - public static RestLiServiceException noLogResourceNotFoundException() { - return new NoLogRestLiServiceException(HttpStatus.S_404_NOT_FOUND); + public static RestLiServiceException nonExceptionResourceNotFound() { + return new NonExceptionHttpErrorResponse(HttpStatus.S_404_NOT_FOUND); } @Nonnull From 6c42e3f547f71ad99a2cc25e25ce0e981674cc16 Mon Sep 17 00:00:00 2001 From: ksrinath Date: Wed, 3 Jul 2024 10:18:49 +0530 Subject: [PATCH 3/3] fix(restli): javadocs --- .../metadata/restli/NonExceptionHttpErrorResponse.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/metadata-utils/src/main/java/com/linkedin/metadata/restli/NonExceptionHttpErrorResponse.java b/metadata-utils/src/main/java/com/linkedin/metadata/restli/NonExceptionHttpErrorResponse.java index 9a319cdddac479..c99a3622d15207 100644 --- a/metadata-utils/src/main/java/com/linkedin/metadata/restli/NonExceptionHttpErrorResponse.java +++ b/metadata-utils/src/main/java/com/linkedin/metadata/restli/NonExceptionHttpErrorResponse.java @@ -9,8 +9,10 @@ * exception within the server.
*
* Restli apparently requires http-error-responses to be represented by {@link - * RestLiServiceException}; thus this class is used to specify an error response that isn't - * really an exception (in the context of the server). + * RestLiServiceException}; thus, we need this class to specify an error response that isn't + * really an exception (in the context of the server).
+ * To highlight the unusual purpose of this exception, the name of this class is also deliberately + * unconventional (the class-name doesn't end with "Exception"). */ public class NonExceptionHttpErrorResponse extends RestLiServiceException {