diff --git a/cluster/configs/src/main/java/com/linkedin/openhouse/cluster/configs/ClusterProperties.java b/cluster/configs/src/main/java/com/linkedin/openhouse/cluster/configs/ClusterProperties.java index 9d0c81876..fe2295df3 100644 --- a/cluster/configs/src/main/java/com/linkedin/openhouse/cluster/configs/ClusterProperties.java +++ b/cluster/configs/src/main/java/com/linkedin/openhouse/cluster/configs/ClusterProperties.java @@ -96,4 +96,12 @@ public class ClusterProperties { // string @Value("${cluster.tables.allowed-client-name-values:}") private List allowedClientNameValues; + + /** + * View SQL dialects this deployment accepts on the /v1 views API. A representation, and the + * source dialect naming one, is rejected unless its dialect is listed here. Defaults to Spark + * only; supporting another engine is a configuration change rather than a code change. + */ + @Value("${cluster.tables.views.supported-dialects:spark}") + private List viewsSupportedDialects; } diff --git a/services/common/src/main/java/com/linkedin/openhouse/common/api/validator/ApiValidatorUtil.java b/services/common/src/main/java/com/linkedin/openhouse/common/api/validator/ApiValidatorUtil.java index 63c058f77..682708d0a 100644 --- a/services/common/src/main/java/com/linkedin/openhouse/common/api/validator/ApiValidatorUtil.java +++ b/services/common/src/main/java/com/linkedin/openhouse/common/api/validator/ApiValidatorUtil.java @@ -1,7 +1,12 @@ package com.linkedin.openhouse.common.api.validator; +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.ALPHA_NUM_UNDERSCORE_ERROR_MSG; +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.ALPHA_NUM_UNDERSCORE_REGEX; + import java.util.List; import javax.validation.ConstraintViolation; +import javax.validation.Validator; +import org.apache.commons.lang3.StringUtils; public final class ApiValidatorUtil { /** @@ -45,4 +50,38 @@ public static void validatePageable( "sortBy : provided %s, does not support multiple sort fields or directions", sortBy)); } } + + /** + * Common method to run bean validation on a request object and append one formatted message per + * violation to the running list of validation failures. + * + * @param validator + * @param object + * @param validationFailures + */ + public static void collectViolations( + Validator validator, T object, List validationFailures) { + for (ConstraintViolation violation : validator.validate(object)) { + validationFailures.add(String.format("%s : %s", getField(violation), violation.getMessage())); + } + } + + /** + * Common method to validate that an identifier is present and contains only the characters + * allowed by {@link ValidatorConstants#ALPHA_NUM_UNDERSCORE_REGEX}. At most one failure is + * reported: an absent identifier is not additionally reported as malformed. + * + * @param fieldName + * @param value + * @param validationFailures + */ + public static void validateIdentifier( + String fieldName, String value, List validationFailures) { + if (StringUtils.isEmpty(value)) { + validationFailures.add(String.format("%s : Cannot be empty", fieldName)); + } else if (!value.matches(ALPHA_NUM_UNDERSCORE_REGEX)) { + validationFailures.add( + String.format("%s : provided %s, %s", fieldName, value, ALPHA_NUM_UNDERSCORE_ERROR_MSG)); + } + } } diff --git a/services/common/src/main/java/com/linkedin/openhouse/common/api/validator/ValidatorConstants.java b/services/common/src/main/java/com/linkedin/openhouse/common/api/validator/ValidatorConstants.java index 3f11694b4..18120c63c 100644 --- a/services/common/src/main/java/com/linkedin/openhouse/common/api/validator/ValidatorConstants.java +++ b/services/common/src/main/java/com/linkedin/openhouse/common/api/validator/ValidatorConstants.java @@ -19,4 +19,24 @@ private ValidatorConstants() {} "Only alphanumerics, hyphen and underscore supported"; public static final int MAX_ALLOWED_CLUSTERING_COLUMNS = 4; public static final String INITIAL_TABLE_VERSION = "INITIAL_VERSION"; + + /** The only view representation type accepted by the /v1 views API. */ + public static final String SQL_VIEW_REPRESENTATION_TYPE = "sql"; + + /** + * Maximum length of a view or database identifier on the /v1 views API. Mirrors the + * {@code @Size(max = 128)} bean constraint on the request body identifiers so a path identifier + * cannot bypass the limit the body enforces. + */ + public static final int MAX_VIEW_IDENTIFIER_LENGTH = 128; + + /** + * Maximum size of a single view representation's SQL text, in UTF-8 bytes. Counted in bytes + * rather than characters because the limit protects storage and transport, and a {@code @Size} + * bean constraint would instead count UTF-16 characters and let a multibyte payload through. + */ + public static final int MAX_VIEW_SQL_BYTES = 256 * 1024; + + /** Maximum size of a view schema document, in UTF-8 bytes. See {@link #MAX_VIEW_SQL_BYTES}. */ + public static final int MAX_VIEW_SCHEMA_BYTES = 512 * 1024; } diff --git a/services/common/src/main/java/com/linkedin/openhouse/common/audit/ServiceAuditAspect.java b/services/common/src/main/java/com/linkedin/openhouse/common/audit/ServiceAuditAspect.java index e5a7c11ec..53dce6f6d 100644 --- a/services/common/src/main/java/com/linkedin/openhouse/common/audit/ServiceAuditAspect.java +++ b/services/common/src/main/java/com/linkedin/openhouse/common/audit/ServiceAuditAspect.java @@ -13,6 +13,8 @@ import com.linkedin.openhouse.common.metrics.MetricsConstant; import java.nio.charset.StandardCharsets; import java.time.Instant; +import java.util.Collections; +import java.util.List; import javax.servlet.http.HttpServletRequest; import lombok.extern.slf4j.Slf4j; import org.aspectj.lang.ProceedingJoinPoint; @@ -42,6 +44,13 @@ public class ServiceAuditAspect { @Autowired private AuditHandler serviceAuditHandler; + /** + * Redactors contributed by the running service, if any. Left empty when the service registers + * none, in which case the payload is audited exactly as it was sent. + */ + @Autowired(required = false) + private List payloadRedactors = Collections.emptyList(); + private static final MetricsReporter METRICS_REPORTER = MetricsReporter.of(MetricsConstant.SERVICE_AUDIT); @@ -140,9 +149,12 @@ private ServiceAuditEvent buildServiceAuditEvent( if (wrapper != null) { String requestPayloadStr = new String(wrapper.getContentAsByteArray(), StandardCharsets.UTF_8); - requestPayload = JsonParser.parseString(requestPayloadStr); + requestPayload = redactSensitiveValues(request, JsonParser.parseString(requestPayloadStr)); } } catch (Exception e) { + // Fail closed. A payload that could not be parsed or could not be redacted is dropped rather + // than audited raw, so a broken redactor cannot leak the values it was meant to remove. + requestPayload = null; log.error("Exception during parsing request payload:\n", e); METRICS_REPORTER.count(MetricsConstant.FAILED_PARSING_REQUEST_PAYLOAD); } @@ -182,6 +194,25 @@ private ServiceAuditEvent buildServiceAuditEvent( .build(); } + /** + * Apply every registered redactor that claims this request. With no redactor registered — the + * case for every service that has not contributed one — the parsed payload is returned unchanged, + * so existing audit payloads are byte-identical. + */ + private JsonElement redactSensitiveValues( + HttpServletRequest request, JsonElement requestPayload) { + if (requestPayload == null || payloadRedactors == null) { + return requestPayload; + } + JsonElement redacted = requestPayload; + for (ServiceAuditPayloadRedactor redactor : payloadRedactors) { + if (redactor.appliesTo(request)) { + redacted = redactor.redact(redacted); + } + } + return redacted; + } + private ServiceName getServiceNameFromRequestURI(String uri) { if (uri.startsWith("/jobs")) { return ServiceName.JOBS_SERVICE; diff --git a/services/common/src/main/java/com/linkedin/openhouse/common/audit/ServiceAuditPayloadRedactor.java b/services/common/src/main/java/com/linkedin/openhouse/common/audit/ServiceAuditPayloadRedactor.java new file mode 100644 index 000000000..93d9c1d3e --- /dev/null +++ b/services/common/src/main/java/com/linkedin/openhouse/common/audit/ServiceAuditPayloadRedactor.java @@ -0,0 +1,37 @@ +package com.linkedin.openhouse.common.audit; + +import com.google.gson.JsonElement; +import javax.servlet.http.HttpServletRequest; + +/** + * Extension point that removes sensitive values from a request payload before {@link + * ServiceAuditAspect} writes it into a {@link + * com.linkedin.openhouse.common.audit.model.ServiceAuditEvent}. + * + *

The aspect audits the complete cached request body of every controller call, so a route whose + * body carries content that must not be retained has to opt out here. Only the mechanism lives in + * {@code services/common}: each service contributes its own beans and names its own fields, and a + * service that contributes none has its payload audited exactly as before. + * + *

Implementations are handed the parsed payload and must not mutate it. Returning a modified + * copy keeps a redactor from observing another redactor's half-rewritten tree, and keeps the + * decision to drop the payload entirely with the aspect. + */ +public interface ServiceAuditPayloadRedactor { + + /** + * Marker written in place of a redacted value. Implementations replace the value and keep the + * key, so an auditor can still see that the field was present in the request. + */ + String REDACTED_VALUE = "[REDACTED]"; + + /** @return whether this redactor owns {@code request}. */ + boolean appliesTo(HttpServletRequest request); + + /** + * @param requestPayload the parsed request body, which may be any {@link JsonElement} the caller + * sent, including a non-object or {@link com.google.gson.JsonNull}. + * @return a redacted copy, or the argument unchanged when there is nothing to redact. + */ + JsonElement redact(JsonElement requestPayload); +} diff --git a/services/common/src/main/java/com/linkedin/openhouse/common/exception/CodedApiException.java b/services/common/src/main/java/com/linkedin/openhouse/common/exception/CodedApiException.java new file mode 100644 index 000000000..dcaeaae0f --- /dev/null +++ b/services/common/src/main/java/com/linkedin/openhouse/common/exception/CodedApiException.java @@ -0,0 +1,29 @@ +package com.linkedin.openhouse.common.exception; + +import org.springframework.http.HttpStatus; + +/** + * Base class for exceptions that already know the HTTP status they should map to. + * + *

This is deliberately a bare seam. It carries no error-code vocabulary of its own: a subclass + * in a downstream service owns whatever taxonomy it needs and reduces that taxonomy to an {@link + * HttpStatus} here. That keeps {@code services/common} free of any service-specific enum while + * still letting {@link com.linkedin.openhouse.common.exception.handler.OpenHouseExceptionHandler} + * map the exception to a response with a single handler. + * + *

The status is the only thing that reaches the wire. The error body shape is unchanged, so no + * subclass taxonomy is serialized. + */ +public abstract class CodedApiException extends RuntimeException { + + protected CodedApiException(String message) { + super(message); + } + + protected CodedApiException(String message, Throwable cause) { + super(message, cause); + } + + /** @return the HTTP status this failure maps to. */ + public abstract HttpStatus getHttpStatus(); +} diff --git a/services/common/src/main/java/com/linkedin/openhouse/common/exception/handler/OpenHouseExceptionHandler.java b/services/common/src/main/java/com/linkedin/openhouse/common/exception/handler/OpenHouseExceptionHandler.java index b03bd6491..b5e19f11e 100644 --- a/services/common/src/main/java/com/linkedin/openhouse/common/exception/handler/OpenHouseExceptionHandler.java +++ b/services/common/src/main/java/com/linkedin/openhouse/common/exception/handler/OpenHouseExceptionHandler.java @@ -3,6 +3,7 @@ import com.fasterxml.jackson.databind.exc.InvalidFormatException; import com.linkedin.openhouse.common.api.spec.ErrorResponseBody; import com.linkedin.openhouse.common.exception.AlreadyExistsException; +import com.linkedin.openhouse.common.exception.CodedApiException; import com.linkedin.openhouse.common.exception.EntityConcurrentModificationException; import com.linkedin.openhouse.common.exception.InvalidSchemaEvolutionException; import com.linkedin.openhouse.common.exception.InvalidTableMetadataException; @@ -100,6 +101,30 @@ protected ResponseEntity handleToggleException( return buildResponseEntity(errorResponseBody); } + /** + * Generic mapping for any exception that already knows its own HTTP status. The status comes from + * {@link CodedApiException#getHttpStatus()}; the body is the existing unchanged {@link + * ErrorResponseBody}, so no service-specific error taxonomy reaches the wire. + * + *

Spring selects the most specific handler, so declaring this does not change the mapping of + * any exception already handled above. + */ + @Hidden + @ExceptionHandler(CodedApiException.class) + protected ResponseEntity handleCodedApiException( + CodedApiException codedApiException) { + HttpStatus httpStatus = codedApiException.getHttpStatus(); + ErrorResponseBody errorResponseBody = + ErrorResponseBody.builder() + .status(httpStatus) + .error(httpStatus.getReasonPhrase()) + .message(codedApiException.getMessage()) + .stacktrace(getAbbreviatedStackTrace(codedApiException)) + .cause(getExceptionCause(codedApiException)) + .build(); + return buildResponseEntity(errorResponseBody); + } + /** * To customize behavior of handling {@link NoHandlerFoundException} one cannot rely on {@link * ExceptionHandler} as that causes ambiguity with {@link ResponseEntityExceptionHandler} and diff --git a/services/common/src/test/java/com/linkedin/openhouse/common/api/validator/ApiValidatorUtilTest.java b/services/common/src/test/java/com/linkedin/openhouse/common/api/validator/ApiValidatorUtilTest.java new file mode 100644 index 000000000..1f97377b1 --- /dev/null +++ b/services/common/src/test/java/com/linkedin/openhouse/common/api/validator/ApiValidatorUtilTest.java @@ -0,0 +1,125 @@ +package com.linkedin.openhouse.common.api.validator; + +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.ALPHA_NUM_UNDERSCORE_ERROR_MSG; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.Iterator; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; +import javax.validation.ConstraintViolation; +import javax.validation.Path; +import javax.validation.Validator; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; + +public class ApiValidatorUtilTest { + + /** Stand-in root bean, only used for the simple name that {@code getField} derives. */ + private static final class SampleRequestBody {} + + @Test + public void testValidateIdentifierAcceptsLegalIdentifier() { + List validationFailures = new ArrayList<>(); + ApiValidatorUtil.validateIdentifier("databaseId", "db_1", validationFailures); + Assertions.assertEquals(Collections.emptyList(), validationFailures); + } + + @Test + public void testValidateIdentifierRejectsEmpty() { + List validationFailures = new ArrayList<>(); + ApiValidatorUtil.validateIdentifier("databaseId", "", validationFailures); + Assertions.assertEquals( + Collections.singletonList("databaseId : Cannot be empty"), validationFailures); + } + + @Test + public void testValidateIdentifierRejectsNull() { + List validationFailures = new ArrayList<>(); + ApiValidatorUtil.validateIdentifier("tableId", null, validationFailures); + Assertions.assertEquals( + Collections.singletonList("tableId : Cannot be empty"), validationFailures); + } + + @Test + public void testValidateIdentifierRejectsIllegalCharacters() { + List validationFailures = new ArrayList<>(); + ApiValidatorUtil.validateIdentifier("tableId", "t#1", validationFailures); + Assertions.assertEquals( + Collections.singletonList("tableId : provided t#1, " + ALPHA_NUM_UNDERSCORE_ERROR_MSG), + validationFailures); + } + + @Test + public void testValidateIdentifierAppendsWithoutClearing() { + List validationFailures = new ArrayList<>(); + validationFailures.add("pre-existing failure"); + ApiValidatorUtil.validateIdentifier("databaseId", "d$b", validationFailures); + Assertions.assertEquals( + Arrays.asList( + "pre-existing failure", "databaseId : provided d$b, " + ALPHA_NUM_UNDERSCORE_ERROR_MSG), + validationFailures); + } + + @Test + public void testCollectViolationsFormatsEachViolation() { + SampleRequestBody requestBody = new SampleRequestBody(); + Set> violations = new LinkedHashSet<>(); + violations.add(violation(requestBody, "tableId", "must not be empty")); + violations.add(violation(requestBody, "", "must be a consistent request")); + + Validator validator = Mockito.mock(Validator.class); + Mockito.when(validator.validate(requestBody)).thenReturn(violations); + + List validationFailures = new ArrayList<>(); + validationFailures.add("pre-existing failure"); + ApiValidatorUtil.collectViolations(validator, requestBody, validationFailures); + + Assertions.assertEquals( + Arrays.asList( + "pre-existing failure", + "SampleRequestBody.tableId : must not be empty", + "SampleRequestBody : must be a consistent request"), + validationFailures); + } + + @Test + public void testCollectViolationsAddsNothingWhenBeanIsValid() { + SampleRequestBody requestBody = new SampleRequestBody(); + Validator validator = Mockito.mock(Validator.class); + Mockito.when(validator.validate(requestBody)).thenReturn(Collections.emptySet()); + + List validationFailures = new ArrayList<>(); + ApiValidatorUtil.collectViolations(validator, requestBody, validationFailures); + + Assertions.assertEquals(Collections.emptyList(), validationFailures); + } + + @SuppressWarnings("unchecked") + private static ConstraintViolation violation( + SampleRequestBody rootBean, String propertyPath, String message) { + ConstraintViolation violation = Mockito.mock(ConstraintViolation.class); + Mockito.when(violation.getRootBean()).thenReturn(rootBean); + Mockito.when(violation.getPropertyPath()).thenReturn(path(propertyPath)); + Mockito.when(violation.getMessage()).thenReturn(message); + return violation; + } + + /** {@code getField} only reads the string form of a path, so an empty node list is enough. */ + private static Path path(String value) { + return new Path() { + @Override + public Iterator iterator() { + return Collections.emptyList().iterator(); + } + + @Override + public String toString() { + return value; + } + }; + } +} diff --git a/services/common/src/test/java/com/linkedin/openhouse/common/exception/handler/CodedApiExceptionHandlerTest.java b/services/common/src/test/java/com/linkedin/openhouse/common/exception/handler/CodedApiExceptionHandlerTest.java new file mode 100644 index 000000000..02d3fba58 --- /dev/null +++ b/services/common/src/test/java/com/linkedin/openhouse/common/exception/handler/CodedApiExceptionHandlerTest.java @@ -0,0 +1,130 @@ +package com.linkedin.openhouse.common.exception.handler; + +import com.linkedin.openhouse.common.api.spec.ErrorResponseBody; +import com.linkedin.openhouse.common.exception.CodedApiException; +import java.lang.reflect.Field; +import java.lang.reflect.Modifier; +import java.util.Arrays; +import java.util.LinkedHashSet; +import java.util.Set; +import java.util.stream.Collectors; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; + +/** + * Direct coverage of the generic {@link CodedApiException} mapping added to {@link + * OpenHouseExceptionHandler}. + * + *

Deliberately free of any downstream vocabulary: {@code services/common} has no dependency on + * {@code services/tables}, so this test builds anonymous coded exceptions rather than referencing a + * view error code. That the view taxonomy reduces to these statuses is a tables-side concern. + * + *

This test lives in the handler's own package so the {@code protected} handler method is + * reachable without reflection. + */ +public class CodedApiExceptionHandlerTest { + + /** + * Every status a downstream taxonomy currently reduces to, plus two it does not, proving the + * handler forwards whatever the exception reports rather than mapping a known set. + */ + private enum RepresentativeStatus { + BAD_REQUEST(HttpStatus.BAD_REQUEST), + NOT_FOUND(HttpStatus.NOT_FOUND), + CONFLICT(HttpStatus.CONFLICT), + UNPROCESSABLE_ENTITY(HttpStatus.UNPROCESSABLE_ENTITY), + SERVICE_UNAVAILABLE(HttpStatus.SERVICE_UNAVAILABLE), + FORBIDDEN(HttpStatus.FORBIDDEN), + GATEWAY_TIMEOUT(HttpStatus.GATEWAY_TIMEOUT); + + private final HttpStatus httpStatus; + + RepresentativeStatus(HttpStatus httpStatus) { + this.httpStatus = httpStatus; + } + } + + private static final String FIXED_MESSAGE = "a fixed redacted failure message"; + + private final OpenHouseExceptionHandler handler = new OpenHouseExceptionHandler(); + + private static CodedApiException codedException(HttpStatus httpStatus, Throwable cause) { + return new CodedApiException(FIXED_MESSAGE, cause) { + @Override + public HttpStatus getHttpStatus() { + return httpStatus; + } + }; + } + + @ParameterizedTest + @EnumSource(RepresentativeStatus.class) + public void codedExceptionKeepsItsOwnStatusAndMessage(RepresentativeStatus representativeStatus) { + HttpStatus expected = representativeStatus.httpStatus; + + ResponseEntity response = + handler.handleCodedApiException(codedException(expected, null)); + + Assertions.assertEquals( + expected, + response.getStatusCode(), + "The response status must come from the exception, not from a fixed mapping."); + + ErrorResponseBody body = response.getBody(); + Assertions.assertNotNull(body); + Assertions.assertEquals(expected, body.getStatus()); + Assertions.assertEquals(expected.getReasonPhrase(), body.getError()); + Assertions.assertEquals( + FIXED_MESSAGE, + body.getMessage(), + "The message is copied verbatim: the handler must not decorate it, because callers rely on" + + " it staying redacted."); + Assertions.assertNotNull(body.getStacktrace()); + } + + @Test + public void codedExceptionCarriesItsCauseIntoTheBody() { + ResponseEntity withCause = + handler.handleCodedApiException( + codedException( + HttpStatus.SERVICE_UNAVAILABLE, new IllegalStateException("downstream"))); + Assertions.assertNotNull(withCause.getBody()); + Assertions.assertTrue( + withCause.getBody().getCause().contains("downstream"), + "A wrapped root cause must survive into the error body."); + + ResponseEntity withoutCause = + handler.handleCodedApiException(codedException(HttpStatus.NOT_FOUND, null)); + Assertions.assertNotNull(withoutCause.getBody()); + Assertions.assertNotNull( + withoutCause.getBody().getCause(), + "A cause-less exception still populates the field rather than omitting it."); + } + + /** + * The generic handler exists precisely so that a downstream taxonomy does not have to widen the + * shared error body. If a new field ever appears here, that decision has been reversed and needs + * its own review. + */ + @Test + public void errorResponseBodyShapeIsUnchanged() { + Set expectedFields = + new LinkedHashSet<>(Arrays.asList("status", "error", "message", "stacktrace", "cause")); + + Set actualFields = + Arrays.stream(ErrorResponseBody.class.getDeclaredFields()) + .filter(field -> !field.isSynthetic()) + .filter(field -> !Modifier.isStatic(field.getModifiers())) + .map(Field::getName) + .collect(Collectors.toCollection(LinkedHashSet::new)); + + Assertions.assertEquals( + expectedFields, + actualFields, + "No error code or other new field may be added to the shared error body."); + } +} diff --git a/services/housetables/src/main/java/com/linkedin/openhouse/housetables/api/validator/impl/OpenHouseJobTablesHtsApiValidator.java b/services/housetables/src/main/java/com/linkedin/openhouse/housetables/api/validator/impl/OpenHouseJobTablesHtsApiValidator.java index 5770711b0..b9aef8689 100644 --- a/services/housetables/src/main/java/com/linkedin/openhouse/housetables/api/validator/impl/OpenHouseJobTablesHtsApiValidator.java +++ b/services/housetables/src/main/java/com/linkedin/openhouse/housetables/api/validator/impl/OpenHouseJobTablesHtsApiValidator.java @@ -9,7 +9,6 @@ import com.linkedin.openhouse.housetables.api.validator.HouseTablesApiValidator; import java.util.ArrayList; import java.util.List; -import javax.validation.ConstraintViolation; import javax.validation.Validator; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Component; @@ -66,10 +65,7 @@ public void validateGetEntities(Job entity, int page, int size, String sortBy) { public void validatePutEntity(Job entity) { List validationFailures = new ArrayList<>(); - for (ConstraintViolation violation : validator.validate(entity)) { - validationFailures.add( - String.format("%s : %s", ApiValidatorUtil.getField(violation), violation.getMessage())); - } + ApiValidatorUtil.collectViolations(validator, entity, validationFailures); if (!validationFailures.isEmpty()) { throw new RequestValidationFailureException(validationFailures); diff --git a/services/housetables/src/main/java/com/linkedin/openhouse/housetables/api/validator/impl/OpenHouseUserTableHtsApiValidator.java b/services/housetables/src/main/java/com/linkedin/openhouse/housetables/api/validator/impl/OpenHouseUserTableHtsApiValidator.java index 087d928fd..c8c65912c 100644 --- a/services/housetables/src/main/java/com/linkedin/openhouse/housetables/api/validator/impl/OpenHouseUserTableHtsApiValidator.java +++ b/services/housetables/src/main/java/com/linkedin/openhouse/housetables/api/validator/impl/OpenHouseUserTableHtsApiValidator.java @@ -9,7 +9,6 @@ import com.linkedin.openhouse.housetables.api.validator.HouseTablesApiValidator; import java.util.ArrayList; import java.util.List; -import javax.validation.ConstraintViolation; import javax.validation.Validator; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Component; @@ -70,10 +69,7 @@ public void validateGetEntities(UserTable userTable, int page, int size, String public void validatePutEntity(UserTable userTable) { List validationFailures = new ArrayList<>(); - for (ConstraintViolation violation : validator.validate(userTable)) { - validationFailures.add( - String.format("%s : %s", ApiValidatorUtil.getField(violation), violation.getMessage())); - } + ApiValidatorUtil.collectViolations(validator, userTable, validationFailures); if (!validationFailures.isEmpty()) { throw new RequestValidationFailureException(validationFailures); diff --git a/services/jobs/src/main/java/com/linkedin/openhouse/jobs/api/validator/impl/OpenHouseJobsApiValidator.java b/services/jobs/src/main/java/com/linkedin/openhouse/jobs/api/validator/impl/OpenHouseJobsApiValidator.java index e1f677579..d347e728b 100644 --- a/services/jobs/src/main/java/com/linkedin/openhouse/jobs/api/validator/impl/OpenHouseJobsApiValidator.java +++ b/services/jobs/src/main/java/com/linkedin/openhouse/jobs/api/validator/impl/OpenHouseJobsApiValidator.java @@ -9,8 +9,6 @@ import com.linkedin.openhouse.jobs.api.validator.JobsApiValidator; import java.util.ArrayList; import java.util.List; -import java.util.Set; -import javax.validation.ConstraintViolation; import javax.validation.Validator; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Component; @@ -22,12 +20,7 @@ public class OpenHouseJobsApiValidator implements JobsApiValidator { @Override public void validateCreateJob(CreateJobRequestBody createJobRequestBody) { List validationFailures = new ArrayList<>(); - Set> violationSet = - validator.validate(createJobRequestBody); - for (ConstraintViolation violation : violationSet) { - validationFailures.add( - String.format("%s : %s", ApiValidatorUtil.getField(violation), violation.getMessage())); - } + ApiValidatorUtil.collectViolations(validator, createJobRequestBody, validationFailures); if (!createJobRequestBody.getJobName().matches(ALPHA_NUM_UNDERSCORE_REGEX_HYPHEN_ALLOW)) { validationFailures.add( String.format( diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/handler/ViewsApiHandler.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/handler/ViewsApiHandler.java new file mode 100644 index 000000000..7bbceafb1 --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/handler/ViewsApiHandler.java @@ -0,0 +1,73 @@ +package com.linkedin.openhouse.tables.api.handler; + +import com.linkedin.openhouse.common.api.spec.ApiResponse; +import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateViewRequestBody; +import com.linkedin.openhouse.tables.api.spec.v0.response.GetAllViewsResponseBody; +import com.linkedin.openhouse.tables.api.spec.v0.response.GetViewResponseBody; + +/** + * Layer between the /v1 views REST routes and the view service. Implementations hold no business + * logic: they validate, map, delegate, map back and pick a status. + */ +public interface ViewsApiHandler { + + /** + * Read a single view. + * + * @param databaseId database identifier + * @param viewId view identifier + * @param actingPrincipal authenticated user + * @return 200 with the view pointer + */ + ApiResponse getView( + String databaseId, String viewId, String actingPrincipal); + + /** + * List views in a database. + * + * @param databaseId database identifier + * @param page zero-based page index + * @param size page size + * @param sortBy optional single sort field + * @param actingPrincipal authenticated user + * @return 200 with a page of sparse identifier-only view bodies + */ + ApiResponse getAllViews( + String databaseId, int page, int size, String sortBy, String actingPrincipal); + + /** + * Create a view. + * + * @param databaseId database identifier + * @param requestBody the create request + * @param actingPrincipal authenticated user + * @return 201 with the created view pointer + */ + ApiResponse createView( + String databaseId, CreateUpdateViewRequestBody requestBody, String actingPrincipal); + + /** + * Replace a view, creating it when it does not exist. + * + * @param databaseId database identifier + * @param viewId view identifier + * @param requestBody the update request + * @param actingPrincipal authenticated user + * @return 201 when the call created the view, otherwise 200 + */ + ApiResponse updateView( + String databaseId, + String viewId, + CreateUpdateViewRequestBody requestBody, + String actingPrincipal); + + /** + * Delete a view. + * + * @param databaseId database identifier + * @param viewId view identifier + * @param actingPrincipal authenticated user + * @return 204 with no body + */ + ApiResponse deleteView(String databaseId, String viewId, String actingPrincipal); +} diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/handler/impl/OpenHouseViewsApiHandler.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/handler/impl/OpenHouseViewsApiHandler.java new file mode 100644 index 000000000..ee9264be1 --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/handler/impl/OpenHouseViewsApiHandler.java @@ -0,0 +1,94 @@ +package com.linkedin.openhouse.tables.api.handler.impl; + +import com.linkedin.openhouse.cluster.configs.ClusterProperties; +import com.linkedin.openhouse.common.api.spec.ApiResponse; +import com.linkedin.openhouse.tables.api.handler.ViewsApiHandler; +import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateViewRequestBody; +import com.linkedin.openhouse.tables.api.spec.v0.response.GetAllViewsResponseBody; +import com.linkedin.openhouse.tables.api.spec.v0.response.GetViewResponseBody; +import com.linkedin.openhouse.tables.api.validator.ViewsApiValidator; +import com.linkedin.openhouse.tables.dto.mapper.ViewsMapper; +import com.linkedin.openhouse.tables.model.ViewDto; +import com.linkedin.openhouse.tables.services.ViewsService; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.data.util.Pair; +import org.springframework.http.HttpStatus; +import org.springframework.stereotype.Component; + +/** + * Default /v1 views API handler. The flow is strictly validate, map, delegate to the service, map + * back, and pick a status: no business logic, no response serialization and no feature gating live + * here. + */ +@Component +public class OpenHouseViewsApiHandler implements ViewsApiHandler { + + @Autowired private ViewsApiValidator viewsApiValidator; + + @Autowired private ViewsService viewsService; + + @Autowired private ViewsMapper viewsMapper; + + @Autowired private ClusterProperties clusterProperties; + + @Override + public ApiResponse getView( + String databaseId, String viewId, String actingPrincipal) { + viewsApiValidator.validateGetView(databaseId, viewId); + ViewDto viewDto = viewsService.getView(databaseId, viewId, actingPrincipal); + return ApiResponse.builder() + .httpStatus(HttpStatus.OK) + .responseBody(viewsMapper.toGetViewResponseBody(viewDto)) + .build(); + } + + @Override + public ApiResponse getAllViews( + String databaseId, int page, int size, String sortBy, String actingPrincipal) { + viewsApiValidator.validateGetAllViews(databaseId, page, size, sortBy); + return ApiResponse.builder() + .httpStatus(HttpStatus.OK) + .responseBody( + GetAllViewsResponseBody.builder() + .pageResults( + viewsMapper.toGetViewResponseBodyPage( + viewsService.getAllViews(databaseId, page, size, sortBy, actingPrincipal))) + .build()) + .build(); + } + + @Override + public ApiResponse createView( + String databaseId, CreateUpdateViewRequestBody requestBody, String actingPrincipal) { + viewsApiValidator.validateCreateView( + clusterProperties.getClusterName(), databaseId, requestBody); + Pair putResult = viewsService.putView(requestBody, actingPrincipal, true); + return ApiResponse.builder() + .httpStatus(HttpStatus.CREATED) + .responseBody(viewsMapper.toGetViewResponseBody(putResult.getFirst())) + .build(); + } + + @Override + public ApiResponse updateView( + String databaseId, + String viewId, + CreateUpdateViewRequestBody requestBody, + String actingPrincipal) { + viewsApiValidator.validateUpdateView( + clusterProperties.getClusterName(), databaseId, viewId, requestBody); + Pair putResult = viewsService.putView(requestBody, actingPrincipal, false); + HttpStatus httpStatus = putResult.getSecond() ? HttpStatus.CREATED : HttpStatus.OK; + return ApiResponse.builder() + .httpStatus(httpStatus) + .responseBody(viewsMapper.toGetViewResponseBody(putResult.getFirst())) + .build(); + } + + @Override + public ApiResponse deleteView(String databaseId, String viewId, String actingPrincipal) { + viewsApiValidator.validateDeleteView(databaseId, viewId); + viewsService.deleteView(databaseId, viewId, actingPrincipal); + return ApiResponse.builder().httpStatus(HttpStatus.NO_CONTENT).build(); + } +} diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/request/CreateUpdateViewRequestBody.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/request/CreateUpdateViewRequestBody.java new file mode 100644 index 000000000..0ee7b669a --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/request/CreateUpdateViewRequestBody.java @@ -0,0 +1,113 @@ +package com.linkedin.openhouse.tables.api.spec.v0.request; + +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.*; + +import com.fasterxml.jackson.annotation.JsonInclude; +import com.google.gson.Gson; +import com.linkedin.openhouse.tables.api.spec.v0.request.components.ViewRepresentation; +import io.swagger.v3.oas.annotations.media.Schema; +import java.util.List; +import java.util.Map; +import javax.validation.Valid; +import javax.validation.constraints.NotEmpty; +import javax.validation.constraints.Pattern; +import javax.validation.constraints.Size; +import lombok.AccessLevel; +import lombok.AllArgsConstructor; +import lombok.Builder; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.NoArgsConstructor; + +/** + * Request body for POST and PUT on /v1/databases/{databaseId}/views. Nullable fields are omitted + * from the serialized payload rather than emitted as JSON null, so an omitted {@code + * baseViewVersion} on create stays absent on the wire. + */ +@Builder(toBuilder = true) +@EqualsAndHashCode +@Getter +@AllArgsConstructor(access = AccessLevel.PROTECTED) +@NoArgsConstructor(access = AccessLevel.PROTECTED) +@JsonInclude(JsonInclude.Include.NON_NULL) +public class CreateUpdateViewRequestBody { + + @Schema( + description = "Unique Resource identifier for a view within a Database", + example = "my_view") + @NotEmpty(message = "viewId cannot be empty") + @Size(max = 128) + @Pattern(regexp = ALPHA_NUM_UNDERSCORE_REGEX, message = ALPHA_NUM_UNDERSCORE_ERROR_MSG) + private String viewId; + + @Schema( + description = "Unique Resource identifier for the Database containing the View", + example = "my_database") + @NotEmpty(message = "databaseId cannot be empty") + @Size(max = 128) + @Pattern(regexp = ALPHA_NUM_UNDERSCORE_REGEX, message = ALPHA_NUM_UNDERSCORE_ERROR_MSG) + private String databaseId; + + @Schema( + description = "Unique Resource identifier for the Cluster containing the Database", + example = "my_cluster") + @NotEmpty(message = "clusterId cannot be empty") + @Pattern( + regexp = ALPHA_NUM_UNDERSCORE_REGEX_HYPHEN_ALLOW, + message = ALPHA_NUM_UNDERSCORE_ERROR_MSG_HYPHEN_ALLOW) + private String clusterId; + + @Schema( + description = "Schema of the view. OpenHouse views use Iceberg schema specification", + example = + "{\"type\": \"struct\", " + + "\"fields\": [{\"id\": 1,\"required\": true,\"name\": \"id\",\"type\": \"string\"}, " + + "{\"id\": 2,\"required\": true,\"name\": \"name\",\"type\": \"string\"}]}") + @NotEmpty(message = "schema cannot be empty") + private String schema; + + @Schema( + description = "Engine-specific representations of the view definition", + example = "[{\"type\": \"sql\", \"sql\": \"SELECT 1\", \"dialect\": \"spark\"}]") + @NotEmpty(message = "representations cannot be empty") + @Valid + private List representations; + + @Schema(description = "Dialect of the representation the view was authored in", example = "spark") + @NotEmpty(message = "sourceDialect cannot be empty") + private String sourceDialect; + + @Schema( + nullable = true, + description = "Catalog used to resolve unqualified identifiers in the view SQL", + example = "openhouse") + private String defaultCatalog; + + @Schema( + nullable = true, + description = "Namespace used to resolve unqualified identifiers in the view SQL", + example = "[\"my_database\"]") + private List defaultNamespace; + + @Schema(nullable = true, description = "View properties", example = "{\"key\": \"value\"}") + private Map viewProperties; + + /** + * Route-sensitive: absent or {@code INITIAL_VERSION} on create, and the current metadata pointer + * on replace. Intentionally carries no bean constraint because the rule differs per HTTP verb and + * is owned by the verb-aware view validator. + */ + @Schema( + nullable = true, + description = "The version of the view that the current update is based upon") + private String baseViewVersion; + + /** + * Uses default Gson null handling rather than {@code serializeNulls()} so this stays consistent + * with the class-level {@link JsonInclude.Include#NON_NULL}: an omitted nullable field is absent + * from the payload, not present as JSON null. + */ + public String toJson() { + return new Gson().toJson(this); + } +} diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/request/components/ViewRepresentation.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/request/components/ViewRepresentation.java new file mode 100644 index 000000000..fdd451aba --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/request/components/ViewRepresentation.java @@ -0,0 +1,40 @@ +package com.linkedin.openhouse.tables.api.spec.v0.request.components; + +import io.swagger.v3.oas.annotations.media.Schema; +import javax.validation.constraints.NotEmpty; +import lombok.AccessLevel; +import lombok.AllArgsConstructor; +import lombok.Builder; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.NoArgsConstructor; + +/** + * ViewRepresentation is the entity holding a single engine-specific representation of a view in the + * /views API request body. SQL text is carried as opaque text; this class holds no parsing, + * translation or dialect-support logic. Byte-size, representation-type and dialect-support rules + * are owned by the manual view validator. + */ +@Builder(toBuilder = true) +@EqualsAndHashCode +@Getter +@AllArgsConstructor(access = AccessLevel.PROTECTED) +@NoArgsConstructor(access = AccessLevel.PROTECTED) +public class ViewRepresentation { + + @Schema(description = "Type of the view representation", example = "sql") + @NotEmpty(message = "type cannot be empty") + private String type; + + @Schema( + description = + "SQL text of the view representation. This endpoint accepts it as opaque text: it is" + + " stored as sent and is not parsed or rewritten here.", + example = "SELECT id, name FROM my_database.my_table") + @NotEmpty(message = "sql cannot be empty") + private String sql; + + @Schema(description = "SQL dialect the representation is written in", example = "spark") + @NotEmpty(message = "dialect cannot be empty") + private String dialect; +} diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/response/GetAllViewsResponseBody.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/response/GetAllViewsResponseBody.java new file mode 100644 index 000000000..f8b608309 --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/response/GetAllViewsResponseBody.java @@ -0,0 +1,25 @@ +package com.linkedin.openhouse.tables.api.spec.v0.response; + +import com.fasterxml.jackson.annotation.JsonProperty; +import com.google.gson.Gson; +import io.swagger.v3.oas.annotations.media.Schema; +import lombok.Builder; +import lombok.Value; +import org.springframework.data.domain.Page; + +/** + * List contract for views. Paginated from the first release, so there is no unpaginated legacy + * {@code results} field to deprecate later. + */ +@Builder +@Value +public class GetAllViewsResponseBody { + + @Schema(description = "Page of View objects in a database", example = "") + @JsonProperty(access = JsonProperty.Access.READ_ONLY) + private Page pageResults; + + public String toJson() { + return new Gson().toJson(this); + } +} diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/response/GetViewResponseBody.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/response/GetViewResponseBody.java new file mode 100644 index 000000000..910d5b910 --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/spec/v0/response/GetViewResponseBody.java @@ -0,0 +1,62 @@ +package com.linkedin.openhouse.tables.api.spec.v0.response; + +import com.fasterxml.jackson.annotation.JsonProperty; +import com.google.gson.Gson; +import io.swagger.v3.oas.annotations.media.Schema; +import lombok.Builder; +import lombok.Value; + +/** + * Read contract for a view. Pointer-only today: this response omits the definition fields. The SQL, + * schema, representations, version history, UUID, properties and resolution context live in the + * view metadata file and are not returned by the item or list response in this milestone. + */ +@Builder(toBuilder = true) +@Value +public class GetViewResponseBody { + + @Schema( + description = "Unique Resource identifier for a view within a Database", + example = "my_view") + @JsonProperty(access = JsonProperty.Access.READ_ONLY) + private String viewId; + + @Schema( + description = "Unique Resource identifier for the Database containing the View", + example = "my_database") + @JsonProperty(access = JsonProperty.Access.READ_ONLY) + private String databaseId; + + @Schema( + description = "Unique Resource identifier for the Cluster containing the Database", + example = "my_cluster") + @JsonProperty(access = JsonProperty.Access.READ_ONLY) + private String clusterId; + + @Schema( + description = "Fully Qualified Resource URI for the view", + example = "my_cluster.my_database.my_view") + @JsonProperty(access = JsonProperty.Access.READ_ONLY) + private String viewUri; + + @Schema( + description = "Location of the view metadata in File System / Blob Store", + example = + "://////metadata/.metadata.json") + @JsonProperty(access = JsonProperty.Access.READ_ONLY) + private String metadataLocation; + + @Schema(description = "Current Version of the View.", example = "") + @JsonProperty(access = JsonProperty.Access.READ_ONLY) + private String viewVersion; + + @Schema( + description = "View creation epoch time measured in UTC in milliseconds of a view.", + example = "1651002318265") + @JsonProperty(access = JsonProperty.Access.READ_ONLY) + private long creationTime; + + public String toJson() { + return new Gson().toJson(this); + } +} diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/ViewsApiValidator.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/ViewsApiValidator.java new file mode 100644 index 000000000..2132006d1 --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/ViewsApiValidator.java @@ -0,0 +1,61 @@ +package com.linkedin.openhouse.tables.api.validator; + +import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateViewRequestBody; + +/** + * Structural validation for the /v1 views API. No SQL is parsed, translated or validated against an + * engine here: view SQL stays opaque and semantic rejection belongs to a later admission step. + * + *

Every method throws {@link + * com.linkedin.openhouse.tables.exception.ViewRequestValidationFailureException} carrying all + * accumulated failures joined with {@code "; "}. + */ +public interface ViewsApiValidator { + + /** + * Validate a request to read a single view. + * + * @param databaseId path database identifier + * @param viewId path view identifier + */ + void validateGetView(String databaseId, String viewId); + + /** + * Validate a request to list views in a database. + * + * @param databaseId path database identifier + * @param page zero-based page index + * @param size page size + * @param sortBy optional single sort field + */ + void validateGetAllViews(String databaseId, int page, int size, String sortBy); + + /** + * Validate a POST request to create a view. + * + * @param clusterId name of the serving cluster + * @param databaseId path database identifier + * @param requestBody the create request + */ + void validateCreateView( + String clusterId, String databaseId, CreateUpdateViewRequestBody requestBody); + + /** + * Validate a PUT request to replace or create a view. + * + * @param clusterId name of the serving cluster + * @param databaseId path database identifier + * @param viewId path view identifier + * @param requestBody the update request + */ + void validateUpdateView( + String clusterId, String databaseId, String viewId, CreateUpdateViewRequestBody requestBody); + + /** + * Validate a request to delete a view. + * + * @param databaseId path database identifier + * @param viewId path view identifier + */ + void validateDeleteView(String databaseId, String viewId); +} diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/IcebergSnapshotsApiValidatorImpl.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/IcebergSnapshotsApiValidatorImpl.java index 7c5f1b199..bd8a07102 100644 --- a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/IcebergSnapshotsApiValidatorImpl.java +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/IcebergSnapshotsApiValidatorImpl.java @@ -10,7 +10,6 @@ import com.linkedin.openhouse.tables.api.validator.TablesApiValidator; import java.util.ArrayList; import java.util.List; -import javax.validation.ConstraintViolation; import javax.validation.Validator; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Component; @@ -20,10 +19,6 @@ public class IcebergSnapshotsApiValidatorImpl implements IcebergSnapshotsApiVali @Autowired private TablesApiValidator tablesApiValidator; @Autowired private Validator validator; - // Suppression needed as checkstyle and spotless doesn't agree to each other in terms of whether - // `:` should be - // in the same line with the for-loop main body or not. - @SuppressWarnings("checkstyle:OperatorWrap") @Override public void validatePutSnapshots( String clusterId, @@ -31,11 +26,7 @@ public void validatePutSnapshots( String tableId, IcebergSnapshotsRequestBody icebergSnapshotsRequestBody) { List validationFailures = new ArrayList<>(); - for (ConstraintViolation violation : - validator.validate(icebergSnapshotsRequestBody)) { - validationFailures.add( - String.format("%s : %s", ApiValidatorUtil.getField(violation), violation.getMessage())); - } + ApiValidatorUtil.collectViolations(validator, icebergSnapshotsRequestBody, validationFailures); // Only iff all constraints are fulfilled will it be safe to proceed to rest of check if (validationFailures.isEmpty()) { diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/OpenHouseDatabasesApiValidator.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/OpenHouseDatabasesApiValidator.java index 2b123e1d6..b59fed128 100644 --- a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/OpenHouseDatabasesApiValidator.java +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/OpenHouseDatabasesApiValidator.java @@ -1,17 +1,12 @@ package com.linkedin.openhouse.tables.api.validator.impl; -import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.ALPHA_NUM_UNDERSCORE_ERROR_MSG; -import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.ALPHA_NUM_UNDERSCORE_REGEX; - import com.linkedin.openhouse.common.api.validator.ApiValidatorUtil; import com.linkedin.openhouse.common.exception.RequestValidationFailureException; import com.linkedin.openhouse.tables.api.spec.v0.request.UpdateAclPoliciesRequestBody; import com.linkedin.openhouse.tables.api.validator.DatabasesApiValidator; import java.util.ArrayList; import java.util.List; -import javax.validation.ConstraintViolation; import javax.validation.Validator; -import org.apache.commons.lang.StringUtils; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Component; @@ -20,16 +15,11 @@ public class OpenHouseDatabasesApiValidator implements DatabasesApiValidator { @Autowired private Validator validator; - @SuppressWarnings("checkstyle:OperatorWrap") @Override public void validateUpdateAclPolicies( String databaseId, UpdateAclPoliciesRequestBody updateAclPoliciesRequestBody) { List validationFailures = new ArrayList<>(); - for (ConstraintViolation violation : - validator.validate(updateAclPoliciesRequestBody)) { - validationFailures.add( - String.format("%s : %s", ApiValidatorUtil.getField(violation), violation.getMessage())); - } + ApiValidatorUtil.collectViolations(validator, updateAclPoliciesRequestBody, validationFailures); if (!validationFailures.isEmpty()) { throw new RequestValidationFailureException(validationFailures); } @@ -55,12 +45,7 @@ public void validateGetAllDatabases(int page, int size, String sortBy) { private void validateDatabaseId(String databaseId) { List validationFailures = new ArrayList<>(); - if (StringUtils.isEmpty(databaseId)) { - validationFailures.add("databaseId : Cannot be empty"); - } else if (!databaseId.matches(ALPHA_NUM_UNDERSCORE_REGEX)) { - validationFailures.add( - String.format("databaseId provided: %s, %s", databaseId, ALPHA_NUM_UNDERSCORE_ERROR_MSG)); - } + ApiValidatorUtil.validateIdentifier("databaseId", databaseId, validationFailures); if (!validationFailures.isEmpty()) { throw new RequestValidationFailureException(validationFailures); } diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/OpenHouseTablesApiValidator.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/OpenHouseTablesApiValidator.java index 2b06a70de..158d44022 100644 --- a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/OpenHouseTablesApiValidator.java +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/OpenHouseTablesApiValidator.java @@ -25,10 +25,8 @@ import java.util.Set; import java.util.UUID; import java.util.stream.Collectors; -import javax.validation.ConstraintViolation; import javax.validation.Validator; import lombok.extern.slf4j.Slf4j; -import org.apache.commons.lang.StringUtils; import org.apache.iceberg.Schema; import org.apache.iceberg.SchemaParser; import org.apache.iceberg.SortOrder; @@ -102,18 +100,13 @@ public void validateSearchTables( } } - @SuppressWarnings("checkstyle:OperatorWrap") @Override public void validateCreateTable( String clusterId, String databaseId, CreateUpdateTableRequestBody createUpdateTableRequestBody) { List validationFailures = new ArrayList<>(); - for (ConstraintViolation violation : - validator.validate(createUpdateTableRequestBody)) { - validationFailures.add( - String.format("%s : %s", ApiValidatorUtil.getField(violation), violation.getMessage())); - } + ApiValidatorUtil.collectViolations(validator, createUpdateTableRequestBody, validationFailures); if (!createUpdateTableRequestBody.getClusterId().equals(clusterId)) { validationFailures.add( String.format( @@ -236,7 +229,6 @@ private void validatePolicies(CreateUpdateTableRequestBody createUpdateTableRequ } } - @SuppressWarnings("checkstyle:OperatorWrap") @Override public void validateUpdateTable( String clusterId, @@ -244,11 +236,7 @@ public void validateUpdateTable( String tableId, CreateUpdateTableRequestBody createUpdateTableRequestBody) { List validationFailures = new ArrayList<>(); - for (ConstraintViolation violation : - validator.validate(createUpdateTableRequestBody)) { - validationFailures.add( - String.format("%s : %s", ApiValidatorUtil.getField(violation), violation.getMessage())); - } + ApiValidatorUtil.collectViolations(validator, createUpdateTableRequestBody, validationFailures); if (!createUpdateTableRequestBody.getClusterId().equals(clusterId)) { validationFailures.add( String.format( @@ -337,7 +325,6 @@ public void validateRenameTable( } } - @SuppressWarnings("checkstyle:OperatorWrap") @Override public void validateUpdateAclPolicies( String databaseId, @@ -345,11 +332,7 @@ public void validateUpdateAclPolicies( UpdateAclPoliciesRequestBody updateAclPoliciesRequestBody) { List validationFailures = new ArrayList<>(); - for (ConstraintViolation violation : - validator.validate(updateAclPoliciesRequestBody)) { - validationFailures.add( - String.format("%s : %s", ApiValidatorUtil.getField(violation), violation.getMessage())); - } + ApiValidatorUtil.collectViolations(validator, updateAclPoliciesRequestBody, validationFailures); if (!validationFailures.isEmpty()) { throw new RequestValidationFailureException(validationFailures); } @@ -524,22 +507,11 @@ public Void primitive(Type.PrimitiveType primitive) { } private void validateDatabaseId(String databaseId, List validationFailures) { - if (StringUtils.isEmpty(databaseId)) { - validationFailures.add("databaseId : Cannot be empty"); - } else if (!databaseId.matches(ALPHA_NUM_UNDERSCORE_REGEX)) { - validationFailures.add( - String.format( - "databaseId : provided %s, %s", databaseId, ALPHA_NUM_UNDERSCORE_ERROR_MSG)); - } + ApiValidatorUtil.validateIdentifier("databaseId", databaseId, validationFailures); } private void validateTableId(String tableId, List validationFailures) { - if (StringUtils.isEmpty(tableId)) { - validationFailures.add("tableId : Cannot be empty"); - } else if (!tableId.matches(ALPHA_NUM_UNDERSCORE_REGEX)) { - validationFailures.add( - String.format("tableId : provided %s, %s", tableId, ALPHA_NUM_UNDERSCORE_ERROR_MSG)); - } + ApiValidatorUtil.validateIdentifier("tableId", tableId, validationFailures); } private void validateSortOrder(String sortOrder, String schema, List validationFailures) { diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/OpenHouseViewsApiValidator.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/OpenHouseViewsApiValidator.java new file mode 100644 index 000000000..73b889088 --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/validator/impl/OpenHouseViewsApiValidator.java @@ -0,0 +1,573 @@ +package com.linkedin.openhouse.tables.api.validator.impl; + +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.ALPHA_NUM_UNDERSCORE_ERROR_MSG; +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.ALPHA_NUM_UNDERSCORE_REGEX; +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.INITIAL_TABLE_VERSION; +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.MAX_VIEW_IDENTIFIER_LENGTH; +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.MAX_VIEW_SCHEMA_BYTES; +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.MAX_VIEW_SQL_BYTES; +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.SQL_VIEW_REPRESENTATION_TYPE; + +import com.linkedin.openhouse.cluster.configs.ClusterProperties; +import com.linkedin.openhouse.common.api.validator.ApiValidatorUtil; +import com.linkedin.openhouse.common.schema.IcebergSchemaHelper; +import com.linkedin.openhouse.internal.catalog.mapper.HouseTableSerdeUtils; +import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateViewRequestBody; +import com.linkedin.openhouse.tables.api.spec.v0.request.components.ViewRepresentation; +import com.linkedin.openhouse.tables.api.validator.ViewsApiValidator; +import com.linkedin.openhouse.tables.exception.ViewRequestValidationFailureException; +import com.linkedin.openhouse.tables.exception.ViewValidationErrorCode; +import java.io.UncheckedIOException; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashSet; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.TreeSet; +import java.util.stream.Collectors; +import javax.annotation.PostConstruct; +import javax.validation.Validator; +import lombok.extern.slf4j.Slf4j; +import org.apache.commons.lang3.StringUtils; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Component; + +/** + * Structural validation of /v1 views requests. + * + *

Security invariant: no message built here interpolates SQL text, schema text or a + * {@code baseViewVersion} token. Messages are copied verbatim into the error response body and into + * service audit events, so every payload-derived failure uses a fixed redacted message. + * + *

SQL is opaque: nothing here parses, translates or engine-validates a view definition. + */ +@Slf4j +@Component +public class OpenHouseViewsApiValidator implements ViewsApiValidator { + + /** + * Exact server-owned property key. {@code InternalRepositoryUtils.POLICIES_KEY} carries the same + * literal but is {@code protected} in another package, so it cannot be referenced from here; the + * {@code openhouse.} prefix check does reuse its canonical predicate. + */ + private static final String POLICIES_PROPERTY_KEY = "policies"; + + @Autowired private Validator validator; + + @Autowired private ClusterProperties clusterProperties; + + /** + * The dialects this deployment accepts, normalized once at startup. Configured values are + * lowercased and deduplicated so a comma-separated property is compared the same way whatever + * spacing or casing it was written with, while a caller's dialect still has to match exactly in + * lowercase, as it always has. + * + *

A {@link LinkedHashSet} over sorted values rather than the {@link TreeSet} that sorted them: + * a comparator-ordered set rejects a {@code null} lookup, and a representation may legally reach + * the membership test with no dialect at all. + */ + private Set supportedDialects; + + /** Fixed, server-owned text: the configured set never contains caller-supplied payload. */ + private String supportedDialectsText; + + @PostConstruct + void resolveSupportedDialects() { + Set sorted = + clusterProperties.getViewsSupportedDialects().stream() + .filter(StringUtils::isNotBlank) + .map(dialect -> dialect.trim().toLowerCase(Locale.ROOT)) + .collect(Collectors.toCollection(TreeSet::new)); + supportedDialects = Collections.unmodifiableSet(new LinkedHashSet<>(sorted)); + supportedDialectsText = String.join(", ", supportedDialects); + log.info("Views API accepting the configured view dialects: {}", supportedDialectsText); + } + + @Override + public void validateGetView(String databaseId, String viewId) { + ViewValidationFailures failures = new ViewValidationFailures(); + validateDatabaseId(databaseId, failures); + validateViewId(viewId, failures); + failures.throwIfPresent(); + } + + @Override + public void validateGetAllViews(String databaseId, int page, int size, String sortBy) { + ViewValidationFailures failures = new ViewValidationFailures(); + validateDatabaseId(databaseId, failures); + ApiValidatorUtil.validatePageable(page, size, sortBy, failures.getMessages()); + failures.throwIfPresent(); + } + + @Override + public void validateCreateView( + String clusterId, String databaseId, CreateUpdateViewRequestBody requestBody) { + ViewValidationFailures failures = new ViewValidationFailures(); + validateBody(clusterId, databaseId, requestBody, failures); + // POST distinguishes only "supplied" from "omitted": a supplied-but-blank token is a value the + // rule below has to reject, not an absence. + validateCreateBaseViewVersion(suppliedField(requestBody.getBaseViewVersion()), failures); + failures.throwIfPresent(); + } + + @Override + public void validateUpdateView( + String clusterId, String databaseId, String viewId, CreateUpdateViewRequestBody requestBody) { + ViewValidationFailures failures = new ViewValidationFailures(); + validateBody(clusterId, databaseId, requestBody, failures); + if (requestBody.getViewId() != null && !requestBody.getViewId().equals(viewId)) { + failures.addGeneric( + String.format( + "viewId : provided %s, doesn't match with the RequestBody %s", + viewId, requestBody.getViewId())); + } + validateUpdateBaseViewVersion(nonBlankField(requestBody.getBaseViewVersion()), failures); + failures.throwIfPresent(); + } + + @Override + public void validateDeleteView(String databaseId, String viewId) { + // Identifier rules are identical to a read, so reuse them. + validateGetView(databaseId, viewId); + } + + /** + * Rules shared by POST and PUT. Verb-specific base-version rules are applied by the caller. + * + *

This is the one place absence is decided. Every nullable field of the request body is + * converted here to an explicit {@link Optional}, so no rule below re-derives what "not supplied" + * means for its own field. The two list fields are normalized differently on purpose: + * + *

    + *
  • {@code representations}: an omitted list and an empty list are the same client mistake + * and are both already reported by bean validation, so an empty list collapses to absent. + *
  • {@code defaultNamespace}: the field is optional, but an explicitly empty list is a + * distinct client error with its own message, so it stays present. + *
+ */ + private void validateBody( + String clusterId, + String databaseId, + CreateUpdateViewRequestBody requestBody, + ViewValidationFailures failures) { + // Bean violations stay in the generic category, so they are collected into a plain list first + // and then forwarded, rather than influencing the error-code precedence. + List beanViolations = new ArrayList<>(); + ApiValidatorUtil.collectViolations(validator, requestBody, beanViolations); + beanViolations.forEach(failures::addGeneric); + if (requestBody.getClusterId() != null && !requestBody.getClusterId().equals(clusterId)) { + failures.addGeneric( + String.format( + "clusterId : provided %s, doesn't match with the server cluster %s", + requestBody.getClusterId(), clusterId)); + } + if (requestBody.getDatabaseId() != null && !requestBody.getDatabaseId().equals(databaseId)) { + failures.addGeneric( + String.format( + "databaseId : provided %s, doesn't match with the RequestBody %s", + databaseId, requestBody.getDatabaseId())); + } + + Optional schema = nonEmptyField(requestBody.getSchema()); + Optional> representations = + nonEmptyField(requestBody.getRepresentations()); + Optional sourceDialect = nonEmptyField(requestBody.getSourceDialect()); + Optional defaultCatalog = suppliedField(requestBody.getDefaultCatalog()); + Optional> defaultNamespace = suppliedField(requestBody.getDefaultNamespace()); + Optional> viewProperties = nonEmptyField(requestBody.getViewProperties()); + + validateSchema(schema, failures); + validateRepresentations(representations, failures); + validateUniqueDialects(representations, failures); + validateSourceDialect(sourceDialect, representations, failures); + validateDefaultCatalog(defaultCatalog, failures); + validateDefaultNamespace(defaultNamespace, failures); + validateViewProperties(viewProperties, failures); + } + + /** Present whenever the caller supplied the field at all, including as a blank or empty value. */ + private static Optional suppliedField(T value) { + return Optional.ofNullable(value); + } + + /** Absent when the field was omitted or supplied empty; the two are equivalent to every rule. */ + private static Optional nonEmptyField(String value) { + return Optional.ofNullable(value).filter(StringUtils::isNotEmpty); + } + + private static Optional> nonEmptyField(List value) { + return Optional.ofNullable(value).filter(list -> !list.isEmpty()); + } + + private static Optional> nonEmptyField(Map value) { + return Optional.ofNullable(value).filter(map -> !map.isEmpty()); + } + + /** Absent when the field was omitted or supplied blank; the two are equivalent to every rule. */ + private static Optional nonBlankField(String value) { + return Optional.ofNullable(value).filter(StringUtils::isNotBlank); + } + + /** + * Parse the schema with Iceberg. Iceberg already rejects duplicate field ids and malformed JSON, + * so this only has to wrap the failure; there is deliberately no separate duplicate-id check. + * + *

The size rule runs first and short-circuits parsing, so an oversized document is never fed + * to the parser. + * + *

Only the two failures Iceberg raises for caller-supplied text are caught. {@code + * SchemaParser} reports a structurally valid document that is not an Iceberg schema — including + * the Spark {@code StructType} shape and duplicate field ids — as an {@link + * IllegalArgumentException}, and text that is not JSON at all as an {@link UncheckedIOException} + * wrapping Jackson's parse failure. Anything else is a server fault and must propagate as a 500 + * rather than be reported to the caller as a bad request. + */ + private void validateSchema(Optional maybeSchema, ViewValidationFailures failures) { + if (!maybeSchema.isPresent()) { + // An absent schema is already reported by bean validation. + return; + } + String schema = maybeSchema.get(); + if (utf8Size(schema) > MAX_VIEW_SCHEMA_BYTES) { + failures.addSchema( + String.format("schema : exceeds maximum UTF-8 size of %d bytes", MAX_VIEW_SCHEMA_BYTES)); + return; + } + try { + IcebergSchemaHelper.getSchemaFromSchemaJson(schema); + } catch (IllegalArgumentException | UncheckedIOException e) { + // Only the exception type is logged: the parser message can echo the caller's schema text. + log.warn("Rejected a view request with an unparseable Iceberg schema: {}", e.getClass()); + failures.addSchema( + "schema : must be valid Iceberg schema JSON; Spark StructType JSON is not supported"); + } + } + + /** + * Every representation must name a dialect this deployment supports. There is deliberately no + * rule on how many representations a request may carry: {@link #validateUniqueDialects} already + * rejects an ambiguous request, so a request carrying one representation per supported dialect is + * well formed and is accepted. + */ + private void validateRepresentations( + Optional> maybeRepresentations, ViewValidationFailures failures) { + if (!maybeRepresentations.isPresent()) { + // An absent or empty list is already reported by bean validation. + return; + } + List representations = maybeRepresentations.get(); + for (int index = 0; index < representations.size(); index++) { + ViewRepresentation representation = representations.get(index); + if (representation == null) { + failures.addGeneric(String.format("representations[%d] : cannot be null", index)); + continue; + } + if (!SQL_VIEW_REPRESENTATION_TYPE.equals(representation.getType())) { + failures.addGeneric( + String.format( + "representations[%d].type : must be '%s'", index, SQL_VIEW_REPRESENTATION_TYPE)); + } + if (!supportedDialects.contains(representation.getDialect())) { + failures.addDialect( + String.format( + "representations[%d].dialect : must be one of the supported dialects: %s", + index, supportedDialectsText)); + } + validateRepresentationSql(index, representation.getSql(), failures); + } + } + + /** + * SQL is opaque, so the only rule is a size ceiling. It is counted in UTF-8 bytes rather than + * characters, which is what a {@code @Size} bean constraint would have counted. + */ + private void validateRepresentationSql(int index, String sql, ViewValidationFailures failures) { + if (StringUtils.isEmpty(sql)) { + // An absent SQL text is already reported by bean validation. + return; + } + if (utf8Size(sql) > MAX_VIEW_SQL_BYTES) { + failures.addGeneric( + String.format( + "representations[%d].sql : exceeds maximum UTF-8 size of %d bytes", + index, MAX_VIEW_SQL_BYTES)); + } + } + + /** + * Dialects identify a representation, so two representations claiming the same dialect are + * ambiguous. Compared case-insensitively: {@code SPARK} and {@code spark} name the same engine, + * and rejecting the pair as duplicates is more useful than reporting only the casing failure. + */ + private void validateUniqueDialects( + Optional> maybeRepresentations, ViewValidationFailures failures) { + List representations = maybeRepresentations.orElse(Collections.emptyList()); + Set seen = new HashSet<>(); + Set duplicates = new TreeSet<>(); + for (ViewRepresentation representation : representations) { + if (representation == null || StringUtils.isEmpty(representation.getDialect())) { + continue; + } + String normalized = representation.getDialect().toLowerCase(Locale.ROOT); + if (!seen.add(normalized)) { + duplicates.add(normalized); + } + } + if (!duplicates.isEmpty()) { + failures.addDialect( + String.format( + "representations : dialects must be unique, duplicated: %s", + String.join(", ", duplicates))); + } + } + + /** + * The source dialect carries two separate obligations: it must name a dialect this deployment + * supports, and it must name one of the representations actually supplied. The first is about + * what the server can serve, the second about what this request defines. + */ + private void validateSourceDialect( + Optional maybeSourceDialect, + Optional> maybeRepresentations, + ViewValidationFailures failures) { + if (!maybeSourceDialect.isPresent()) { + // An absent source dialect is already reported by bean validation. + return; + } + String sourceDialect = maybeSourceDialect.get(); + if (!supportedDialects.contains(sourceDialect)) { + failures.addDialect( + String.format( + "sourceDialect : must be one of the supported dialects: %s", supportedDialectsText)); + return; + } + // Only meaningful when at least one usable representation was supplied. With none, the missing + // or null representation is already reported, and adding a second message here would both + // duplicate that diagnosis and promote a malformed body to the more specific dialect code. + List suppliedRepresentations = + maybeRepresentations.orElse(Collections.emptyList()).stream() + .filter(Objects::nonNull) + .collect(Collectors.toList()); + if (suppliedRepresentations.isEmpty()) { + return; + } + if (suppliedRepresentations.stream() + .noneMatch(representation -> sourceDialect.equals(representation.getDialect()))) { + failures.addDialect("sourceDialect : does not name a supplied representation"); + } + } + + /** + * The resolution catalog is optional, but supplying a blank or unbounded one is a client bug + * rather than an omission, so it is rejected instead of silently ignored. + */ + private void validateDefaultCatalog( + Optional maybeDefaultCatalog, ViewValidationFailures failures) { + if (!maybeDefaultCatalog.isPresent()) { + return; + } + String defaultCatalog = maybeDefaultCatalog.get(); + if (StringUtils.isBlank(defaultCatalog)) { + failures.addGeneric("defaultCatalog : cannot be blank when provided"); + } else if (defaultCatalog.length() > MAX_VIEW_IDENTIFIER_LENGTH) { + failures.addGeneric( + String.format( + "defaultCatalog : exceeds the maximum length of %d characters", + MAX_VIEW_IDENTIFIER_LENGTH)); + } + } + + /** + * Namespace segments follow the same identifier rules as a database id. Messages are indexed but + * fixed: the offending segment is never echoed, keeping every payload-derived message redacted. + */ + private void validateDefaultNamespace( + Optional> maybeDefaultNamespace, ViewValidationFailures failures) { + if (!maybeDefaultNamespace.isPresent()) { + return; + } + List defaultNamespace = maybeDefaultNamespace.get(); + if (defaultNamespace.isEmpty()) { + failures.addGeneric("defaultNamespace : cannot be empty when provided"); + return; + } + for (int index = 0; index < defaultNamespace.size(); index++) { + String segment = defaultNamespace.get(index); + if (StringUtils.isBlank(segment)) { + failures.addGeneric(String.format("defaultNamespace[%d] : cannot be blank", index)); + } else if (!segment.matches(ALPHA_NUM_UNDERSCORE_REGEX)) { + failures.addGeneric( + String.format("defaultNamespace[%d] : %s", index, ALPHA_NUM_UNDERSCORE_ERROR_MSG)); + } else if (segment.length() > MAX_VIEW_IDENTIFIER_LENGTH) { + failures.addGeneric( + String.format( + "defaultNamespace[%d] : exceeds the maximum length of %d characters", + index, MAX_VIEW_IDENTIFIER_LENGTH)); + } + } + } + + /** + * View properties are user-owned, with two exceptions carved out for the server: the {@code + * openhouse.} namespace, whose canonical case-sensitive predicate is reused from the internal + * catalog, and the exact key {@code policies}. Case sensitivity is deliberate and inherited: a + * user property such as {@code OpenHouse.myTeam} stays legal. + * + *

Property keys are user-authored identifiers rather than payload text, so listing the + * offending keys is intentional and does not breach the SQL/schema/token redaction invariant. + */ + private void validateViewProperties( + Optional> maybeViewProperties, ViewValidationFailures failures) { + if (!maybeViewProperties.isPresent()) { + return; + } + Map viewProperties = maybeViewProperties.get(); + boolean blankKey = false; + Set nullValueKeys = new TreeSet<>(); + Set reservedKeys = new TreeSet<>(); + for (Map.Entry property : viewProperties.entrySet()) { + String key = property.getKey(); + if (StringUtils.isBlank(key)) { + blankKey = true; + continue; + } + if (property.getValue() == null) { + nullValueKeys.add(key); + } + if (HouseTableSerdeUtils.IS_OH_PREFIXED.test(key) || POLICIES_PROPERTY_KEY.equals(key)) { + reservedKeys.add(key); + } + } + if (blankKey) { + failures.addGeneric("viewProperties : property keys cannot be blank"); + } + if (!nullValueKeys.isEmpty()) { + failures.addGeneric( + String.format( + "viewProperties : property values cannot be null, keys: %s", + String.join(", ", nullValueKeys))); + } + if (!reservedKeys.isEmpty()) { + failures.addGeneric( + String.format( + "viewProperties : reserved keys are not allowed: %s", + String.join(", ", reservedKeys))); + } + } + + /** Counts UTF-8 bytes, not UTF-16 characters. */ + private static int utf8Size(String value) { + return value.getBytes(StandardCharsets.UTF_8).length; + } + + /** + * POST accepts an omitted base version or the table-style {@code INITIAL_VERSION} token, matching + * both the Iceberg client, which sends the initial token on create, and callers that omit the + * field entirely. + */ + private void validateCreateBaseViewVersion( + Optional baseViewVersion, ViewValidationFailures failures) { + if (baseViewVersion.isPresent() && !INITIAL_TABLE_VERSION.equals(baseViewVersion.get())) { + failures.addGeneric( + "baseViewVersion : must be omitted or " + INITIAL_TABLE_VERSION + " on POST create"); + } + } + + /** + * PUT requires a base version but treats it as fully opaque: no path, scheme, suffix or length + * rule is applied, so the service alone decides whether the token is current. + * + *

The caller normalizes a blank token to absent, because a token of whitespace is + * indistinguishable from an omitted one to every rule here. + */ + private void validateUpdateBaseViewVersion( + Optional baseViewVersion, ViewValidationFailures failures) { + if (!baseViewVersion.isPresent()) { + failures.addGeneric("baseViewVersion : is required and cannot be blank on PUT"); + } + } + + private void validateDatabaseId(String databaseId, ViewValidationFailures failures) { + List identifierFailures = new ArrayList<>(); + ApiValidatorUtil.validateIdentifier("databaseId", databaseId, identifierFailures); + if (!identifierFailures.isEmpty()) { + identifierFailures.forEach(failures::addGeneric); + } else if (databaseId.length() > MAX_VIEW_IDENTIFIER_LENGTH) { + failures.addGeneric(identifierTooLong("databaseId")); + } + } + + private void validateViewId(String viewId, ViewValidationFailures failures) { + List identifierFailures = new ArrayList<>(); + ApiValidatorUtil.validateIdentifier("viewId", viewId, identifierFailures); + if (!identifierFailures.isEmpty()) { + identifierFailures.forEach(failures::addGeneric); + } else if (viewId.length() > MAX_VIEW_IDENTIFIER_LENGTH) { + failures.addGeneric(identifierTooLong("viewId")); + } + } + + /** + * Deliberately omits the offending value. Every other identifier message echoes it, but an + * over-long identifier is by definition large and the message is copied into the error body and + * into service audit events. + */ + private static String identifierTooLong(String field) { + return String.format( + "%s : exceeds the maximum length of %d characters", field, MAX_VIEW_IDENTIFIER_LENGTH); + } + + /** + * Accumulates failure messages in discovery order while separately remembering whether a schema + * or dialect rule failed, so the thrown exception can carry the most specific internal code. + * + *

Precedence is schema, then dialect, then the generic definition code. All three map to 400, + * so the choice is observable only to internal callers and tests. + */ + private static final class ViewValidationFailures { + private final List messages = new ArrayList<>(); + private boolean schemaFailure; + private boolean dialectFailure; + + private List getMessages() { + return messages; + } + + private void addGeneric(String message) { + messages.add(message); + } + + private void addSchema(String message) { + schemaFailure = true; + messages.add(message); + } + + private void addDialect(String message) { + dialectFailure = true; + messages.add(message); + } + + private void throwIfPresent() { + if (messages.isEmpty()) { + return; + } + throw new ViewRequestValidationFailureException(errorCode(), messages); + } + + private ViewValidationErrorCode errorCode() { + if (schemaFailure) { + return ViewValidationErrorCode.UNSUPPORTED_VIEW_SCHEMA; + } + if (dialectFailure) { + return ViewValidationErrorCode.UNSUPPORTED_VIEW_DIALECT; + } + return ViewValidationErrorCode.INVALID_VIEW_DEFINITION; + } + } +} diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/audit/ViewRequestPayloadRedactor.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/audit/ViewRequestPayloadRedactor.java new file mode 100644 index 000000000..3e8ecbadf --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/audit/ViewRequestPayloadRedactor.java @@ -0,0 +1,74 @@ +package com.linkedin.openhouse.tables.audit; + +import com.google.gson.JsonElement; +import com.google.gson.JsonObject; +import com.google.gson.JsonPrimitive; +import com.linkedin.openhouse.common.audit.ServiceAuditPayloadRedactor; +import javax.servlet.http.HttpServletRequest; +import org.springframework.stereotype.Component; +import org.springframework.util.AntPathMatcher; + +/** + * Keeps view definitions out of service audit events. + * + *

{@link com.linkedin.openhouse.common.audit.ServiceAuditAspect} audits the complete cached + * request body of every controller call, which for the view create and replace routes would retain + * the caller's full SQL text and schema document. This replaces {@code schema} and every {@code + * representations[*].sql} value with {@link #REDACTED_VALUE} before the event is built. The keys + * are kept, so an auditor still sees that the fields were sent. + * + *

Scoped by request URI rather than by field name on purpose. {@code + * CreateUpdateTableRequestBody} also carries a {@code schema}, and redacting by name alone would + * silently change table, database and snapshot audit payloads. Matching the view routes leaves + * every other route's payload exactly as it was. + * + *

Every field that is not part of the view definition — {@code viewId}, {@code databaseId}, + * {@code clusterId}, {@code sourceDialect}, {@code defaultCatalog}, {@code defaultNamespace}, + * {@code viewProperties} and {@code baseViewVersion} — is left intact, so an audit event still + * identifies what was operated on and by whom. + */ +@Component +public class ViewRequestPayloadRedactor implements ServiceAuditPayloadRedactor { + + static final String SCHEMA_FIELD = "schema"; + static final String REPRESENTATIONS_FIELD = "representations"; + static final String SQL_FIELD = "sql"; + + /** The view collection route, which POST creates against. */ + private static final String VIEW_COLLECTION_PATTERN = "/v1/databases/*/views"; + + /** The view item route, which PUT replaces against. */ + private static final String VIEW_ITEM_PATTERN = "/v1/databases/*/views/*"; + + private static final AntPathMatcher PATH_MATCHER = new AntPathMatcher(); + + @Override + public boolean appliesTo(HttpServletRequest request) { + String uri = request.getRequestURI(); + return uri != null + && (PATH_MATCHER.match(VIEW_COLLECTION_PATTERN, uri) + || PATH_MATCHER.match(VIEW_ITEM_PATTERN, uri)); + } + + @Override + public JsonElement redact(JsonElement requestPayload) { + if (requestPayload == null || !requestPayload.isJsonObject()) { + // A bodyless request parses to JsonNull, and a malformed body can be any other element. + // Neither carries a view definition, so there is nothing to remove. + return requestPayload; + } + JsonObject redacted = requestPayload.deepCopy().getAsJsonObject(); + if (redacted.has(SCHEMA_FIELD)) { + redacted.add(SCHEMA_FIELD, new JsonPrimitive(REDACTED_VALUE)); + } + JsonElement representations = redacted.get(REPRESENTATIONS_FIELD); + if (representations != null && representations.isJsonArray()) { + for (JsonElement representation : representations.getAsJsonArray()) { + if (representation.isJsonObject() && representation.getAsJsonObject().has(SQL_FIELD)) { + representation.getAsJsonObject().add(SQL_FIELD, new JsonPrimitive(REDACTED_VALUE)); + } + } + } + return redacted; + } +} diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/authorization/Privileges.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/authorization/Privileges.java index be544a116..f8e61574f 100644 --- a/services/tables/src/main/java/com/linkedin/openhouse/tables/authorization/Privileges.java +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/authorization/Privileges.java @@ -16,7 +16,11 @@ public enum Privileges { UPDATE_ACL(Privilege.UPDATE_ACL), SYSTEM_ADMIN(Privilege.SYSTEM_ADMIN), LOCK_ADMIN(Privilege.LOCK_ADMIN), - SELECT(Privilege.SELECT); + SELECT(Privilege.SELECT), + CREATE_VIEW(Privilege.CREATE_VIEW), + LIST_VIEW(Privilege.LIST_VIEW), + UPDATE_VIEW_METADATA(Privilege.UPDATE_VIEW_METADATA), + DELETE_VIEW(Privilege.DELETE_VIEW); private String privilege; @@ -43,6 +47,20 @@ public static class Privilege { public static final String LOCK_ADMIN = "LOCK_ADMIN"; public static final String SELECT = "SELECT"; + + /** + * View privileges. Reads of a single view reuse {@link #SELECT}; the remaining view operations + * get their own constants so a later authorization implementation can grant them independently + * of the table privileges. + */ + public static final String CREATE_VIEW = "CREATE_VIEW"; + + public static final String LIST_VIEW = "LIST_VIEW"; + + public static final String UPDATE_VIEW_METADATA = "UPDATE_VIEW_METADATA"; + + public static final String DELETE_VIEW = "DELETE_VIEW"; + private static final Set SUPPORTED_PRIVILEGES = Stream.of(Privileges.values()).map(Privileges::getPrivilege).collect(Collectors.toSet()); diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/controller/ViewsController.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/controller/ViewsController.java new file mode 100644 index 000000000..f645054dd --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/controller/ViewsController.java @@ -0,0 +1,208 @@ +package com.linkedin.openhouse.tables.controller; + +import static com.linkedin.openhouse.common.security.AuthenticationUtils.*; + +import com.linkedin.openhouse.tables.api.handler.ViewsApiHandler; +import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateViewRequestBody; +import com.linkedin.openhouse.tables.api.spec.v0.response.GetAllViewsResponseBody; +import com.linkedin.openhouse.tables.api.spec.v0.response.GetViewResponseBody; +import com.linkedin.openhouse.tables.authorization.Privileges; +import io.swagger.v3.oas.annotations.Operation; +import io.swagger.v3.oas.annotations.Parameter; +import io.swagger.v3.oas.annotations.media.Schema; +import io.swagger.v3.oas.annotations.responses.ApiResponse; +import io.swagger.v3.oas.annotations.responses.ApiResponses; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.http.ResponseEntity; +import org.springframework.security.access.annotation.Secured; +import org.springframework.web.bind.annotation.DeleteMapping; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.PathVariable; +import org.springframework.web.bind.annotation.PostMapping; +import org.springframework.web.bind.annotation.PutMapping; +import org.springframework.web.bind.annotation.RequestBody; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.RestController; + +/** + * Controller for the Views API. Views are a new resource under {@code /v1}, alongside every other + * OpenHouse resource, and break no existing client: they occupy their own {@code views} path + * segment, and {@code /v1/databases/{databaseId}/tables/{tableId}} stays table-only. + * + *

The controller is registered regardless of whether views are enabled, and holds no business + * logic. Request bodies are deliberately not annotated with {@code @Valid}: the view validator + * accumulates every structural failure and reports them together, which Spring's fail-fast binding + * cannot do. + */ +@RestController +public class ViewsController { + + @Autowired private ViewsApiHandler viewsApiHandler; + + @Operation( + summary = "Get View in a Database", + description = + "Returns a View resource identified by viewId in the database identified by databaseId.", + tags = {"View"}) + @ApiResponses( + value = { + @ApiResponse(responseCode = "200", description = "View GET: OK"), + @ApiResponse(responseCode = "400", description = "View GET: BAD_REQUEST"), + @ApiResponse(responseCode = "401", description = "View GET: UNAUTHORIZED"), + @ApiResponse(responseCode = "403", description = "View GET: FORBIDDEN"), + @ApiResponse(responseCode = "404", description = "View GET: NOT_FOUND"), + @ApiResponse(responseCode = "503", description = "View GET: SERVICE_UNAVAILABLE") + }) + @GetMapping( + value = {"/v1/databases/{databaseId}/views/{viewId}"}, + produces = {"application/json"}) + @Secured(value = Privileges.Privilege.SELECT) + public ResponseEntity getView( + @Parameter(description = "Database ID", required = true) @PathVariable String databaseId, + @Parameter(description = "View ID", required = true) @PathVariable String viewId) { + + com.linkedin.openhouse.common.api.spec.ApiResponse apiResponse = + viewsApiHandler.getView(databaseId, viewId, extractAuthenticatedUserPrincipal()); + + return new ResponseEntity<>( + apiResponse.getResponseBody(), apiResponse.getHttpHeaders(), apiResponse.getHttpStatus()); + } + + @Operation( + summary = "Search Views in a Database", + description = "Returns a Page of View resources present in a database.", + tags = {"View"}) + @ApiResponses( + value = { + @ApiResponse(responseCode = "200", description = "View SEARCH: OK"), + @ApiResponse(responseCode = "400", description = "View SEARCH: BAD_REQUEST"), + @ApiResponse(responseCode = "401", description = "View SEARCH: UNAUTHORIZED"), + @ApiResponse(responseCode = "403", description = "View SEARCH: FORBIDDEN"), + @ApiResponse(responseCode = "404", description = "View SEARCH: NOT_FOUND"), + @ApiResponse(responseCode = "503", description = "View SEARCH: SERVICE_UNAVAILABLE") + }) + @GetMapping( + value = {"/v1/databases/{databaseId}/views"}, + produces = {"application/json"}) + @Secured(value = Privileges.Privilege.LIST_VIEW) + public ResponseEntity getAllViews( + @Parameter(description = "Database ID", required = true) @PathVariable String databaseId, + @RequestParam(required = false, defaultValue = "0") int page, + @RequestParam(required = false, defaultValue = "50") int size, + @RequestParam(required = false) String sortBy) { + + com.linkedin.openhouse.common.api.spec.ApiResponse apiResponse = + viewsApiHandler.getAllViews( + databaseId, page, size, sortBy, extractAuthenticatedUserPrincipal()); + + return new ResponseEntity<>( + apiResponse.getResponseBody(), apiResponse.getHttpHeaders(), apiResponse.getHttpStatus()); + } + + @Operation( + summary = "Create a View", + description = "Creates and returns a View resource in a database identified by databaseId", + tags = {"View"}) + @ApiResponses( + value = { + @ApiResponse(responseCode = "201", description = "View POST: CREATED"), + @ApiResponse(responseCode = "400", description = "View POST: BAD_REQUEST"), + @ApiResponse(responseCode = "401", description = "View POST: UNAUTHORIZED"), + @ApiResponse(responseCode = "403", description = "View POST: FORBIDDEN"), + @ApiResponse(responseCode = "404", description = "View POST: DB_NOT_FOUND"), + @ApiResponse(responseCode = "409", description = "View POST: VIEW_EXISTS"), + @ApiResponse(responseCode = "422", description = "View POST: UNPROCESSABLE_ENTITY"), + @ApiResponse(responseCode = "503", description = "View POST: SERVICE_UNAVAILABLE") + }) + @PostMapping( + value = {"/v1/databases/{databaseId}/views"}, + produces = {"application/json"}, + consumes = {"application/json"}) + @Secured(value = Privileges.Privilege.CREATE_VIEW) + public ResponseEntity createView( + @Parameter(description = "Database ID", required = true) @PathVariable String databaseId, + @Parameter( + description = "Request containing details of the View to be created", + required = true, + schema = @Schema(implementation = CreateUpdateViewRequestBody.class)) + @RequestBody + CreateUpdateViewRequestBody createUpdateViewRequestBody) { + + com.linkedin.openhouse.common.api.spec.ApiResponse apiResponse = + viewsApiHandler.createView( + databaseId, createUpdateViewRequestBody, extractAuthenticatedUserPrincipal()); + + return new ResponseEntity<>( + apiResponse.getResponseBody(), apiResponse.getHttpHeaders(), apiResponse.getHttpStatus()); + } + + @Operation( + summary = "Update a View", + description = + "Updates or creates a View and returns the View resource. If the view does not exist, it " + + "will be created. If the view exists, it will be replaced.", + tags = {"View"}) + @ApiResponses( + value = { + @ApiResponse(responseCode = "200", description = "View PUT: UPDATED"), + @ApiResponse(responseCode = "201", description = "View PUT: CREATED"), + @ApiResponse(responseCode = "400", description = "View PUT: BAD_REQUEST"), + @ApiResponse(responseCode = "401", description = "View PUT: UNAUTHORIZED"), + @ApiResponse(responseCode = "403", description = "View PUT: FORBIDDEN"), + @ApiResponse(responseCode = "404", description = "View PUT: DB_NOT_FOUND"), + @ApiResponse(responseCode = "409", description = "View PUT: CONFLICT"), + @ApiResponse(responseCode = "422", description = "View PUT: UNPROCESSABLE_ENTITY"), + @ApiResponse(responseCode = "503", description = "View PUT: SERVICE_UNAVAILABLE") + }) + @PutMapping( + value = {"/v1/databases/{databaseId}/views/{viewId}"}, + produces = {"application/json"}, + consumes = {"application/json"}) + @Secured(value = Privileges.Privilege.UPDATE_VIEW_METADATA) + public ResponseEntity updateView( + @Parameter(description = "Database ID", required = true) @PathVariable String databaseId, + @Parameter(description = "View ID", required = true) @PathVariable String viewId, + @Parameter( + description = "Request containing details of the View to be created/updated", + required = true, + schema = @Schema(implementation = CreateUpdateViewRequestBody.class)) + @RequestBody + CreateUpdateViewRequestBody createUpdateViewRequestBody) { + + com.linkedin.openhouse.common.api.spec.ApiResponse apiResponse = + viewsApiHandler.updateView( + databaseId, viewId, createUpdateViewRequestBody, extractAuthenticatedUserPrincipal()); + + return new ResponseEntity<>( + apiResponse.getResponseBody(), apiResponse.getHttpHeaders(), apiResponse.getHttpStatus()); + } + + @Operation( + summary = "Drop a View", + description = + "Drops a View resource identified by viewId in the database identified by databaseId.", + tags = {"View"}) + @ApiResponses( + value = { + @ApiResponse(responseCode = "204", description = "View DELETE: NO_CONTENT"), + @ApiResponse(responseCode = "400", description = "View DELETE: BAD_REQUEST"), + @ApiResponse(responseCode = "401", description = "View DELETE: UNAUTHORIZED"), + @ApiResponse(responseCode = "403", description = "View DELETE: FORBIDDEN"), + @ApiResponse(responseCode = "404", description = "View DELETE: VIEW_NOT_FOUND"), + @ApiResponse(responseCode = "503", description = "View DELETE: SERVICE_UNAVAILABLE") + }) + @DeleteMapping( + value = {"/v1/databases/{databaseId}/views/{viewId}"}, + produces = {"application/json"}) + @Secured(value = Privileges.Privilege.DELETE_VIEW) + public ResponseEntity deleteView( + @Parameter(description = "Database ID", required = true) @PathVariable String databaseId, + @Parameter(description = "View ID", required = true) @PathVariable String viewId) { + + com.linkedin.openhouse.common.api.spec.ApiResponse apiResponse = + viewsApiHandler.deleteView(databaseId, viewId, extractAuthenticatedUserPrincipal()); + + return new ResponseEntity<>( + apiResponse.getResponseBody(), apiResponse.getHttpHeaders(), apiResponse.getHttpStatus()); + } +} diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/dto/mapper/ViewsMapper.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/dto/mapper/ViewsMapper.java new file mode 100644 index 000000000..48fb831c7 --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/dto/mapper/ViewsMapper.java @@ -0,0 +1,67 @@ +package com.linkedin.openhouse.tables.dto.mapper; + +import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateViewRequestBody; +import com.linkedin.openhouse.tables.api.spec.v0.response.GetViewResponseBody; +import com.linkedin.openhouse.tables.model.ViewDto; +import org.mapstruct.Mapper; +import org.mapstruct.Mapping; +import org.mapstruct.Mappings; +import org.springframework.data.domain.Page; + +/** Mapper between the /v1 views wire models and {@link ViewDto}. */ +@Mapper(componentModel = "spring") +public interface ViewsMapper { + + /** + * Transform a create/update request into a {@link ViewDto} for the service layer. + * + *

The caller-supplied {@code baseViewVersion} is stored as {@code viewVersion} so the service + * can compare it against the current pointer later, mirroring how {@code TablesMapper} stores + * {@code baseTableVersion} as {@code tableVersion}. Server-owned pointer fields are left unset: + * only the service can populate them. + * + * @param requestBody source request + * @return a new immutable {@link ViewDto} + */ + @Mappings({ + @Mapping(source = "viewId", target = "viewId"), + @Mapping(source = "databaseId", target = "databaseId"), + @Mapping(source = "clusterId", target = "clusterId"), + @Mapping(source = "schema", target = "schema"), + @Mapping(source = "representations", target = "representations"), + @Mapping(source = "sourceDialect", target = "sourceDialect"), + @Mapping(source = "defaultCatalog", target = "defaultCatalog"), + @Mapping(source = "defaultNamespace", target = "defaultNamespace"), + @Mapping(source = "viewProperties", target = "viewProperties"), + @Mapping( + source = "baseViewVersion", + target = "viewVersion"), /* store base version to check later */ + @Mapping(target = "viewUri", ignore = true), + @Mapping(target = "metadataLocation", ignore = true), + @Mapping(target = "viewCreator", ignore = true), + @Mapping(target = "creationTime", ignore = true), + @Mapping(target = "lastModifiedTime", ignore = true) + }) + ViewDto toViewDto(CreateUpdateViewRequestBody requestBody); + + /** + * Transform a {@link ViewDto} into the pointer-only read contract. Definition fields on the DTO + * have no counterpart on the response by design and are dropped here. + * + * @param viewDto source dto + * @return the response body forwarded to the client + */ + GetViewResponseBody toGetViewResponseBody(ViewDto viewDto); + + /** + * Transform a page of {@link ViewDto} into a page of response bodies, preserving the page + * metadata. List DTOs carry identifiers only, so the resulting response bodies are intentionally + * sparse. + * + * @param viewDtoPage source page + * @return a page of sparse response bodies + */ + default Page toGetViewResponseBodyPage(Page viewDtoPage) { + return viewDtoPage.map(this::toGetViewResponseBody); + } +} diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/exception/ViewApiException.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/exception/ViewApiException.java new file mode 100644 index 000000000..93363f6f9 --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/exception/ViewApiException.java @@ -0,0 +1,49 @@ +package com.linkedin.openhouse.tables.exception; + +import com.linkedin.openhouse.common.exception.CodedApiException; +import java.util.Objects; +import org.springframework.http.HttpStatus; + +/** + * Failure of a /v1 views API operation, carrying the internal {@link ViewErrorCode} that selects + * the response status. + * + *

The code is deliberately tables-local and never serialized: {@link #getHttpStatus()} is the + * only thing {@code services/common} sees, which keeps the error body shape unchanged. The typed + * {@link #getErrorCode()} getter exists so unit tests can assert the internal taxonomy directly. + * + *

Messages carried by this exception are copied verbatim into the error response body and into + * service audit events, so callers must never interpolate SQL text, schema text or a base version + * token into them. + * + *

The code is required. Without the null check the failure would surface only when {@link + * #getHttpStatus()} is called, which happens inside the exception handler: the resulting {@code + * NullPointerException} would be reported as a generic 500 instead of the status the throwing site + * intended. Rejecting the null at construction keeps the fault at its origin. + */ +public class ViewApiException extends CodedApiException { + + private static final String ERROR_CODE_REQUIRED = + "ViewApiException requires a non-null ViewErrorCode: it selects the response status"; + + private final ViewErrorCode errorCode; + + public ViewApiException(ViewErrorCode errorCode, String message) { + super(message); + this.errorCode = Objects.requireNonNull(errorCode, ERROR_CODE_REQUIRED); + } + + public ViewApiException(ViewErrorCode errorCode, String message, Throwable cause) { + super(message, cause); + this.errorCode = Objects.requireNonNull(errorCode, ERROR_CODE_REQUIRED); + } + + public ViewErrorCode getErrorCode() { + return errorCode; + } + + @Override + public HttpStatus getHttpStatus() { + return errorCode.getHttpStatus(); + } +} diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/exception/ViewErrorCode.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/exception/ViewErrorCode.java new file mode 100644 index 000000000..c44648e44 --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/exception/ViewErrorCode.java @@ -0,0 +1,33 @@ +package com.linkedin.openhouse.tables.exception; + +import lombok.AllArgsConstructor; +import lombok.Getter; +import org.springframework.http.HttpStatus; + +/** + * Internal taxonomy of view failure modes. This enum is never serialized to the wire: it exists + * only to select the HTTP status of the response, and the error body shape stays unchanged. + * + *

The full set is declared up front, including codes M1 never emits, so later milestones (view + * admission, dependency analysis) add behavior without a breaking change to this enum. + */ +@AllArgsConstructor +@Getter +public enum ViewErrorCode { + NO_SUCH_VIEW(HttpStatus.NOT_FOUND), + VIEW_ALREADY_EXISTS(HttpStatus.CONFLICT), + NAME_ALREADY_EXISTS_AS_TABLE(HttpStatus.CONFLICT), + CONCURRENT_VIEW_MODIFICATION(HttpStatus.CONFLICT), + DATABASE_NOT_FOUND(HttpStatus.NOT_FOUND), + VIEWS_DISABLED(HttpStatus.NOT_FOUND), + INVALID_VIEW_DEFINITION(HttpStatus.BAD_REQUEST), + UNSUPPORTED_VIEW_DIALECT(HttpStatus.BAD_REQUEST), + UNSUPPORTED_VIEW_SCHEMA(HttpStatus.BAD_REQUEST), + VIEW_ADMISSION_FAILED(HttpStatus.UNPROCESSABLE_ENTITY), + REQUIRED_REPRESENTATION_MISSING(HttpStatus.UNPROCESSABLE_ENTITY), + DEPENDENCY_CYCLE(HttpStatus.UNPROCESSABLE_ENTITY), + MAX_VIEW_DEPTH_EXCEEDED(HttpStatus.UNPROCESSABLE_ENTITY), + ADMISSION_SERVICE_UNAVAILABLE(HttpStatus.SERVICE_UNAVAILABLE); + + private final HttpStatus httpStatus; +} diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/exception/ViewRequestValidationFailureException.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/exception/ViewRequestValidationFailureException.java new file mode 100644 index 000000000..2b2355136 --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/exception/ViewRequestValidationFailureException.java @@ -0,0 +1,35 @@ +package com.linkedin.openhouse.tables.exception; + +import java.util.List; +import java.util.Objects; + +/** + * Structural validation failure of a /v1 views request. Accumulated reasons are joined with {@code + * "; "} exactly like {@link + * com.linkedin.openhouse.common.exception.RequestValidationFailureException} does for tables, so + * the two APIs report multiple failures identically. + * + *

Only 400-mapped codes are accepted, and that is expressed in the type: the constructors take a + * {@link ViewValidationErrorCode}, which can only name one of the three {@code BAD_REQUEST} codes. + * A validation failure that is not a bad request is therefore not expressible, rather than + * representable and rejected at runtime. {@link #getErrorCode()} still reports the corresponding + * {@link ViewErrorCode}, so status selection is unchanged. + */ +public final class ViewRequestValidationFailureException extends ViewApiException { + + private static final String ERROR_CODE_REQUIRED = + "ViewRequestValidationFailureException requires a non-null ViewValidationErrorCode"; + + public ViewRequestValidationFailureException( + ViewValidationErrorCode errorCode, List reasons) { + super(viewErrorCode(errorCode), String.join("; ", reasons)); + } + + public ViewRequestValidationFailureException(ViewValidationErrorCode errorCode, String message) { + super(viewErrorCode(errorCode), message); + } + + private static ViewErrorCode viewErrorCode(ViewValidationErrorCode errorCode) { + return Objects.requireNonNull(errorCode, ERROR_CODE_REQUIRED).getViewErrorCode(); + } +} diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/exception/ViewValidationErrorCode.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/exception/ViewValidationErrorCode.java new file mode 100644 index 000000000..2b0b54298 --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/exception/ViewValidationErrorCode.java @@ -0,0 +1,28 @@ +package com.linkedin.openhouse.tables.exception; + +import lombok.AllArgsConstructor; +import lombok.Getter; + +/** + * The subset of {@link ViewErrorCode} a structural validation failure is allowed to carry: exactly + * the three codes that map to {@link org.springframework.http.HttpStatus#BAD_REQUEST}. + * + *

This exists so {@link ViewRequestValidationFailureException} can take a type that cannot hold + * a non-400 code, rather than accepting the full taxonomy and rejecting the illegal ones at + * runtime. A validation failure carrying, say, {@code NO_SUCH_VIEW} is a programming error, and the + * compiler is a better place to catch it than a constructor guard. + * + *

{@link ViewErrorCode} keeps all of its values and its status mapping: this enum narrows what a + * validator may throw, it does not narrow the taxonomy itself. Every constant here must name a + * {@code ViewErrorCode} whose status is {@code BAD_REQUEST}, which {@code ViewApiExceptionTest} + * asserts. + */ +@AllArgsConstructor +@Getter +public enum ViewValidationErrorCode { + INVALID_VIEW_DEFINITION(ViewErrorCode.INVALID_VIEW_DEFINITION), + UNSUPPORTED_VIEW_DIALECT(ViewErrorCode.UNSUPPORTED_VIEW_DIALECT), + UNSUPPORTED_VIEW_SCHEMA(ViewErrorCode.UNSUPPORTED_VIEW_SCHEMA); + + private final ViewErrorCode viewErrorCode; +} diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/model/ViewDto.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/model/ViewDto.java new file mode 100644 index 000000000..8931d9e9b --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/model/ViewDto.java @@ -0,0 +1,67 @@ +package com.linkedin.openhouse.tables.model; + +import com.linkedin.openhouse.tables.api.spec.v0.request.components.ViewRepresentation; +import java.util.List; +import java.util.Map; +import lombok.AccessLevel; +import lombok.AllArgsConstructor; +import lombok.Builder; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.NoArgsConstructor; + +/** + * Internal representation of a view as it moves between the API handler and the view service. + * + *

Deliberately not a JPA entity: it carries no {@code @Entity}, no {@code @IdClass} and + * no primary-key companion class, because view persistence does not exist yet and M2 must not + * retrofit this DTO as a separate persisted namespace. It likewise carries no UUID and no {@code + * TableType} — views are not a table variant. + * + *

Fields split into two groups. The pointer group ({@code viewUri}, {@code metadataLocation}, + * {@code viewVersion}, {@code creationTime}, {@code lastModifiedTime}, {@code viewCreator}) is what + * a read returns. The definition group ({@code schema}, {@code representations}, {@code + * sourceDialect}, {@code defaultCatalog}, {@code defaultNamespace}, {@code viewProperties}) is + * write-only input today and is not carried into any response this milestone returns. + */ +@Builder(toBuilder = true) +@Getter +@EqualsAndHashCode +@NoArgsConstructor(access = AccessLevel.PROTECTED) +@AllArgsConstructor(access = AccessLevel.PROTECTED) +public class ViewDto { + + private String viewId; + + private String databaseId; + + private String clusterId; + + private String viewUri; + + private String metadataLocation; + + /** + * On a read this is the view's current version pointer. On a write it carries the caller's + * supplied {@code baseViewVersion} so the service can compare it later. + */ + private String viewVersion; + + private String viewCreator; + + private long creationTime; + + private long lastModifiedTime; + + private String schema; + + private List representations; + + private String sourceDialect; + + private String defaultCatalog; + + private List defaultNamespace; + + private Map viewProperties; +} diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/services/ViewsDisabledService.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/services/ViewsDisabledService.java new file mode 100644 index 000000000..872730a0e --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/services/ViewsDisabledService.java @@ -0,0 +1,57 @@ +package com.linkedin.openhouse.tables.services; + +import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateViewRequestBody; +import com.linkedin.openhouse.tables.exception.ViewApiException; +import com.linkedin.openhouse.tables.exception.ViewErrorCode; +import com.linkedin.openhouse.tables.model.ViewDto; +import org.springframework.data.domain.Page; +import org.springframework.data.util.Pair; +import org.springframework.stereotype.Component; + +/** + * The only {@link ViewsService} bean today. View business logic is intentionally out of scope for + * this API-only increment, so every operation reports that views are disabled. + * + *

It throws a {@link ViewApiException} carrying {@link ViewErrorCode#VIEWS_DISABLED} rather than + * an {@code UnsupportedOperationException}: the finalized design specifies 404 {@code + * VIEWS_DISABLED} for a database without views enabled, and an unchecked non-coded exception would + * instead surface as a generic 500 with a stack trace. A structurally valid view request therefore + * gets the designed disabled response, not an error probe. + * + *

The later real service replaces this bean and implements the per-database gate. + */ +@Component +public class ViewsDisabledService implements ViewsService { + + /** + * Fixed and redacted. The message is copied into the error body and into service audit events, so + * it must never echo request content. + */ + static final String VIEWS_DISABLED_MESSAGE = "Views are disabled"; + + @Override + public ViewDto getView(String databaseId, String viewId, String actingPrincipal) { + throw viewsDisabled(); + } + + @Override + public Page getAllViews( + String databaseId, int page, int size, String sortBy, String actingPrincipal) { + throw viewsDisabled(); + } + + @Override + public Pair putView( + CreateUpdateViewRequestBody requestBody, String actingPrincipal, boolean failOnExist) { + throw viewsDisabled(); + } + + @Override + public void deleteView(String databaseId, String viewId, String actingPrincipal) { + throw viewsDisabled(); + } + + private ViewApiException viewsDisabled() { + return new ViewApiException(ViewErrorCode.VIEWS_DISABLED, VIEWS_DISABLED_MESSAGE); + } +} diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/services/ViewsService.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/services/ViewsService.java new file mode 100644 index 000000000..36ef66e8e --- /dev/null +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/services/ViewsService.java @@ -0,0 +1,55 @@ +package com.linkedin.openhouse.tables.services; + +import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateViewRequestBody; +import com.linkedin.openhouse.tables.model.ViewDto; +import org.springframework.data.domain.Page; +import org.springframework.data.util.Pair; + +/** Service interface backing the /v1 views endpoints. */ +public interface ViewsService { + + /** + * Given a databaseId and viewId, prepare a {@link ViewDto} if actingPrincipal has the right + * privilege. + * + * @param databaseId database identifier + * @param viewId view identifier + * @param actingPrincipal authenticated user + * @return the view pointer + */ + ViewDto getView(String databaseId, String viewId, String actingPrincipal); + + /** + * Given a databaseId, prepare a page of identifier-only {@link ViewDto}s. + * + * @param databaseId database identifier + * @param page zero-based page index + * @param size page size + * @param sortBy optional single sort field + * @param actingPrincipal authenticated user + * @return a page of identifier-only dtos + */ + Page getAllViews( + String databaseId, int page, int size, String sortBy, String actingPrincipal); + + /** + * Create or replace a view. + * + * @param requestBody the create/update request + * @param actingPrincipal authenticated user performing the write + * @param failOnExist true for POST create, false for PUT create-or-replace + * @return a pair whose first element is the saved view and whose second element is true iff the + * call created the view rather than replacing it + */ + Pair putView( + CreateUpdateViewRequestBody requestBody, String actingPrincipal, boolean failOnExist); + + /** + * Delete the view identified by databaseId and viewId if actingPrincipal has the right privilege. + * + * @param databaseId database identifier + * @param viewId view identifier + * @param actingPrincipal authenticated user + */ + void deleteView(String databaseId, String viewId, String actingPrincipal); +} diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/api/spec/ViewApiContractTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/api/spec/ViewApiContractTest.java new file mode 100644 index 000000000..d13ce7278 --- /dev/null +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/api/spec/ViewApiContractTest.java @@ -0,0 +1,466 @@ +package com.linkedin.openhouse.tables.api.spec; + +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.INITIAL_TABLE_VERSION; + +import com.fasterxml.jackson.databind.BeanDescription; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.introspect.BeanPropertyDefinition; +import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateViewRequestBody; +import com.linkedin.openhouse.tables.api.spec.v0.request.components.ViewRepresentation; +import com.linkedin.openhouse.tables.api.spec.v0.response.GetAllViewsResponseBody; +import com.linkedin.openhouse.tables.api.spec.v0.response.GetViewResponseBody; +import com.linkedin.openhouse.tables.exception.ViewErrorCode; +import com.linkedin.openhouse.tables.model.ViewModelConstants; +import java.lang.reflect.Field; +import java.lang.reflect.Modifier; +import java.util.Arrays; +import java.util.EnumMap; +import java.util.Iterator; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.springframework.http.HttpStatus; + +/** + * Freezes the M1 wire surface of {@code /v1/databases/{databaseId}/views}. + * + *

This test exists to satisfy the BDP-108397 acceptance criterion: "A contract test pins the M1 + * wire surface, so adding the admission service, the polymorphic lookup or /versions later changes + * no field this ships." Every assertion is an exact set equality, so the test fails when a field is + * added as well as when one is removed. + * + *

It deliberately runs as a plain JUnit 5 test with reflection and a bare Jackson {@link + * ObjectMapper}: no Spring context is loaded, so the contract stays pinned even if application + * wiring changes. + */ +public class ViewApiContractTest { + + /** + * Bare mapper with default configuration. Assertions run on the Jackson path because Jackson is + * what actually serializes responses over the wire; Gson {@code toJson()} on these models is a + * convenience helper only. + * + *

TODO: this is a bare mapper, not the Spring MVC message converter. {@code + * TablesMvcConfigurer} customizes no converters today, so the two are equivalent, but a + * MockMvc-path serialization assertion belongs in the views controller-test slice to keep that + * equivalence honest. + */ + private static final ObjectMapper MAPPER = new ObjectMapper(); + + @Test + public void testCreateUpdateViewRequestBodyFieldsAreFrozen() { + Set expected = + setOf( + "viewId", + "databaseId", + "clusterId", + "schema", + "representations", + "sourceDialect", + "defaultCatalog", + "defaultNamespace", + "viewProperties", + "baseViewVersion"); + + Assertions.assertEquals( + expected, + contractFieldNames(CreateUpdateViewRequestBody.class), + "CreateUpdateViewRequestBody is a frozen M1 request contract; adding or removing a field is" + + " a wire-visible change that needs an explicit contract review."); + + Assertions.assertEquals( + expected, + jacksonPropertyNames(CreateUpdateViewRequestBody.class), + "The Jackson-visible property set is the true wire surface. It must not drift from the" + + " declared fields, which a computed or inherited getter would silently do."); + } + + @Test + public void testViewRepresentationFieldsAreFrozen() { + Set expected = setOf("type", "sql", "dialect"); + + Assertions.assertEquals( + expected, + contractFieldNames(ViewRepresentation.class), + "ViewRepresentation is a frozen M1 request component."); + + Assertions.assertEquals( + expected, + jacksonPropertyNames(ViewRepresentation.class), + "The Jackson-visible property set is the true wire surface for the nested component."); + } + + @Test + public void testGetViewResponseBodyFieldsAreFrozen() { + Set expected = + setOf( + "viewId", + "databaseId", + "clusterId", + "viewUri", + "metadataLocation", + "viewVersion", + "creationTime"); + + Assertions.assertEquals( + expected, + contractFieldNames(GetViewResponseBody.class), + "GetViewResponseBody is pointer-only. SQL, schema, representations, history, UUID," + + " properties and resolution context must stay in the metadata file."); + + Assertions.assertEquals( + expected, + jacksonPropertyNames(GetViewResponseBody.class), + "A getter-only property would leak onto the wire without adding a declared field, so the" + + " Jackson property set is pinned as well."); + } + + @Test + public void testGetAllViewsResponseBodyFieldsAreFrozen() { + Assertions.assertEquals( + setOf("pageResults"), + contractFieldNames(GetAllViewsResponseBody.class), + "GetAllViewsResponseBody is paginated from the first release; there is deliberately no" + + " unpaginated legacy 'results' field."); + } + + @Test + public void testFullyPopulatedRequestSerializesExactKeys() { + JsonNode json = MAPPER.valueToTree(ViewModelConstants.fullyPopulatedRequest()); + + Assertions.assertEquals( + setOf( + "viewId", + "databaseId", + "clusterId", + "schema", + "representations", + "sourceDialect", + "defaultCatalog", + "defaultNamespace", + "viewProperties", + "baseViewVersion"), + keysOf(json)); + + Assertions.assertEquals(ViewModelConstants.VIEW_ID, json.get("viewId").asText()); + Assertions.assertEquals(ViewModelConstants.DATABASE_ID, json.get("databaseId").asText()); + Assertions.assertEquals(ViewModelConstants.CLUSTER_ID, json.get("clusterId").asText()); + Assertions.assertEquals(ViewModelConstants.SOURCE_DIALECT, json.get("sourceDialect").asText()); + Assertions.assertEquals( + ViewModelConstants.METADATA_LOCATION, json.get("baseViewVersion").asText()); + + Assertions.assertTrue(json.get("representations").isArray()); + Assertions.assertEquals(1, json.get("representations").size()); + JsonNode representation = json.get("representations").get(0); + Assertions.assertEquals(setOf("type", "sql", "dialect"), keysOf(representation)); + Assertions.assertEquals( + ViewModelConstants.SQL_REPRESENTATION_TYPE, representation.get("type").asText()); + Assertions.assertEquals(ViewModelConstants.VIEW_SQL, representation.get("sql").asText()); + Assertions.assertEquals( + ViewModelConstants.SOURCE_DIALECT, representation.get("dialect").asText()); + + Assertions.assertTrue(json.get("defaultNamespace").isArray()); + Assertions.assertEquals( + ViewModelConstants.DATABASE_ID, json.get("defaultNamespace").get(0).asText()); + Assertions.assertEquals( + setOf("owner"), keysOf(json.get("viewProperties")), "viewProperties is a free-form map"); + } + + @Test + public void testCreateRequestOmitsNullBaseViewVersion() { + CreateUpdateViewRequestBody request = ViewModelConstants.createRequestWithoutBaseVersion(); + Assertions.assertNull(request.getBaseViewVersion()); + + JsonNode json = MAPPER.valueToTree(request); + + Assertions.assertFalse( + json.has("baseViewVersion"), + "An omitted baseViewVersion must be absent from the payload, not present as JSON null," + + " so the server can distinguish 'not supplied' on create."); + Assertions.assertEquals( + setOf( + "viewId", + "databaseId", + "clusterId", + "schema", + "representations", + "sourceDialect", + "defaultCatalog", + "defaultNamespace", + "viewProperties"), + keysOf(json)); + + // The Gson helper on the model is configured to agree with @JsonInclude(NON_NULL): unlike + // CreateUpdateTableRequestBody, it does not call serializeNulls(). + Assertions.assertFalse( + request.toJson().contains("baseViewVersion"), + "toJson() must not disagree with the Jackson wire representation."); + } + + @Test + public void testCreateRequestSerializesInitialBaseViewVersion() { + CreateUpdateViewRequestBody request = ViewModelConstants.createRequestWithInitialBaseVersion(); + + JsonNode json = MAPPER.valueToTree(request); + + Assertions.assertTrue(json.has("baseViewVersion")); + Assertions.assertEquals("INITIAL_VERSION", json.get("baseViewVersion").asText()); + Assertions.assertEquals( + INITIAL_TABLE_VERSION, + json.get("baseViewVersion").asText(), + "The create token reuses the existing INITIAL_VERSION literal rather than minting a" + + " view-specific value."); + } + + @Test + public void testPointerResponseSerializesExactKeysAndNoDefinition() { + // Uses distinct metadataLocation/viewVersion sentinels so a swap of the two Jackson property + // associations cannot pass. Production keeps them equal; see ViewModelConstants. + JsonNode json = MAPPER.valueToTree(ViewModelConstants.pointerResponseWithDistinctPointers()); + + Assertions.assertEquals( + setOf( + "viewId", + "databaseId", + "clusterId", + "viewUri", + "metadataLocation", + "viewVersion", + "creationTime"), + keysOf(json)); + + List definitionFields = + Arrays.asList( + "sql", + "schema", + "representations", + "sourceDialect", + "defaultCatalog", + "defaultNamespace", + "viewProperties", + "viewUUID", + "history", + "versions", + "properties", + "tableType"); + for (String forbidden : definitionFields) { + Assertions.assertFalse( + json.has(forbidden), "Pointer response leaked definition field '" + forbidden + "'."); + } + + Assertions.assertEquals(ViewModelConstants.VIEW_URI, json.get("viewUri").asText()); + Assertions.assertEquals( + ViewModelConstants.DISTINCT_METADATA_LOCATION, json.get("metadataLocation").asText()); + Assertions.assertEquals( + ViewModelConstants.DISTINCT_VIEW_VERSION, json.get("viewVersion").asText()); + Assertions.assertNotEquals( + json.get("metadataLocation").asText(), + json.get("viewVersion").asText(), + "The fixture must keep the two pointers distinct, otherwise this test cannot detect a" + + " swapped property association."); + Assertions.assertTrue(json.get("creationTime").isNumber()); + Assertions.assertEquals(ViewModelConstants.CREATION_TIME, json.get("creationTime").asLong()); + } + + @Test + public void testSparseListResponseUsesGetViewResponseBodyElementsAndPageMetadata() { + GetAllViewsResponseBody listResponse = ViewModelConstants.listResponse(); + + Assertions.assertTrue( + listResponse.getPageResults().getContent().stream() + .allMatch(element -> element instanceof GetViewResponseBody), + "List elements are the full response type populated sparsely, not a separate identifier" + + " response type."); + + JsonNode json = MAPPER.valueToTree(listResponse); + Assertions.assertEquals(setOf("pageResults"), keysOf(json)); + + JsonNode page = json.get("pageResults"); + + // Exact key-set equality, not has(): GetAllTablesResponseBody already ships a Spring Data Page + // on the wire today, so this documents the real shipped shape. A Spring Data upgrade that adds, + // removes or renames a page-level key is a client-visible wire change and must be reviewed + // here rather than silently absorbed. + Assertions.assertEquals( + setOf( + "content", + "pageable", + "totalPages", + "totalElements", + "last", + "sort", + "number", + "size", + "numberOfElements", + "first", + "empty"), + keysOf(page), + "The serialized Page shape is part of the view list contract."); + + Assertions.assertEquals(1, page.get("totalPages").asInt()); + Assertions.assertEquals(2L, page.get("totalElements").asLong()); + Assertions.assertEquals(2, page.get("numberOfElements").asInt()); + Assertions.assertEquals(0, page.get("number").asInt()); + Assertions.assertEquals(50, page.get("size").asInt()); + Assertions.assertTrue(page.get("first").asBoolean()); + Assertions.assertTrue(page.get("last").asBoolean()); + Assertions.assertFalse(page.get("empty").asBoolean()); + + Assertions.assertEquals( + setOf("empty", "sorted", "unsorted"), + keysOf(page.get("sort")), + "The nested sort descriptor is client-visible too."); + Assertions.assertTrue(page.get("sort").get("empty").asBoolean()); + Assertions.assertFalse(page.get("sort").get("sorted").asBoolean()); + Assertions.assertTrue(page.get("sort").get("unsorted").asBoolean()); + + JsonNode pageable = page.get("pageable"); + Assertions.assertEquals( + setOf("sort", "offset", "pageNumber", "pageSize", "paged", "unpaged"), + keysOf(pageable), + "The nested pageable descriptor is client-visible too."); + Assertions.assertEquals(0L, pageable.get("offset").asLong()); + Assertions.assertEquals(0, pageable.get("pageNumber").asInt()); + Assertions.assertEquals(50, pageable.get("pageSize").asInt()); + Assertions.assertTrue(pageable.get("paged").asBoolean()); + Assertions.assertFalse(pageable.get("unpaged").asBoolean()); + Assertions.assertEquals(setOf("empty", "sorted", "unsorted"), keysOf(pageable.get("sort"))); + + JsonNode content = page.get("content"); + Assertions.assertEquals(2, content.size()); + for (JsonNode element : content) { + Assertions.assertEquals( + setOf( + "viewId", + "databaseId", + "clusterId", + "viewUri", + "metadataLocation", + "viewVersion", + "creationTime"), + keysOf(element), + "List elements must expose exactly the pointer contract."); + Assertions.assertFalse(element.get("viewId").isNull()); + Assertions.assertEquals(ViewModelConstants.DATABASE_ID, element.get("databaseId").asText()); + List unpopulatedPointerFields = + Arrays.asList("clusterId", "viewUri", "metadataLocation", "viewVersion"); + for (String unpopulated : unpopulatedPointerFields) { + Assertions.assertTrue( + element.get(unpopulated).isNull(), + "List results are identifier-only, so '" + unpopulated + "' must stay unpopulated."); + } + Assertions.assertEquals(0L, element.get("creationTime").asLong()); + } + } + + @Test + public void testViewErrorCodeNamesAndStatusesAreFrozen() { + Map expected = new EnumMap<>(ViewErrorCode.class); + expected.put(ViewErrorCode.NO_SUCH_VIEW, HttpStatus.NOT_FOUND); + expected.put(ViewErrorCode.VIEW_ALREADY_EXISTS, HttpStatus.CONFLICT); + expected.put(ViewErrorCode.NAME_ALREADY_EXISTS_AS_TABLE, HttpStatus.CONFLICT); + expected.put(ViewErrorCode.CONCURRENT_VIEW_MODIFICATION, HttpStatus.CONFLICT); + expected.put(ViewErrorCode.DATABASE_NOT_FOUND, HttpStatus.NOT_FOUND); + expected.put(ViewErrorCode.VIEWS_DISABLED, HttpStatus.NOT_FOUND); + expected.put(ViewErrorCode.INVALID_VIEW_DEFINITION, HttpStatus.BAD_REQUEST); + expected.put(ViewErrorCode.UNSUPPORTED_VIEW_DIALECT, HttpStatus.BAD_REQUEST); + expected.put(ViewErrorCode.UNSUPPORTED_VIEW_SCHEMA, HttpStatus.BAD_REQUEST); + expected.put(ViewErrorCode.VIEW_ADMISSION_FAILED, HttpStatus.UNPROCESSABLE_ENTITY); + expected.put(ViewErrorCode.REQUIRED_REPRESENTATION_MISSING, HttpStatus.UNPROCESSABLE_ENTITY); + expected.put(ViewErrorCode.DEPENDENCY_CYCLE, HttpStatus.UNPROCESSABLE_ENTITY); + expected.put(ViewErrorCode.MAX_VIEW_DEPTH_EXCEEDED, HttpStatus.UNPROCESSABLE_ENTITY); + expected.put(ViewErrorCode.ADMISSION_SERVICE_UNAVAILABLE, HttpStatus.SERVICE_UNAVAILABLE); + + Assertions.assertEquals( + 14, ViewErrorCode.values().length, "ViewErrorCode ships exactly 14 values."); + + Assertions.assertEquals( + setOf( + "NO_SUCH_VIEW", + "VIEW_ALREADY_EXISTS", + "NAME_ALREADY_EXISTS_AS_TABLE", + "CONCURRENT_VIEW_MODIFICATION", + "DATABASE_NOT_FOUND", + "VIEWS_DISABLED", + "INVALID_VIEW_DEFINITION", + "UNSUPPORTED_VIEW_DIALECT", + "UNSUPPORTED_VIEW_SCHEMA", + "VIEW_ADMISSION_FAILED", + "REQUIRED_REPRESENTATION_MISSING", + "DEPENDENCY_CYCLE", + "MAX_VIEW_DEPTH_EXCEEDED", + "ADMISSION_SERVICE_UNAVAILABLE"), + Arrays.stream(ViewErrorCode.values()) + .map(Enum::name) + .collect(Collectors.toCollection(LinkedHashSet::new)), + "Reserved codes ship now so later milestones add behavior without an enum change."); + + Assertions.assertEquals(expected.size(), ViewErrorCode.values().length); + for (ViewErrorCode code : ViewErrorCode.values()) { + Assertions.assertEquals( + expected.get(code), + code.getHttpStatus(), + "ViewErrorCode." + code.name() + " must keep its HTTP status."); + Assertions.assertEquals( + expected.get(code).value(), + code.getHttpStatus().value(), + "ViewErrorCode." + code.name() + " must keep its numeric HTTP status."); + } + + // The enum only selects an HTTP status; it is never serialized into the error body. + Assertions.assertEquals( + setOf("httpStatus"), + contractFieldNames(ViewErrorCode.class), + "ViewErrorCode carries only an HttpStatus. A wire-facing code field would change the" + + " unchanged error response contract."); + } + + /** + * Declared instance fields that form the contract. Static fields (including the enum constants + * themselves and {@code $VALUES}), synthetic fields, and instrumentation artifacts such as + * JaCoCo's {@code $jacocoData} or Lombok-generated members are excluded so the assertion stays + * stable under coverage instrumentation. + */ + private static Set contractFieldNames(Class type) { + return Arrays.stream(type.getDeclaredFields()) + .filter(field -> !field.isSynthetic()) + .filter(field -> !Modifier.isStatic(field.getModifiers())) + .map(Field::getName) + .filter(name -> !name.contains("$")) + .collect(Collectors.toCollection(LinkedHashSet::new)); + } + + /** + * Property names Jackson will actually serialize for the type. Unlike {@link + * #contractFieldNames}, this sees inherited and getter-only (computed) properties, so it pins the + * true wire surface rather than the declared source shape. + */ + private static Set jacksonPropertyNames(Class type) { + BeanDescription description = + MAPPER.getSerializationConfig().introspect(MAPPER.getTypeFactory().constructType(type)); + return description.findProperties().stream() + .map(BeanPropertyDefinition::getName) + .collect(Collectors.toCollection(LinkedHashSet::new)); + } + + private static Set keysOf(JsonNode node) { + Set keys = new LinkedHashSet<>(); + Iterator fieldNames = node.fieldNames(); + while (fieldNames.hasNext()) { + keys.add(fieldNames.next()); + } + return keys; + } + + private static Set setOf(String... values) { + List asList = Arrays.asList(values); + Set set = new LinkedHashSet<>(asList); + Assertions.assertEquals(asList.size(), set.size(), "Duplicate expectation in test fixture."); + return set; + } +} diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/api/validator/ViewSchemaParseBoundaryTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/api/validator/ViewSchemaParseBoundaryTest.java new file mode 100644 index 000000000..16ee0c6e2 --- /dev/null +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/api/validator/ViewSchemaParseBoundaryTest.java @@ -0,0 +1,93 @@ +package com.linkedin.openhouse.tables.api.validator; + +import com.linkedin.openhouse.common.schema.IcebergSchemaHelper; +import com.linkedin.openhouse.tables.model.ViewModelConstants; +import java.io.UncheckedIOException; +import java.util.Arrays; +import java.util.List; +import java.util.stream.Stream; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +/** + * Pins the exception taxonomy {@code OpenHouseViewsApiValidator#validateSchema} depends on. + * + *

That method catches {@link IllegalArgumentException} and {@link UncheckedIOException} rather + * than bare {@code Exception}, so a genuine parser defect propagates as a server fault instead of + * being reported to the caller as a bad request. The narrowing is only correct while those two + * types remain exactly what Iceberg raises for caller-supplied text, and that is a property of a + * third-party library which an upgrade can change silently. + * + *

Iceberg splits the failure in two, and both halves are reachable from a request body: + * + *

    + *
  • Text that Jackson cannot parse as JSON at all fails inside {@code JsonUtil.parse}, which + * wraps the {@code IOException} in an {@link UncheckedIOException}. + *
  • Structurally valid JSON that is not an Iceberg schema fails Iceberg's own semantic checks + * and surfaces as an {@link IllegalArgumentException}. + *
+ * + *

Catching only the second would let a merely malformed document escape and be reported as a + * 500. This test fails the moment that split moves. + * + *

The complementary assertion — that all of these inputs are reported to the caller as a 400 + * carrying the same fixed, redacted schema message — lives in {@code + * ViewsValidatorTest#validateRejectsEverySchemaIcebergCannotParse}, which drives the same three + * fixtures through the validator itself. + */ +public class ViewSchemaParseBoundaryTest { + + /** + * The types {@code validateSchema} catches. Any parse failure not assignable to one of these + * escapes the validator and becomes a 500. + */ + private static final List> CAUGHT_BY_VALIDATOR = + Arrays.asList(IllegalArgumentException.class, UncheckedIOException.class); + + private static Stream unparseableSchemas() { + return Stream.of( + Arguments.of( + "syntactically malformed JSON", + ViewModelConstants.MALFORMED_SCHEMA_LITERAL, + UncheckedIOException.class), + Arguments.of("text that is not JSON at all", "not json at all", UncheckedIOException.class), + Arguments.of( + "Spark StructType JSON", + ViewModelConstants.SPARK_STRUCT_TYPE_SCHEMA_LITERAL, + IllegalArgumentException.class), + Arguments.of( + "duplicate field ids", + ViewModelConstants.DUPLICATE_FIELD_ID_SCHEMA_LITERAL, + IllegalArgumentException.class)); + } + + @ParameterizedTest(name = "{0} is rejected as {2}") + @MethodSource("unparseableSchemas") + public void icebergRejectsEachUnparseableSchemaWithATypeTheValidatorCatches( + String description, String schema, Class expectedType) { + RuntimeException thrown = + Assertions.assertThrows( + RuntimeException.class, + () -> IcebergSchemaHelper.getSchemaFromSchemaJson(schema), + description + " must not parse as an Iceberg schema."); + + Assertions.assertEquals( + expectedType, + thrown.getClass(), + description + + " must keep failing as " + + expectedType.getSimpleName() + + ". If Iceberg changed this, the catch clause in OpenHouseViewsApiValidator" + + ".validateSchema has to change with it."); + + Assertions.assertTrue( + CAUGHT_BY_VALIDATOR.stream().anyMatch(caught -> caught.isInstance(thrown)), + description + + " throws " + + thrown.getClass().getName() + + ", which OpenHouseViewsApiValidator.validateSchema does not catch, so a client" + + " sending it would receive a 500 instead of a 400."); + } +} diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/controller/ViewsControllerPrivilegeTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/controller/ViewsControllerPrivilegeTest.java new file mode 100644 index 000000000..31448dc8a --- /dev/null +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/controller/ViewsControllerPrivilegeTest.java @@ -0,0 +1,86 @@ +package com.linkedin.openhouse.tables.controller; + +import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateViewRequestBody; +import com.linkedin.openhouse.tables.authorization.Privileges; +import java.lang.reflect.Method; +import java.util.Arrays; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.springframework.security.access.annotation.Secured; +import org.springframework.web.bind.annotation.RequestMapping; + +/** + * Pins the privilege each /v1 views route is guarded by. + * + *

{@code @Secured} is enforced by a proxy at runtime, so a route that loses its annotation, or + * has it silently retargeted at the wrong privilege, still compiles and still serves traffic. This + * test freezes the mapping so that drift is a build failure rather than an authorization hole. + * + *

Runs as a plain JUnit 5 reflection test: no Spring context is loaded, so the mapping stays + * pinned independently of how method security happens to be wired. + */ +public class ViewsControllerPrivilegeTest { + + @Test + public void everyViewRouteDeclaresItsExpectedPrivilege() throws NoSuchMethodException { + Map expected = new LinkedHashMap<>(); + expected.put( + ViewsController.class.getMethod("getView", String.class, String.class), + Privileges.Privilege.SELECT); + expected.put( + ViewsController.class.getMethod( + "getAllViews", String.class, int.class, int.class, String.class), + Privileges.Privilege.LIST_VIEW); + expected.put( + ViewsController.class.getMethod( + "createView", String.class, CreateUpdateViewRequestBody.class), + Privileges.Privilege.CREATE_VIEW); + expected.put( + ViewsController.class.getMethod( + "updateView", String.class, String.class, CreateUpdateViewRequestBody.class), + Privileges.Privilege.UPDATE_VIEW_METADATA); + expected.put( + ViewsController.class.getMethod("deleteView", String.class, String.class), + Privileges.Privilege.DELETE_VIEW); + + for (Map.Entry route : expected.entrySet()) { + Secured secured = route.getKey().getAnnotation(Secured.class); + Assertions.assertNotNull( + secured, + "ViewsController." + route.getKey().getName() + " must stay guarded by @Secured."); + Assertions.assertArrayEquals( + new String[] {route.getValue()}, + secured.value(), + "ViewsController." + + route.getKey().getName() + + " must require exactly the " + + route.getValue() + + " privilege."); + } + + Assertions.assertEquals( + expected.keySet().stream().map(Method::getName).collect(Collectors.toSet()), + handlerMethodNames(), + "Every request-mapped method on ViewsController must have its privilege pinned above."); + } + + /** + * Names of the methods Spring MVC would expose as routes. Derived from the mapping annotations + * rather than a hard-coded list, so adding a route without pinning its privilege fails here. + */ + private static Set handlerMethodNames() { + return Arrays.stream(ViewsController.class.getDeclaredMethods()) + .filter( + method -> + Arrays.stream(method.getAnnotations()) + .anyMatch( + annotation -> + annotation.annotationType().isAnnotationPresent(RequestMapping.class))) + .map(Method::getName) + .collect(Collectors.toSet()); + } +} diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/exception/ViewApiExceptionTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/exception/ViewApiExceptionTest.java new file mode 100644 index 000000000..2424266d5 --- /dev/null +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/exception/ViewApiExceptionTest.java @@ -0,0 +1,116 @@ +package com.linkedin.openhouse.tables.exception; + +import java.util.Arrays; +import java.util.Collections; +import java.util.LinkedHashSet; +import java.util.Set; +import java.util.stream.Collectors; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.springframework.http.HttpStatus; + +/** + * Construction-time invariants of the views exception hierarchy. + * + *

Runs as a plain JUnit 5 test: these types hold no Spring wiring, and the point of the class is + * that the failures below happen at the throw site rather than anywhere a context could be + * involved. + */ +public class ViewApiExceptionTest { + + /** + * {@link ViewApiException#getHttpStatus()} dereferences the code, and it is called by {@code + * OpenHouseExceptionHandler} while it is already building the response. A null code accepted at + * construction would therefore surface as a {@code NullPointerException} inside the handler and + * be reported as a generic 500, hiding both the real fault and the status the throw site + * intended. + */ + @Test + public void constructionRejectsANullErrorCodeRatherThanFailingInsideTheHandler() { + NullPointerException fromMessageConstructor = + Assertions.assertThrows( + NullPointerException.class, () -> new ViewApiException(null, "any message")); + Assertions.assertTrue( + fromMessageConstructor.getMessage().contains("ViewErrorCode"), + "The failure must name the missing argument, not read as an anonymous NPE."); + + Assertions.assertThrows( + NullPointerException.class, + () -> new ViewApiException(null, "any message", new RuntimeException("cause")), + "The cause-carrying constructor must apply the same rule."); + + Assertions.assertThrows( + NullPointerException.class, + () -> new ViewRequestValidationFailureException(null, Collections.singletonList("reason")), + "The validation subclass must reject a null code at construction too."); + + Assertions.assertThrows( + NullPointerException.class, + () -> new ViewRequestValidationFailureException(null, "reason")); + } + + /** A well-formed exception reports the code it was given and the status that code maps to. */ + @Test + public void aWellFormedExceptionReportsItsCodeAndStatus() { + ViewApiException exception = new ViewApiException(ViewErrorCode.NO_SUCH_VIEW, "view not found"); + + Assertions.assertEquals(ViewErrorCode.NO_SUCH_VIEW, exception.getErrorCode()); + Assertions.assertEquals(HttpStatus.NOT_FOUND, exception.getHttpStatus()); + Assertions.assertEquals("view not found", exception.getMessage()); + } + + /** + * The reason {@link ViewValidationErrorCode} exists: a validation failure that is not a bad + * request must not be expressible. This freezes the mapping so a later code added to {@link + * ViewErrorCode} cannot drift into or out of the validation subset unnoticed. + */ + @Test + public void everyValidationCodeMapsToABadRequestViewErrorCode() { + Set expected = + setOf("INVALID_VIEW_DEFINITION", "UNSUPPORTED_VIEW_DIALECT", "UNSUPPORTED_VIEW_SCHEMA"); + + Assertions.assertEquals( + expected, + Arrays.stream(ViewValidationErrorCode.values()) + .map(Enum::name) + .collect(Collectors.toCollection(LinkedHashSet::new)), + "ViewValidationErrorCode names exactly the 400-mapped codes."); + + for (ViewValidationErrorCode code : ViewValidationErrorCode.values()) { + Assertions.assertEquals( + code.name(), + code.getViewErrorCode().name(), + "Each validation code must name the identically named ViewErrorCode."); + Assertions.assertEquals( + HttpStatus.BAD_REQUEST, + code.getViewErrorCode().getHttpStatus(), + "ViewValidationErrorCode." + code.name() + " must map to a BAD_REQUEST code."); + } + + Assertions.assertEquals( + expected, + Arrays.stream(ViewErrorCode.values()) + .filter(code -> code.getHttpStatus() == HttpStatus.BAD_REQUEST) + .map(Enum::name) + .collect(Collectors.toCollection(LinkedHashSet::new)), + "The validation subset must stay exhaustive: every BAD_REQUEST ViewErrorCode is" + + " expressible as a validation failure."); + } + + /** Reasons are joined the way the tables API joins them, so both APIs read identically. */ + @Test + public void accumulatedReasonsAreJoinedWithASemicolon() { + ViewRequestValidationFailureException exception = + new ViewRequestValidationFailureException( + ViewValidationErrorCode.UNSUPPORTED_VIEW_DIALECT, + Arrays.asList("first reason", "second reason")); + + Assertions.assertEquals("first reason; second reason", exception.getMessage()); + Assertions.assertEquals(ViewErrorCode.UNSUPPORTED_VIEW_DIALECT, exception.getErrorCode()); + Assertions.assertEquals(HttpStatus.BAD_REQUEST, exception.getHttpStatus()); + } + + private static Set setOf(String... values) { + return new LinkedHashSet<>(Arrays.asList(values)); + } +} diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/MockViewsApiHandler.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/MockViewsApiHandler.java new file mode 100644 index 000000000..e4901265f --- /dev/null +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/MockViewsApiHandler.java @@ -0,0 +1,150 @@ +package com.linkedin.openhouse.tables.mock; + +import com.linkedin.openhouse.common.api.spec.ApiResponse; +import com.linkedin.openhouse.tables.api.handler.ViewsApiHandler; +import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateViewRequestBody; +import com.linkedin.openhouse.tables.api.spec.v0.response.GetAllViewsResponseBody; +import com.linkedin.openhouse.tables.api.spec.v0.response.GetViewResponseBody; +import com.linkedin.openhouse.tables.exception.ViewApiException; +import com.linkedin.openhouse.tables.exception.ViewErrorCode; +import com.linkedin.openhouse.tables.model.ViewModelConstants; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.Map; +import org.springframework.context.annotation.Primary; +import org.springframework.http.HttpStatus; +import org.springframework.security.access.AccessDeniedException; +import org.springframework.security.access.AuthorizationServiceException; +import org.springframework.stereotype.Component; + +/** + * {@code @Primary} stand-in for {@link ViewsApiHandler} used by the views controller tests, + * mirroring {@link MockTablesApiHandler}. It exists so the controller tests exercise routing, + * status codes and response serialization without depending on a view service that does not exist + * yet. + * + *

Error signal: every route first runs the request's {@code databaseId} through a + * deterministic switch, mirroring {@link MockTablesApiHandler}'s {@code "d200"}/{@code "d404"} + * convention. {@link #databaseIdFor(ViewErrorCode)} yields the database id that makes a route throw + * a {@link ViewApiException} carrying that code, and two further ids cover the uncoded paths: an + * {@link AccessDeniedException} and a generic infrastructure {@link AuthorizationServiceException}. + * Any other database id, including the {@code "d200"} the tests use for success, responds normally. + * + *

PUT signal: PUT has two success statuses and the handler picks between them from the + * service's created flag, which does not exist here. The mock therefore uses a deterministic, + * documented identifier signal instead: a PUT for {@link #PUT_CREATES_VIEW_ID} reports 201 CREATED + * and every other view id reports 200 OK. Keep this signal on the view id rather than the database + * id so a later negative-path slice is free to use the database id for error selection, matching + * how {@link MockTablesApiHandler} switches on {@code databaseId}. + */ +@Component +@Primary +public class MockViewsApiHandler implements ViewsApiHandler { + + /** A PUT to this view id reports 201 CREATED; any other view id reports 200 OK. */ + public static final String PUT_CREATES_VIEW_ID = "v201"; + + /** + * Fixed message carried by every thrown {@link ViewApiException}. Deliberately identical across + * codes: the controller contract is status plus message, and the code itself never reaches the + * wire, so tests must not be able to recover it from the body by accident. + */ + public static final String VIEW_FAILURE_MESSAGE = "Mock view handler failure"; + + /** Fixed message for the access-denied path. */ + public static final String ACCESS_DENIED_MESSAGE = "Mock view handler denied access"; + + /** Fixed message for the generic, uncoded infrastructure failure path. */ + public static final String UNAVAILABLE_MESSAGE = "Mock view handler dependency unavailable"; + + /** Database id that makes any route throw {@link AccessDeniedException} (403). */ + public static final String ACCESS_DENIED_DATABASE_ID = "d403"; + + /** + * Database id that makes any route throw {@link AuthorizationServiceException}, the generic + * uncoded infrastructure failure that the shared handler maps to 503. + */ + public static final String UNAVAILABLE_DATABASE_ID = "d503"; + + private static final Map ERROR_CODE_BY_DATABASE_ID; + + static { + Map byDatabaseId = new LinkedHashMap<>(); + for (ViewErrorCode errorCode : ViewErrorCode.values()) { + byDatabaseId.put(databaseIdFor(errorCode), errorCode); + } + ERROR_CODE_BY_DATABASE_ID = Collections.unmodifiableMap(byDatabaseId); + } + + /** + * @return the database id that makes every route throw a {@link ViewApiException} carrying {@code + * errorCode}. Derived from the enum constant so a new code is covered automatically rather + * than needing a hand-maintained switch arm. + */ + public static String databaseIdFor(ViewErrorCode errorCode) { + return "derr_" + errorCode.name(); + } + + private static void throwIfErrorDatabaseId(String databaseId) { + if (ACCESS_DENIED_DATABASE_ID.equals(databaseId)) { + throw new AccessDeniedException(ACCESS_DENIED_MESSAGE); + } + if (UNAVAILABLE_DATABASE_ID.equals(databaseId)) { + throw new AuthorizationServiceException(UNAVAILABLE_MESSAGE); + } + ViewErrorCode errorCode = ERROR_CODE_BY_DATABASE_ID.get(databaseId); + if (errorCode != null) { + throw new ViewApiException(errorCode, VIEW_FAILURE_MESSAGE); + } + } + + @Override + public ApiResponse getView( + String databaseId, String viewId, String actingPrincipal) { + throwIfErrorDatabaseId(databaseId); + return ApiResponse.builder() + .httpStatus(HttpStatus.OK) + .responseBody(ViewModelConstants.pointerResponse()) + .build(); + } + + @Override + public ApiResponse getAllViews( + String databaseId, int page, int size, String sortBy, String actingPrincipal) { + throwIfErrorDatabaseId(databaseId); + return ApiResponse.builder() + .httpStatus(HttpStatus.OK) + .responseBody(ViewModelConstants.listResponse()) + .build(); + } + + @Override + public ApiResponse createView( + String databaseId, CreateUpdateViewRequestBody requestBody, String actingPrincipal) { + throwIfErrorDatabaseId(databaseId); + return ApiResponse.builder() + .httpStatus(HttpStatus.CREATED) + .responseBody(ViewModelConstants.pointerResponse()) + .build(); + } + + @Override + public ApiResponse updateView( + String databaseId, + String viewId, + CreateUpdateViewRequestBody requestBody, + String actingPrincipal) { + throwIfErrorDatabaseId(databaseId); + HttpStatus httpStatus = PUT_CREATES_VIEW_ID.equals(viewId) ? HttpStatus.CREATED : HttpStatus.OK; + return ApiResponse.builder() + .httpStatus(httpStatus) + .responseBody(ViewModelConstants.pointerResponse()) + .build(); + } + + @Override + public ApiResponse deleteView(String databaseId, String viewId, String actingPrincipal) { + throwIfErrorDatabaseId(databaseId); + return ApiResponse.builder().httpStatus(HttpStatus.NO_CONTENT).build(); + } +} diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/api/OpenHouseViewsApiHandlerTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/api/OpenHouseViewsApiHandlerTest.java new file mode 100644 index 000000000..2596c7483 --- /dev/null +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/api/OpenHouseViewsApiHandlerTest.java @@ -0,0 +1,208 @@ +package com.linkedin.openhouse.tables.mock.api; + +import static org.mockito.Mockito.when; + +import com.linkedin.openhouse.cluster.configs.ClusterProperties; +import com.linkedin.openhouse.common.api.spec.ApiResponse; +import com.linkedin.openhouse.tables.api.handler.impl.OpenHouseViewsApiHandler; +import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateViewRequestBody; +import com.linkedin.openhouse.tables.api.spec.v0.response.GetAllViewsResponseBody; +import com.linkedin.openhouse.tables.api.spec.v0.response.GetViewResponseBody; +import com.linkedin.openhouse.tables.api.validator.ViewsApiValidator; +import com.linkedin.openhouse.tables.dto.mapper.ViewsMapper; +import com.linkedin.openhouse.tables.model.ViewDto; +import com.linkedin.openhouse.tables.model.ViewModelConstants; +import com.linkedin.openhouse.tables.services.ViewsService; +import java.util.Collections; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.InOrder; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.PageImpl; +import org.springframework.data.domain.PageRequest; +import org.springframework.data.util.Pair; +import org.springframework.http.HttpStatus; + +/** + * Plain-Mockito coverage of {@link OpenHouseViewsApiHandler}. + * + *

Intentionally no Spring context: {@code MockTablesApplication} does not component-scan {@code + * tables.api.handler.impl}, and the {@code @Primary} mock handler would shadow the real one anyway, + * so a Spring test could not obtain this bean. + */ +@ExtendWith(MockitoExtension.class) +public class OpenHouseViewsApiHandlerTest { + + private static final String SERVING_CLUSTER = "local-cluster"; + private static final String ACTING_PRINCIPAL = "DUMMY_ANONYMOUS_USER"; + + @Mock private ViewsApiValidator viewsApiValidator; + + @Mock private ViewsService viewsService; + + @Mock private ViewsMapper viewsMapper; + + @Mock private ClusterProperties clusterProperties; + + @InjectMocks private OpenHouseViewsApiHandler handler; + + private ViewDto viewDto; + + private GetViewResponseBody responseBody; + + @BeforeEach + public void setup() { + viewDto = + ViewDto.builder() + .viewId(ViewModelConstants.VIEW_ID) + .databaseId(ViewModelConstants.DATABASE_ID) + .build(); + responseBody = ViewModelConstants.pointerResponse(); + } + + @Test + public void getViewValidatesBeforeCallingTheServiceAndReturns200() { + when(viewsService.getView( + ViewModelConstants.DATABASE_ID, ViewModelConstants.VIEW_ID, ACTING_PRINCIPAL)) + .thenReturn(viewDto); + when(viewsMapper.toGetViewResponseBody(viewDto)).thenReturn(responseBody); + + ApiResponse apiResponse = + handler.getView( + ViewModelConstants.DATABASE_ID, ViewModelConstants.VIEW_ID, ACTING_PRINCIPAL); + + InOrder inOrder = Mockito.inOrder(viewsApiValidator, viewsService); + inOrder + .verify(viewsApiValidator) + .validateGetView(ViewModelConstants.DATABASE_ID, ViewModelConstants.VIEW_ID); + inOrder + .verify(viewsService) + .getView(ViewModelConstants.DATABASE_ID, ViewModelConstants.VIEW_ID, ACTING_PRINCIPAL); + + Assertions.assertEquals(HttpStatus.OK, apiResponse.getHttpStatus()); + Assertions.assertSame( + responseBody, + apiResponse.getResponseBody(), + "The handler must forward the mapper's result untouched; it does no serialization or" + + " enrichment of its own."); + } + + @Test + public void getAllViewsPassesThroughTheMappedPageAndReturns200() { + Page servicePage = + new PageImpl<>(Collections.singletonList(viewDto), PageRequest.of(0, 50), 1); + Page mappedPage = ViewModelConstants.sparseListPage(); + + when(viewsService.getAllViews(ViewModelConstants.DATABASE_ID, 0, 50, null, ACTING_PRINCIPAL)) + .thenReturn(servicePage); + when(viewsMapper.toGetViewResponseBodyPage(servicePage)).thenReturn(mappedPage); + + ApiResponse apiResponse = + handler.getAllViews(ViewModelConstants.DATABASE_ID, 0, 50, null, ACTING_PRINCIPAL); + + InOrder inOrder = Mockito.inOrder(viewsApiValidator, viewsService); + inOrder + .verify(viewsApiValidator) + .validateGetAllViews(ViewModelConstants.DATABASE_ID, 0, 50, null); + inOrder + .verify(viewsService) + .getAllViews(ViewModelConstants.DATABASE_ID, 0, 50, null, ACTING_PRINCIPAL); + + Assertions.assertEquals(HttpStatus.OK, apiResponse.getHttpStatus()); + Assertions.assertSame(mappedPage, apiResponse.getResponseBody().getPageResults()); + } + + @Test + public void createViewValidatesAgainstTheServingClusterAndReturns201() { + CreateUpdateViewRequestBody requestBody = ViewModelConstants.createRequestWithoutBaseVersion(); + when(clusterProperties.getClusterName()).thenReturn(SERVING_CLUSTER); + when(viewsService.putView(requestBody, ACTING_PRINCIPAL, true)) + .thenReturn(Pair.of(viewDto, true)); + when(viewsMapper.toGetViewResponseBody(viewDto)).thenReturn(responseBody); + + ApiResponse apiResponse = + handler.createView(ViewModelConstants.DATABASE_ID, requestBody, ACTING_PRINCIPAL); + + InOrder inOrder = Mockito.inOrder(viewsApiValidator, viewsService); + inOrder + .verify(viewsApiValidator) + .validateCreateView(SERVING_CLUSTER, ViewModelConstants.DATABASE_ID, requestBody); + // failOnExist is true on POST: a POST must never silently replace an existing view. + inOrder.verify(viewsService).putView(requestBody, ACTING_PRINCIPAL, true); + + Assertions.assertEquals(HttpStatus.CREATED, apiResponse.getHttpStatus()); + Assertions.assertSame(responseBody, apiResponse.getResponseBody()); + } + + @Test + public void updateViewSelectsStatusFromTheServiceCreatedFlag() { + CreateUpdateViewRequestBody requestBody = ViewModelConstants.fullyPopulatedRequest(); + when(clusterProperties.getClusterName()).thenReturn(SERVING_CLUSTER); + when(viewsMapper.toGetViewResponseBody(viewDto)).thenReturn(responseBody); + + when(viewsService.putView(requestBody, ACTING_PRINCIPAL, false)) + .thenReturn(Pair.of(viewDto, false)); + Assertions.assertEquals( + HttpStatus.OK, + handler + .updateView( + ViewModelConstants.DATABASE_ID, + ViewModelConstants.VIEW_ID, + requestBody, + ACTING_PRINCIPAL) + .getHttpStatus(), + "A PUT that replaced an existing view reports 200."); + + when(viewsService.putView(requestBody, ACTING_PRINCIPAL, false)) + .thenReturn(Pair.of(viewDto, true)); + Assertions.assertEquals( + HttpStatus.CREATED, + handler + .updateView( + ViewModelConstants.DATABASE_ID, + ViewModelConstants.VIEW_ID, + requestBody, + ACTING_PRINCIPAL) + .getHttpStatus(), + "A PUT that created the view reports 201."); + + // Strict alternation proves the validator ran before the service on *each* invocation, not + // merely that both collaborators were touched. failOnExist is false on PUT: a PUT may create. + InOrder inOrder = Mockito.inOrder(viewsApiValidator, viewsService); + for (int invocation = 0; invocation < 2; invocation++) { + inOrder + .verify(viewsApiValidator) + .validateUpdateView( + SERVING_CLUSTER, + ViewModelConstants.DATABASE_ID, + ViewModelConstants.VIEW_ID, + requestBody); + inOrder.verify(viewsService).putView(requestBody, ACTING_PRINCIPAL, false); + } + inOrder.verifyNoMoreInteractions(); + } + + @Test + public void deleteViewValidatesBeforeCallingTheServiceAndReturns204() { + ApiResponse apiResponse = + handler.deleteView( + ViewModelConstants.DATABASE_ID, ViewModelConstants.VIEW_ID, ACTING_PRINCIPAL); + + InOrder inOrder = Mockito.inOrder(viewsApiValidator, viewsService); + inOrder + .verify(viewsApiValidator) + .validateDeleteView(ViewModelConstants.DATABASE_ID, ViewModelConstants.VIEW_ID); + inOrder + .verify(viewsService) + .deleteView(ViewModelConstants.DATABASE_ID, ViewModelConstants.VIEW_ID, ACTING_PRINCIPAL); + + Assertions.assertEquals(HttpStatus.NO_CONTENT, apiResponse.getHttpStatus()); + Assertions.assertNull(apiResponse.getResponseBody()); + } +} diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/api/ViewsValidatorMultiDialectTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/api/ViewsValidatorMultiDialectTest.java new file mode 100644 index 000000000..9e68148e0 --- /dev/null +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/api/ViewsValidatorMultiDialectTest.java @@ -0,0 +1,151 @@ +package com.linkedin.openhouse.tables.mock.api; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; + +import com.linkedin.openhouse.cluster.configs.ClusterProperties; +import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateViewRequestBody; +import com.linkedin.openhouse.tables.api.spec.v0.request.components.ViewRepresentation; +import com.linkedin.openhouse.tables.api.validator.ViewsApiValidator; +import com.linkedin.openhouse.tables.exception.ViewErrorCode; +import com.linkedin.openhouse.tables.exception.ViewRequestValidationFailureException; +import com.linkedin.openhouse.tables.model.ViewModelConstants; +import java.util.Arrays; +import java.util.List; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.function.Executable; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; + +/** + * The half of the dialect rule that a single-dialect deployment cannot demonstrate: what a + * deployment that configures a second dialect actually accepts. + * + *

{@link ViewsValidatorTest} covers the default configuration, where {@code spark} is the only + * supported dialect. That default makes "supported" and "the only one" indistinguishable, so it + * cannot show that the rule is a membership test against configuration rather than a count of the + * list or a literal comparison against Spark. This class configures {@code spark,trino} and asserts + * the difference that makes: a two-representation request is accepted, and every rule that is not + * about the supported set still rejects what it did before. + */ +@SpringBootTest(properties = "cluster.tables.views.supported-dialects=spark,trino") +public class ViewsValidatorMultiDialectTest { + + private static final ViewRepresentation TRINO_REPRESENTATION = + ViewModelConstants.SPARK_REPRESENTATION.toBuilder().dialect("trino").build(); + + @Autowired private ViewsApiValidator viewsApiValidator; + + @Autowired private ClusterProperties clusterProperties; + + @Test + public void validateAcceptsOneRepresentationPerConfiguredDialect() { + assertDoesNotThrow( + createOf( + requestWith( + Arrays.asList(ViewModelConstants.SPARK_REPRESENTATION, TRINO_REPRESENTATION))), + "Both dialects are configured and neither is duplicated, so the request is unambiguous and" + + " must be accepted; the old rule rejected it purely for its length."); + } + + /** The source may name either configured dialect, not just the one that happens to be first. */ + @Test + public void validateAcceptsAConfiguredSourceDialectThatIsNotSpark() { + assertDoesNotThrow( + createOf( + requestWith( + Arrays.asList(ViewModelConstants.SPARK_REPRESENTATION, TRINO_REPRESENTATION)) + .toBuilder() + .sourceDialect("trino") + .build()), + "trino is configured and is supplied as a representation, so it is a legal source dialect."); + + assertDoesNotThrow( + createOf( + requestWith(Arrays.asList(TRINO_REPRESENTATION)) + .toBuilder() + .sourceDialect("trino") + .build()), + "A view defined only in trino is legal once trino is configured."); + } + + /** Widening the set widens it by exactly what was configured, and by nothing else. */ + @Test + public void validateStillRejectsADialectOutsideTheConfiguredSet() { + assertRejected( + createOf( + requestWith( + Arrays.asList( + ViewModelConstants.SPARK_REPRESENTATION, + ViewModelConstants.SPARK_REPRESENTATION + .toBuilder() + .dialect("presto") + .build()))), + ViewErrorCode.UNSUPPORTED_VIEW_DIALECT, + "representations[1].dialect : must be one of the supported dialects: spark, trino"); + + assertRejected( + createOf( + requestWith(Arrays.asList(ViewModelConstants.SPARK_REPRESENTATION)) + .toBuilder() + .sourceDialect("presto") + .build()), + ViewErrorCode.UNSUPPORTED_VIEW_DIALECT, + "sourceDialect : must be one of the supported dialects: spark, trino"); + } + + /** + * Uniqueness is what makes dropping the count rule safe, so it has to keep holding for a dialect + * that only a widened configuration can reach. + */ + @Test + public void validateStillRejectsDuplicateDialectsCaseInsensitively() { + assertRejected( + createOf( + requestWith( + Arrays.asList( + TRINO_REPRESENTATION, + TRINO_REPRESENTATION.toBuilder().dialect("TRINO").build()))), + ViewErrorCode.UNSUPPORTED_VIEW_DIALECT, + "representations : dialects must be unique, duplicated: trino"); + } + + /** Configuring a dialect widens the supported set; it does not relax the exact-lowercase rule. */ + @Test + public void validateStillRejectsAConfiguredDialectSuppliedInTheWrongCase() { + assertRejected( + createOf( + requestWith( + Arrays.asList( + ViewModelConstants.SPARK_REPRESENTATION, + TRINO_REPRESENTATION.toBuilder().dialect("Trino").build()))), + ViewErrorCode.UNSUPPORTED_VIEW_DIALECT, + "representations[1].dialect : must be one of the supported dialects: spark, trino"); + } + + private CreateUpdateViewRequestBody requestWith(List representations) { + return ViewModelConstants.createRequestWithoutBaseVersion() + .toBuilder() + .clusterId(clusterProperties.getClusterName()) + .representations(representations) + .build(); + } + + private Executable createOf(CreateUpdateViewRequestBody requestBody) { + return () -> + viewsApiValidator.validateCreateView( + clusterProperties.getClusterName(), ViewModelConstants.DATABASE_ID, requestBody); + } + + private void assertRejected( + Executable executable, ViewErrorCode expectedCode, String expectedMessage) { + ViewRequestValidationFailureException exception = + Assertions.assertThrows(ViewRequestValidationFailureException.class, executable); + Assertions.assertTrue( + exception.getMessage().contains(expectedMessage), + String.format( + "Expected the failure to report \"%s\" but it reported \"%s\"", + expectedMessage, exception.getMessage())); + Assertions.assertEquals(expectedCode, exception.getErrorCode()); + } +} diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/api/ViewsValidatorTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/api/ViewsValidatorTest.java new file mode 100644 index 000000000..6202ddd36 --- /dev/null +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/api/ViewsValidatorTest.java @@ -0,0 +1,726 @@ +package com.linkedin.openhouse.tables.mock.api; + +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.INITIAL_TABLE_VERSION; +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.MAX_VIEW_IDENTIFIER_LENGTH; +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.MAX_VIEW_SCHEMA_BYTES; +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.MAX_VIEW_SQL_BYTES; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; + +import com.linkedin.openhouse.cluster.configs.ClusterProperties; +import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateViewRequestBody; +import com.linkedin.openhouse.tables.api.spec.v0.request.components.ViewRepresentation; +import com.linkedin.openhouse.tables.api.validator.ViewsApiValidator; +import com.linkedin.openhouse.tables.exception.ViewErrorCode; +import com.linkedin.openhouse.tables.exception.ViewRequestValidationFailureException; +import com.linkedin.openhouse.tables.model.ViewModelConstants; +import java.util.Arrays; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.function.Executable; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; + +/** + * Coverage of {@link ViewsApiValidator}: the requests it must accept, and every structural rule it + * must reject. + * + *

Rejections are asserted on both the exact message and the internal {@link + * ViewErrorCode}. The code never reaches the wire, so this test is the only place its selection is + * observable, and all three 400-mapped codes are otherwise indistinguishable from a client's point + * of view. + * + *

Nothing here parses or interprets SQL: SQL is opaque to the server and the only rule applied + * to it is a size ceiling. + */ +@SpringBootTest +public class ViewsValidatorTest { + + @Autowired private ViewsApiValidator viewsApiValidator; + + @Autowired private ClusterProperties clusterProperties; + + /** + * {@link ViewModelConstants} fixes a literal cluster id so the contract test stays byte-stable, + * but the validator compares against the cluster this server is actually serving. Rebind it here. + */ + private CreateUpdateViewRequestBody servingCluster(CreateUpdateViewRequestBody requestBody) { + return requestBody.toBuilder().clusterId(clusterProperties.getClusterName()).build(); + } + + @Test + public void validateCreateViewAcceptsBothLegalPostTokenForms() { + assertDoesNotThrow( + () -> + viewsApiValidator.validateCreateView( + clusterProperties.getClusterName(), + ViewModelConstants.DATABASE_ID, + servingCluster(ViewModelConstants.createRequestWithoutBaseVersion())), + "A POST that omits baseViewVersion entirely is the plain create shape and must be accepted."); + + assertDoesNotThrow( + () -> + viewsApiValidator.validateCreateView( + clusterProperties.getClusterName(), + ViewModelConstants.DATABASE_ID, + servingCluster(ViewModelConstants.createRequestWithInitialBaseVersion())), + "The Iceberg client sends " + + INITIAL_TABLE_VERSION + + " on create, so that form must be accepted too."); + } + + @Test + public void validateUpdateViewAcceptsAnOpaqueBaseVersionToken() { + CreateUpdateViewRequestBody request = + servingCluster(ViewModelConstants.fullyPopulatedRequest()) + .toBuilder() + // Deliberately not a metadata path: PUT treats the token as fully opaque. + .baseViewVersion("an-entirely-opaque-token") + .build(); + + assertDoesNotThrow( + () -> + viewsApiValidator.validateUpdateView( + clusterProperties.getClusterName(), + ViewModelConstants.DATABASE_ID, + ViewModelConstants.VIEW_ID, + request)); + } + + @Test + public void validateIdentifierAndPagingRoutesAcceptValidInput() { + assertDoesNotThrow( + () -> + viewsApiValidator.validateGetView( + ViewModelConstants.DATABASE_ID, ViewModelConstants.VIEW_ID)); + + assertDoesNotThrow( + () -> + viewsApiValidator.validateDeleteView( + ViewModelConstants.DATABASE_ID, ViewModelConstants.VIEW_ID)); + + assertDoesNotThrow( + () -> viewsApiValidator.validateGetAllViews(ViewModelConstants.DATABASE_ID, 0, 50, null), + "The controller's default paging values must pass unchanged."); + + assertDoesNotThrow( + () -> + viewsApiValidator.validateGetAllViews( + ViewModelConstants.DATABASE_ID, 3, 10000, "viewId"), + "A single sort field and a large page size are both legal: view paging deliberately has no" + + " upper size cap, matching the shared table paging rules."); + } + + @Test + public void validateGetViewAcceptsMaximumLengthIdentifiers() { + String maxLengthId = String.join("", Collections.nCopies(MAX_VIEW_IDENTIFIER_LENGTH, "a")); + Assertions.assertEquals(MAX_VIEW_IDENTIFIER_LENGTH, maxLengthId.length()); + + assertDoesNotThrow( + () -> viewsApiValidator.validateGetView(maxLengthId, maxLengthId), + "The identifier length limit is inclusive, so an identifier of exactly" + + " MAX_VIEW_IDENTIFIER_LENGTH characters must still be accepted."); + } + + // --------------------------------------------------------------------------------------------- + // Negative paths + // --------------------------------------------------------------------------------------------- + + private static final String SCHEMA_PARSE_MESSAGE = + "schema : must be valid Iceberg schema JSON; Spark StructType JSON is not supported"; + + /** + * The messages the default configuration produces. Written out rather than derived from the + * property, so a change to how the supported set is rendered has to be made deliberately here + * too: these strings are copied verbatim into the response body a caller reads. + */ + private static final String UNSUPPORTED_REPRESENTATION_DIALECT = + "representations[0].dialect : must be one of the supported dialects: spark"; + + private static final String UNSUPPORTED_SOURCE_DIALECT = + "sourceDialect : must be one of the supported dialects: spark"; + + /** + * Asserts the request is rejected, that the reported reason is exactly {@code expectedMessage}, + * and that the internal code is the expected one. The code is asserted everywhere because it is + * never serialized: all three 400-mapped codes look identical to a client, so this test class is + * the only place their selection is observable. + */ + private ViewRequestValidationFailureException assertRejected( + Executable executable, ViewErrorCode expectedCode, String expectedMessage) { + ViewRequestValidationFailureException exception = + Assertions.assertThrows(ViewRequestValidationFailureException.class, executable); + Assertions.assertTrue( + exception.getMessage().contains(expectedMessage), + String.format( + "Expected the failure to report \"%s\" but it reported \"%s\"", + expectedMessage, exception.getMessage())); + Assertions.assertEquals( + expectedCode, + exception.getErrorCode(), + "The internal code must be the most specific one the accumulated failures warrant."); + return exception; + } + + /** A request that a POST would accept unchanged: serving cluster, no base version. */ + private CreateUpdateViewRequestBody validCreateRequest() { + return servingCluster(ViewModelConstants.createRequestWithoutBaseVersion()); + } + + /** A request that a PUT would accept unchanged: serving cluster, replace token present. */ + private CreateUpdateViewRequestBody validUpdateRequest() { + return servingCluster(ViewModelConstants.fullyPopulatedRequest()); + } + + private CreateUpdateViewRequestBody createRequestWith(List representations) { + return validCreateRequest().toBuilder().representations(representations).build(); + } + + private CreateUpdateViewRequestBody createRequestWith(ViewRepresentation representation) { + return createRequestWith(Collections.singletonList(representation)); + } + + private static ViewRepresentation sparkRepresentationWithSql(String sql) { + return ViewModelConstants.SPARK_REPRESENTATION.toBuilder().sql(sql).build(); + } + + private Executable createOf(CreateUpdateViewRequestBody requestBody) { + return () -> + viewsApiValidator.validateCreateView( + clusterProperties.getClusterName(), ViewModelConstants.DATABASE_ID, requestBody); + } + + private Executable updateOf(CreateUpdateViewRequestBody requestBody) { + return () -> + viewsApiValidator.validateUpdateView( + clusterProperties.getClusterName(), + ViewModelConstants.DATABASE_ID, + ViewModelConstants.VIEW_ID, + requestBody); + } + + @Test + public void validateRejectsIdentifierMismatchesAgainstPathAndCluster() { + assertRejected( + createOf(validCreateRequest().toBuilder().databaseId("another_database").build()), + ViewErrorCode.INVALID_VIEW_DEFINITION, + String.format( + "databaseId : provided %s, doesn't match with the RequestBody another_database", + ViewModelConstants.DATABASE_ID)); + + assertRejected( + updateOf(validUpdateRequest().toBuilder().viewId("another_view").build()), + ViewErrorCode.INVALID_VIEW_DEFINITION, + String.format( + "viewId : provided %s, doesn't match with the RequestBody another_view", + ViewModelConstants.VIEW_ID)); + + assertRejected( + createOf(validCreateRequest().toBuilder().clusterId("not-the-serving-cluster").build()), + ViewErrorCode.INVALID_VIEW_DEFINITION, + String.format( + "clusterId : provided not-the-serving-cluster, doesn't match with the server cluster %s", + clusterProperties.getClusterName())); + } + + @Test + public void validateRejectsMissingRequiredBodyFields() { + assertRejected( + createOf(validCreateRequest().toBuilder().schema(null).build()), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "CreateUpdateViewRequestBody.schema : schema cannot be empty"); + + assertRejected( + createOf(validCreateRequest().toBuilder().sourceDialect(null).build()), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "CreateUpdateViewRequestBody.sourceDialect : sourceDialect cannot be empty"); + + assertRejected( + createOf(createRequestWith(Collections.emptyList())), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "CreateUpdateViewRequestBody.representations : representations cannot be empty"); + } + + @Test + public void validateGetViewRejectsEmptyAndMalformedIdentifiers() { + assertRejected( + () -> viewsApiValidator.validateGetView("", ViewModelConstants.VIEW_ID), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "databaseId : Cannot be empty"); + + assertRejected( + () -> viewsApiValidator.validateGetView(ViewModelConstants.DATABASE_ID, "bad view!"), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "viewId : provided bad view!, Only alphanumerics and underscore supported"); + + assertRejected( + () -> viewsApiValidator.validateDeleteView(ViewModelConstants.DATABASE_ID, ""), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "viewId : Cannot be empty"); + } + + /** + * Known deviation from the table convention. {@code OpenHouseTablesApiValidator} checks + * only emptiness and the character regex on path identifiers, so this length rule is new + * behaviour rather than a copy of the table rule. It is here because an over-long identifier + * would otherwise pass validation and come back as a misleading 404 rather than a 400 naming the + * actual problem. If the table convention is preferred, removing this rule and this test is the + * whole change. + */ + @Test + public void validateGetViewRejectsOverLongIdentifiersUnlikeTheTableValidator() { + String tooLong = String.join("", Collections.nCopies(MAX_VIEW_IDENTIFIER_LENGTH + 1, "a")); + + ViewRequestValidationFailureException exception = + assertRejected( + () -> viewsApiValidator.validateGetView(ViewModelConstants.DATABASE_ID, tooLong), + ViewErrorCode.INVALID_VIEW_DEFINITION, + String.format( + "viewId : exceeds the maximum length of %d characters", + MAX_VIEW_IDENTIFIER_LENGTH)); + + Assertions.assertFalse( + exception.getMessage().contains(tooLong), + "An over-long identifier is by definition large and the message is copied into the error" + + " body and into audit events, so it must not be echoed back."); + } + + /** + * Malformed JSON, the Spark {@code StructType} shape an engine might send by mistake, and a + * schema with duplicate field ids all fail inside Iceberg's parser and must collapse to the same + * fixed message. The duplicate-id case is why the validator deliberately has no duplicate-id + * check of its own. + */ + @ParameterizedTest + @ValueSource(strings = {"malformed", "sparkStructType", "duplicateFieldIds"}) + public void validateRejectsEverySchemaIcebergCannotParse(String variant) { + String schema; + if ("malformed".equals(variant)) { + schema = ViewModelConstants.MALFORMED_SCHEMA_LITERAL; + } else if ("sparkStructType".equals(variant)) { + schema = ViewModelConstants.SPARK_STRUCT_TYPE_SCHEMA_LITERAL; + } else { + schema = ViewModelConstants.DUPLICATE_FIELD_ID_SCHEMA_LITERAL; + } + + ViewRequestValidationFailureException exception = + assertRejected( + createOf(validCreateRequest().toBuilder().schema(schema).build()), + ViewErrorCode.UNSUPPORTED_VIEW_SCHEMA, + SCHEMA_PARSE_MESSAGE); + + Assertions.assertFalse( + exception.getMessage().contains("fields"), + "Iceberg's own parse message can echo the submitted schema, so the validator must replace" + + " it with a fixed one rather than wrap it."); + } + + @Test + public void validateEnforcesTheSchemaUtf8ByteBoundary() { + assertDoesNotThrow( + createOf( + validCreateRequest() + .toBuilder() + .schema(ViewModelConstants.schemaAtMaxUtf8Size()) + .build()), + "The limit is inclusive: a schema of exactly the maximum size must be accepted."); + + assertRejected( + createOf( + validCreateRequest() + .toBuilder() + .schema(ViewModelConstants.schemaOneByteOverMaxUtf8Size()) + .build()), + ViewErrorCode.UNSUPPORTED_VIEW_SCHEMA, + String.format("schema : exceeds maximum UTF-8 size of %d bytes", MAX_VIEW_SCHEMA_BYTES)); + } + + @Test + public void validateEnforcesTheSqlUtf8ByteBoundary() { + assertDoesNotThrow( + createOf( + createRequestWith(sparkRepresentationWithSql(ViewModelConstants.sqlAtMaxUtf8Size()))), + "The limit is inclusive: SQL of exactly the maximum size must be accepted."); + + assertRejected( + createOf( + createRequestWith( + sparkRepresentationWithSql(ViewModelConstants.sqlOneByteOverMaxUtf8Size()))), + ViewErrorCode.INVALID_VIEW_DEFINITION, + String.format( + "representations[0].sql : exceeds maximum UTF-8 size of %d bytes", MAX_VIEW_SQL_BYTES)); + } + + /** + * The reason the size rules are hand-written instead of declared with {@code @Size}: a bean + * constraint counts UTF-16 characters, so this payload — under the limit in characters, and at + * roughly twice the limit in bytes — would have been accepted. + */ + @Test + public void validateCountsSqlInUtf8BytesNotCharacters() { + String multiByteSql = ViewModelConstants.multiByteSql(MAX_VIEW_SQL_BYTES - 1); + + Assertions.assertTrue( + multiByteSql.length() <= MAX_VIEW_SQL_BYTES, + "Precondition: the fixture must be within the limit when counted as characters."); + + assertRejected( + createOf(createRequestWith(sparkRepresentationWithSql(multiByteSql))), + ViewErrorCode.INVALID_VIEW_DEFINITION, + String.format( + "representations[0].sql : exceeds maximum UTF-8 size of %d bytes", MAX_VIEW_SQL_BYTES)); + } + + /** + * The redaction invariant. Exception messages are copied verbatim into the error response body + * and into service audit events, so a rejection must never carry back the payload that caused it. + */ + @Test + public void rejectionMessagesNeverEchoSqlSchemaOrVersionToken() { + String secretSql = + "SELECT secret_column FROM secret_database.secret_table " + + ViewModelConstants.multiByteSql(MAX_VIEW_SQL_BYTES); + String secretToken = "file:/secret/metadata/00000-secret.metadata.json"; + + CreateUpdateViewRequestBody requestBody = + createRequestWith(sparkRepresentationWithSql(secretSql)) + .toBuilder() + .schema(ViewModelConstants.SPARK_STRUCT_TYPE_SCHEMA_LITERAL) + .baseViewVersion(secretToken) + .build(); + + ViewRequestValidationFailureException exception = + Assertions.assertThrows(ViewRequestValidationFailureException.class, updateOf(requestBody)); + + Assertions.assertTrue( + exception.getMessage().contains("exceeds maximum UTF-8 size"), + "Precondition: this request must actually be rejected for its oversized SQL."); + Assertions.assertFalse( + exception.getMessage().contains("secret_column"), "SQL text must not reach the message."); + Assertions.assertFalse( + exception.getMessage().contains("nullable"), "Schema text must not reach the message."); + Assertions.assertFalse( + exception.getMessage().contains(secretToken), + "The base version token must not reach the message."); + } + + @Test + public void validateRejectsANullRepresentationAndANonSqlType() { + assertRejected( + createOf(createRequestWith(Collections.singletonList((ViewRepresentation) null))), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "representations[0] : cannot be null"); + + assertRejected( + createOf( + createRequestWith( + ViewModelConstants.SPARK_REPRESENTATION.toBuilder().type("SQL").build())), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "representations[0].type : must be 'sql'"); + } + + /** + * The supported set is configuration, and this deployment configures nothing, so the default + * applies. That default is exactly {@code spark}: a Spark view is accepted and a Trino one is + * rejected, which is the behaviour the count-and-literal rules this replaced used to produce. + */ + @Test + public void validateAcceptsTheDefaultConfiguredDialectAndRejectsEveryOther() { + assertDoesNotThrow( + createOf(validCreateRequest()), + "spark is the default supported dialect, so a single spark representation must be accepted."); + + assertRejected( + createOf( + createRequestWith( + ViewModelConstants.SPARK_REPRESENTATION.toBuilder().dialect("trino").build())), + ViewErrorCode.UNSUPPORTED_VIEW_DIALECT, + UNSUPPORTED_REPRESENTATION_DIALECT); + + assertRejected( + createOf( + createRequestWith( + ViewModelConstants.SPARK_REPRESENTATION.toBuilder().dialect("SPARK").build())), + ViewErrorCode.UNSUPPORTED_VIEW_DIALECT, + UNSUPPORTED_REPRESENTATION_DIALECT); + + assertRejected( + createOf(validCreateRequest().toBuilder().sourceDialect("trino").build()), + ViewErrorCode.UNSUPPORTED_VIEW_DIALECT, + UNSUPPORTED_SOURCE_DIALECT); + } + + /** + * Two representations are structurally fine; two representations the deployment cannot both serve + * are not. With only spark configured, the second one is rejected on its own dialect rather than + * on a count of the list. + */ + @Test + public void validateRejectsASecondRepresentationOnlyForItsUnsupportedDialect() { + ViewRequestValidationFailureException exception = + assertRejected( + createOf( + createRequestWith( + Arrays.asList( + ViewModelConstants.SPARK_REPRESENTATION, + ViewModelConstants.SPARK_REPRESENTATION + .toBuilder() + .dialect("trino") + .build()))), + ViewErrorCode.UNSUPPORTED_VIEW_DIALECT, + "representations[1].dialect : must be one of the supported dialects: spark"); + + Assertions.assertFalse( + exception.getMessage().contains("representations[0]"), + "The supported representation must not be implicated: only the unsupported one is at fault."); + } + + /** + * A source dialect naming no supplied representation leaves the view with no definition to read, + * which is a different failure from an unsupported dialect and gets its own message. + */ + @Test + public void validateRejectsASourceDialectThatNamesNoSuppliedRepresentation() { + assertRejected( + createOf( + createRequestWith( + ViewModelConstants.SPARK_REPRESENTATION.toBuilder().dialect(null).build())), + ViewErrorCode.UNSUPPORTED_VIEW_DIALECT, + "sourceDialect : does not name a supplied representation"); + } + + /** Two representations claiming the same engine are ambiguous, whatever their casing. */ + @Test + public void validateRejectsDuplicateDialectsCaseInsensitively() { + assertRejected( + createOf( + createRequestWith( + Arrays.asList( + ViewModelConstants.SPARK_REPRESENTATION, + ViewModelConstants.SPARK_REPRESENTATION.toBuilder().dialect("SPARK").build()))), + ViewErrorCode.UNSUPPORTED_VIEW_DIALECT, + "representations : dialects must be unique, duplicated: spark"); + } + + @Test + public void validateRejectsBlankAndOverLongDefaultCatalog() { + assertRejected( + createOf(validCreateRequest().toBuilder().defaultCatalog(" ").build()), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "defaultCatalog : cannot be blank when provided"); + + assertRejected( + createOf( + validCreateRequest() + .toBuilder() + .defaultCatalog( + String.join("", Collections.nCopies(MAX_VIEW_IDENTIFIER_LENGTH + 1, "c"))) + .build()), + ViewErrorCode.INVALID_VIEW_DEFINITION, + String.format( + "defaultCatalog : exceeds the maximum length of %d characters", + MAX_VIEW_IDENTIFIER_LENGTH)); + + assertDoesNotThrow( + createOf(validCreateRequest().toBuilder().defaultCatalog(null).build()), + "The catalog is optional: omitting it entirely stays legal."); + } + + @Test + public void validateRejectsEmptyAndMalformedDefaultNamespaceSegments() { + assertRejected( + createOf( + validCreateRequest() + .toBuilder() + .defaultNamespace(Collections.emptyList()) + .build()), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "defaultNamespace : cannot be empty when provided"); + + assertRejected( + createOf( + validCreateRequest() + .toBuilder() + .defaultNamespace(Collections.singletonList((String) null)) + .build()), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "defaultNamespace[0] : cannot be blank"); + + assertRejected( + createOf( + validCreateRequest() + .toBuilder() + .defaultNamespace(Arrays.asList(ViewModelConstants.DATABASE_ID, " ")) + .build()), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "defaultNamespace[1] : cannot be blank"); + + assertRejected( + createOf( + validCreateRequest() + .toBuilder() + .defaultNamespace(Collections.singletonList("not a namespace!")) + .build()), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "defaultNamespace[0] : Only alphanumerics and underscore supported"); + + assertRejected( + createOf( + validCreateRequest() + .toBuilder() + .defaultNamespace( + Collections.singletonList( + String.join("", Collections.nCopies(MAX_VIEW_IDENTIFIER_LENGTH + 1, "n")))) + .build()), + ViewErrorCode.INVALID_VIEW_DEFINITION, + String.format( + "defaultNamespace[0] : exceeds the maximum length of %d characters", + MAX_VIEW_IDENTIFIER_LENGTH)); + } + + @Test + public void validateRejectsBlankPropertyKeysAndNullPropertyValues() { + Map blankKey = new LinkedHashMap<>(); + blankKey.put(" ", "value"); + assertRejected( + createOf(validCreateRequest().toBuilder().viewProperties(blankKey).build()), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "viewProperties : property keys cannot be blank"); + + Map nullValues = new LinkedHashMap<>(); + nullValues.put("owner", null); + nullValues.put("team", null); + assertRejected( + createOf(validCreateRequest().toBuilder().viewProperties(nullValues).build()), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "viewProperties : property values cannot be null, keys: owner, team"); + } + + /** + * Reserved-key detection reuses the internal catalog's case-sensitive {@code openhouse.} + * predicate verbatim. The final assertion is the point of the test: because that predicate is + * case-sensitive and {@code policies} is matched exactly, a user property such as {@code + * OpenHouse.myTeam} is not reserved and must keep working. + */ + @Test + public void validateRejectsReservedPropertyKeysCaseSensitively() { + Map openhousePrefixed = new LinkedHashMap<>(); + openhousePrefixed.put("openhouse.tableId", "hijacked"); + assertRejected( + createOf(validCreateRequest().toBuilder().viewProperties(openhousePrefixed).build()), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "viewProperties : reserved keys are not allowed: openhouse.tableId"); + + Map policies = new LinkedHashMap<>(); + policies.put("policies", "hijacked"); + assertRejected( + createOf(validCreateRequest().toBuilder().viewProperties(policies).build()), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "viewProperties : reserved keys are not allowed: policies"); + + Map userOwned = new LinkedHashMap<>(); + userOwned.put("OpenHouse.myTeam", "grid"); + userOwned.put("policies_owner", "grid"); + assertDoesNotThrow( + createOf(validCreateRequest().toBuilder().viewProperties(userOwned).build()), + "Neither of these user-owned keys is reserved: the openhouse. predicate is case-sensitive" + + " and policies is matched exactly."); + } + + @Test + public void validateRejectsIllegalBaseVersionTokensPerVerb() { + assertRejected( + createOf(validCreateRequest().toBuilder().baseViewVersion("some-other-token").build()), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "baseViewVersion : must be omitted or " + INITIAL_TABLE_VERSION + " on POST create"); + + assertRejected( + updateOf(validUpdateRequest().toBuilder().baseViewVersion(null).build()), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "baseViewVersion : is required and cannot be blank on PUT"); + + assertRejected( + updateOf(validUpdateRequest().toBuilder().baseViewVersion(" ").build()), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "baseViewVersion : is required and cannot be blank on PUT"); + } + + @Test + public void validateGetAllViewsRejectsInvalidPagingAndCompositeSort() { + assertRejected( + () -> viewsApiValidator.validateGetAllViews(ViewModelConstants.DATABASE_ID, -1, 50, null), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "page : provided -1, cannot be negative"); + + assertRejected( + () -> viewsApiValidator.validateGetAllViews(ViewModelConstants.DATABASE_ID, 0, 0, null), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "size : provided 0, must be greater than 0"); + + assertRejected( + () -> + viewsApiValidator.validateGetAllViews( + ViewModelConstants.DATABASE_ID, 0, 50, "viewId,databaseId"), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "sortBy : provided viewId,databaseId, does not support multiple sort fields or directions"); + } + + /** Failures accumulate, so a client sees every structural problem in one response. */ + @Test + public void validateReportsEveryFailureRatherThanStoppingAtTheFirst() { + CreateUpdateViewRequestBody requestBody = + validCreateRequest() + .toBuilder() + .databaseId("another_database") + .defaultCatalog(" ") + .baseViewVersion("not-the-initial-token") + .build(); + + ViewRequestValidationFailureException exception = + Assertions.assertThrows(ViewRequestValidationFailureException.class, createOf(requestBody)); + + Assertions.assertTrue(exception.getMessage().contains("databaseId : provided")); + Assertions.assertTrue( + exception.getMessage().contains("defaultCatalog : cannot be blank when provided")); + Assertions.assertTrue(exception.getMessage().contains("baseViewVersion : must be omitted")); + Assertions.assertTrue( + exception.getMessage().contains("; "), + "Reasons are joined with \"; \", matching how the table API reports multiple failures."); + } + + /** + * When a request breaks several rules at once the thrown code is the most specific one: schema + * beats dialect, and dialect beats the generic definition code. All three map to 400, so this + * ordering is invisible on the wire and only assertable here. + */ + @Test + public void errorCodePrecedenceIsSchemaThenDialectThenGeneric() { + CreateUpdateViewRequestBody allThree = + createRequestWith( + ViewModelConstants.SPARK_REPRESENTATION.toBuilder().dialect("trino").build()) + .toBuilder() + .schema(ViewModelConstants.MALFORMED_SCHEMA_LITERAL) + .defaultCatalog(" ") + .build(); + assertRejected(createOf(allThree), ViewErrorCode.UNSUPPORTED_VIEW_SCHEMA, SCHEMA_PARSE_MESSAGE); + + CreateUpdateViewRequestBody dialectAndGeneric = + allThree.toBuilder().schema(ViewModelConstants.VIEW_SCHEMA_LITERAL).build(); + assertRejected( + createOf(dialectAndGeneric), + ViewErrorCode.UNSUPPORTED_VIEW_DIALECT, + "defaultCatalog : cannot be blank when provided"); + + CreateUpdateViewRequestBody genericOnly = + validCreateRequest().toBuilder().defaultCatalog(" ").build(); + assertRejected( + createOf(genericOnly), + ViewErrorCode.INVALID_VIEW_DEFINITION, + "defaultCatalog : cannot be blank when provided"); + } +} diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/audit/ViewRequestPayloadRedactorTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/audit/ViewRequestPayloadRedactorTest.java new file mode 100644 index 000000000..2624df014 --- /dev/null +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/audit/ViewRequestPayloadRedactorTest.java @@ -0,0 +1,167 @@ +package com.linkedin.openhouse.tables.mock.audit; + +import com.google.gson.JsonArray; +import com.google.gson.JsonElement; +import com.google.gson.JsonNull; +import com.google.gson.JsonObject; +import com.google.gson.JsonParser; +import com.google.gson.JsonPrimitive; +import com.linkedin.openhouse.common.audit.ServiceAuditPayloadRedactor; +import com.linkedin.openhouse.tables.audit.ViewRequestPayloadRedactor; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.springframework.mock.web.MockHttpServletRequest; + +/** + * Unit coverage of {@link ViewRequestPayloadRedactor}. The controller-level proof that the redactor + * is actually wired into {@link com.linkedin.openhouse.common.audit.ServiceAuditAspect} lives in + * {@code ViewsControllerTest}; this pins the route scoping and the shape of the rewrite, including + * the payload shapes a caller can send that are not a well-formed view request. + */ +public class ViewRequestPayloadRedactorTest { + + private final ViewRequestPayloadRedactor redactor = new ViewRequestPayloadRedactor(); + + private static MockHttpServletRequest requestFor(String uri) { + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setRequestURI(uri); + return request; + } + + @ParameterizedTest + @ValueSource( + strings = { + "/v1/databases/my_database/views", + "/v1/databases/my_database/views/my_view", + "/v1/databases/d200/views" + }) + public void appliesToTheViewRoutes(String uri) { + Assertions.assertTrue(redactor.appliesTo(requestFor(uri))); + } + + /** + * The scoping that protects the existing resources. A table create carries a {@code schema} too, + * so the redactor must decline every route but views. The {@code /v2} entries pin that the scope + * moved off the prefix views were briefly drafted against, and that the live {@code /v2} table + * search route is untouched. + */ + @ParameterizedTest + @ValueSource( + strings = { + "/v1/databases/my_database/tables", + "/v1/databases/my_database/tables/my_table", + "/v1/databases", + "/v1/databases/my_database/tables/my_table/aclPolicies", + "/v1/databases/my_database/views/my_view/extra", + "/v1/databases/views", + "/v2/databases/my_database/tables/search", + "/v2/databases/my_database/views", + "/v2/databases/my_database/views/my_view" + }) + public void declinesEveryOtherRoute(String uri) { + Assertions.assertFalse(redactor.appliesTo(requestFor(uri))); + } + + @Test + public void redactsSchemaAndEverySqlRepresentation() { + JsonElement payload = + JsonParser.parseString( + "{\"viewId\": \"my_view\", \"databaseId\": \"my_database\"," + + " \"schema\": \"secret schema\"," + + " \"representations\": [" + + "{\"type\": \"sql\", \"sql\": \"secret sql one\", \"dialect\": \"spark\"}," + + "{\"type\": \"sql\", \"sql\": \"secret sql two\", \"dialect\": \"trino\"}]," + + " \"sourceDialect\": \"spark\"}"); + + JsonObject redacted = redactor.redact(payload).getAsJsonObject(); + + Assertions.assertEquals( + ServiceAuditPayloadRedactor.REDACTED_VALUE, redacted.get("schema").getAsString()); + JsonArray representations = redacted.getAsJsonArray("representations"); + for (JsonElement representation : representations) { + Assertions.assertEquals( + ServiceAuditPayloadRedactor.REDACTED_VALUE, + representation.getAsJsonObject().get("sql").getAsString(), + "Every representation is redacted, not only the first."); + } + Assertions.assertFalse(redacted.toString().contains("secret")); + + // Identifiers and dialect metadata survive. + Assertions.assertEquals("my_view", redacted.get("viewId").getAsString()); + Assertions.assertEquals("my_database", redacted.get("databaseId").getAsString()); + Assertions.assertEquals("spark", redacted.get("sourceDialect").getAsString()); + Assertions.assertEquals( + "trino", representations.get(1).getAsJsonObject().get("dialect").getAsString()); + } + + @Test + public void leavesTheArgumentUntouched() { + JsonElement payload = + JsonParser.parseString( + "{\"schema\": \"secret schema\"," + + " \"representations\": [{\"type\": \"sql\", \"sql\": \"secret sql\"}]}"); + String before = payload.toString(); + + redactor.redact(payload); + + Assertions.assertEquals( + before, + payload.toString(), + "Redacting must return a copy so one redactor cannot observe another's rewrite."); + } + + /** + * A request whose body is absent, malformed or simply not shaped like a view request still + * reaches the redactor, because the aspect audits whatever the caller sent. + */ + @Test + public void toleratesPayloadsThatCarryNoViewDefinition() { + Assertions.assertNull(redactor.redact(null)); + Assertions.assertEquals(JsonNull.INSTANCE, redactor.redact(JsonNull.INSTANCE)); + Assertions.assertEquals( + new JsonPrimitive("not an object"), redactor.redact(new JsonPrimitive("not an object"))); + Assertions.assertEquals(new JsonArray(), redactor.redact(new JsonArray())); + + JsonObject withoutDefinition = new JsonObject(); + withoutDefinition.addProperty("viewId", "my_view"); + Assertions.assertEquals( + withoutDefinition, + redactor.redact(withoutDefinition), + "An object carrying neither field is returned as an equal copy."); + } + + /** + * {@code representations} is caller-supplied JSON, so it can be any element. Redaction must skip + * what it cannot interpret rather than fail and lose the whole payload to the aspect's + * fail-closed handler. + */ + @Test + public void skipsRepresentationsThatAreNotSqlObjects() { + JsonElement payload = + JsonParser.parseString( + "{\"schema\": \"secret schema\"," + + " \"representations\": [\"not an object\", {\"type\": \"sql\"}, null]}"); + + JsonObject redacted = redactor.redact(payload).getAsJsonObject(); + + Assertions.assertEquals( + ServiceAuditPayloadRedactor.REDACTED_VALUE, redacted.get("schema").getAsString()); + Assertions.assertEquals(3, redacted.getAsJsonArray("representations").size()); + } + + @Test + public void redactsAnObjectRepresentationsFieldThatIsNotAnArray() { + JsonElement payload = + JsonParser.parseString( + "{\"schema\": \"secret schema\", \"representations\": \"nonsense\"}"); + + JsonObject redacted = redactor.redact(payload).getAsJsonObject(); + + Assertions.assertEquals( + ServiceAuditPayloadRedactor.REDACTED_VALUE, + redacted.get("schema").getAsString(), + "A malformed representations field must not stop the schema from being redacted."); + } +} diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/controller/TablesControllerTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/controller/TablesControllerTest.java index 9f0d25134..c8877956c 100644 --- a/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/controller/TablesControllerTest.java +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/controller/TablesControllerTest.java @@ -1,6 +1,7 @@ package com.linkedin.openhouse.tables.mock.controller; import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.INITIAL_TABLE_VERSION; +import static com.linkedin.openhouse.common.audit.ServiceAuditPayloadRedactor.REDACTED_VALUE; import static com.linkedin.openhouse.tables.e2e.h2.ValidationUtilities.CURRENT_MAJOR_VERSION_PREFIX; import static com.linkedin.openhouse.tables.model.ServiceAuditModelConstants.EXCLUDE_FIELDS; import static com.linkedin.openhouse.tables.model.ServiceAuditModelConstants.SERVICE_AUDIT_EVENT_CREATE_TABLE_FAILED; @@ -21,15 +22,18 @@ import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateTableRequestBody; import com.linkedin.openhouse.tables.api.spec.v0.response.GetTableResponseBody; import com.linkedin.openhouse.tables.controller.TablesController; +import com.linkedin.openhouse.tables.controller.ViewsController; import com.linkedin.openhouse.tables.e2e.h2.ValidationUtilities; import com.linkedin.openhouse.tables.mock.RequestConstants; import com.linkedin.openhouse.tables.mock.properties.AuthorizationPropertiesInitializer; +import com.linkedin.openhouse.tables.model.ViewModelConstants; import java.io.IOException; import java.text.ParseException; import java.util.*; import javax.servlet.Filter; import javax.servlet.http.HttpServletRequest; import org.codehaus.jettison.json.JSONException; +import org.hamcrest.Matchers; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -53,6 +57,16 @@ @ContextConfiguration(initializers = AuthorizationPropertiesInitializer.class) public class TablesControllerTest { + /** + * A table schema carrying a marker that appears nowhere else, so {@link + * #tableCreateAuditPayloadStillCarriesItsSchemaUnredacted()} fails loudly if the value is ever + * scrubbed rather than passing on a coincidence. + */ + private static final String TABLE_SCHEMA_WITH_MARKER = + "{\"type\": \"struct\", \"schema-id\": 0, \"fields\": [" + + "{\"id\": 1, \"required\": true, \"name\": \"table_schema_marker_column\"," + + " \"type\": \"string\"}]}"; + private MockMvc mvc; private MockMvc mvcUnauthenticated; @@ -63,6 +77,8 @@ public class TablesControllerTest { @Autowired private TablesController tablesController; + @Autowired private ViewsController viewsController; + @Autowired private OpenHouseExceptionHandler openHouseExceptionHandler; @Captor private ArgumentCaptor argCaptor; @@ -124,6 +140,116 @@ public void findTableById4xx() throws Exception { .andExpect(status().isNotFound()); } + /** + * Adding {@link ViewsController} must not change how a v1 table request is routed. Views now + * mount under the same {@code /v1/databases/{databaseId}/} prefix as tables and are told apart + * only by the {@code tables} vs {@code views} path segment, so this is the assertion that pins + * that disambiguation. Both controllers are registered in one dispatcher here, which is the only + * arrangement in which the new mappings could shadow or collide with the existing ones, and each + * assertion checks the response body, so it fails if a request reaches the wrong handler rather + * than merely returning a plausible status. + * + *

This partially satisfies the acceptance criterion. It proves the v1 table route is + * unchanged and that the sibling v1 views route reaches the views handler. It cannot prove the + * other half, that a real view's name returns 404 from the table route, because no view is + * persisted in this PR and the server has no way to tell a view's name from a missing table until + * the M2 {@code entityType} discriminator and the table-only read filter land. A test claiming + * that today would pass for the wrong reason. + */ + @Test + public void v1TableAndViewRoutesResolveToTheirOwnHandlers() throws Exception { + MockMvc mvcWithBothControllers = + MockMvcBuilders.standaloneSetup(tablesController, viewsController) + .setControllerAdvice(openHouseExceptionHandler) + .addInterceptors(new DummyTokenInterceptor()) + .addFilter(new CachingRequestBodyFilter()) + .build(); + + // A view-like name on the v1 table route still reaches the tables handler. + mvcWithBothControllers + .perform( + MockMvcRequestBuilders.get( + CURRENT_MAJOR_VERSION_PREFIX + "/databases/d200/tables/my_view") + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)) + .andExpect(status().isOk()) + .andExpect(content().json(RequestConstants.TEST_GET_TABLE_RESPONSE_BODY.toJson())); + + // A table literally named "views" is still a table: the views mapping must not capture it from + // the tableId position under the shared prefix. + mvcWithBothControllers + .perform( + MockMvcRequestBuilders.get( + CURRENT_MAJOR_VERSION_PREFIX + "/databases/d200/tables/views") + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)) + .andExpect(status().isOk()) + .andExpect(content().json(RequestConstants.TEST_GET_TABLE_RESPONSE_BODY.toJson())); + + // The same name under the sibling views segment reaches the views handler instead. + mvcWithBothControllers + .perform( + MockMvcRequestBuilders.get( + CURRENT_MAJOR_VERSION_PREFIX + "/databases/d200/views/my_view") + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)) + .andExpect(status().isOk()) + .andExpect(content().json(ViewModelConstants.pointerResponse().toJson())); + + // The views collection route is likewise the views handler's, not the tables handler's. + mvcWithBothControllers + .perform( + MockMvcRequestBuilders.get(CURRENT_MAJOR_VERSION_PREFIX + "/databases/d200/views") + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.pageResults.content[0].viewId", Matchers.is("my_view"))); + + // The v1 table route is otherwise unchanged, including its not-found behaviour. + mvcWithBothControllers + .perform( + MockMvcRequestBuilders.get( + CURRENT_MAJOR_VERSION_PREFIX + "/databases/d404/tables/my_view") + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)) + .andExpect(status().isNotFound()); + } + + /** + * Regression pin for {@link com.linkedin.openhouse.tables.audit.ViewRequestPayloadRedactor}. + * {@code CreateUpdateTableRequestBody} also carries a {@code schema}, so a redactor keyed on the + * field name alone would silently start scrubbing table audit events, which is not what the views + * change is allowed to do. The view redactor is scoped by request URI instead, and this asserts + * that a v1 table create still audits its schema verbatim. + */ + @Test + public void tableCreateAuditPayloadStillCarriesItsSchemaUnredacted() throws Exception { + CreateUpdateTableRequestBody requestBody = + RequestConstants.TEST_CREATE_TABLE_REQUEST_BODY + .toBuilder() + .schema(TABLE_SCHEMA_WITH_MARKER) + .build(); + + mvc.perform( + MockMvcRequestBuilders.post(CURRENT_MAJOR_VERSION_PREFIX + "/databases/d200/tables") + .contentType(MediaType.APPLICATION_JSON) + .content(requestBody.toJson()) + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)); + + Mockito.verify(serviceAuditHandler, Mockito.atLeastOnce()).audit(argCaptor.capture()); + ServiceAuditEvent actualEvent = argCaptor.getValue(); + JsonObject payload = actualEvent.getRequestPayload().getAsJsonObject(); + + Assertions.assertEquals( + TABLE_SCHEMA_WITH_MARKER, + payload.get("schema").getAsString(), + "A table create must audit its schema exactly as it was sent."); + Assertions.assertFalse( + actualEvent.getRequestPayload().toString().contains(REDACTED_VALUE), + "Nothing in a table request payload may be redacted by the views change."); + } + @Test public void findTableById401() throws Exception { mvc.perform( diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/controller/ViewsControllerTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/controller/ViewsControllerTest.java new file mode 100644 index 000000000..6d3648037 --- /dev/null +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/controller/ViewsControllerTest.java @@ -0,0 +1,613 @@ +package com.linkedin.openhouse.tables.mock.controller; + +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import com.google.gson.JsonArray; +import com.google.gson.JsonElement; +import com.google.gson.JsonObject; +import com.linkedin.openhouse.common.audit.AuditHandler; +import com.linkedin.openhouse.common.audit.CachingRequestBodyFilter; +import com.linkedin.openhouse.common.audit.ServiceAuditPayloadRedactor; +import com.linkedin.openhouse.common.audit.model.ServiceAuditEvent; +import com.linkedin.openhouse.common.exception.handler.OpenHouseExceptionHandler; +import com.linkedin.openhouse.common.security.DummyTokenInterceptor; +import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateViewRequestBody; +import com.linkedin.openhouse.tables.api.spec.v0.request.components.ViewRepresentation; +import com.linkedin.openhouse.tables.audit.ViewRequestPayloadRedactor; +import com.linkedin.openhouse.tables.controller.ViewsController; +import com.linkedin.openhouse.tables.exception.ViewErrorCode; +import com.linkedin.openhouse.tables.mock.MockViewsApiHandler; +import com.linkedin.openhouse.tables.mock.properties.AuthorizationPropertiesInitializer; +import com.linkedin.openhouse.tables.model.ViewModelConstants; +import io.swagger.v3.oas.annotations.responses.ApiResponses; +import java.io.IOException; +import java.lang.reflect.Method; +import java.text.ParseException; +import java.util.Arrays; +import java.util.Collections; +import java.util.Set; +import java.util.TreeSet; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.codehaus.jettison.json.JSONException; +import org.hamcrest.Matchers; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.EnumSource; +import org.junit.jupiter.params.provider.MethodSource; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.Mockito; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.http.MediaType; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder; +import org.springframework.test.web.servlet.request.MockMvcRequestBuilders; +import org.springframework.test.web.servlet.setup.MockMvcBuilders; + +/** + * MockMvc coverage of the five /v1 view routes: success statuses plus every failure status the + * routes can report. + * + *

Error statuses are driven through {@link MockViewsApiHandler}'s database-id switch, so this + * class exercises controller wiring and the shared exception handler rather than validation. The + * validator's own rejections are covered by {@code ViewsValidatorTest}. + * + *

No test here asserts an error code in the response JSON. View error codes are internal + * status selectors: they choose the HTTP status and are never serialized. The assertions are + * therefore status plus the fixed message, and one explicit assertion that no code field leaked. + * + *

Paths are written as literal {@code /v1} strings rather than reusing {@code + * ValidationUtilities.CURRENT_MAJOR_VERSION_PREFIX}. Views share the prefix with tables but are a + * separate resource, so spelling the routes out here keeps this class asserting the exact URIs the + * controller publishes rather than whatever that constant later becomes. + */ +@SpringBootTest +@ContextConfiguration(initializers = AuthorizationPropertiesInitializer.class) +public class ViewsControllerTest { + + private static final String VIEWS_PATH = "/v1/databases/d200/views"; + + private MockMvc mvc; + + /** + * A second MockMvc that raises {@link org.springframework.web.servlet.NoHandlerFoundException} + * for an unmapped path instead of letting the container answer a bare 404. That routes the + * failure through {@link OpenHouseExceptionHandler#handleNoHandlerFoundException}, which is the + * behaviour the deployed application has, and it lets the unresolved-route test assert the real + * "cannot be resolved" response rather than a status that a missing controller would also + * produce. + */ + private MockMvc mvcThrowingOnUnmappedPath; + + private String jwtAccessToken; + + @Autowired private ViewsController viewsController; + + @Autowired private OpenHouseExceptionHandler openHouseExceptionHandler; + + @MockBean private AuditHandler serviceAuditHandler; + + @Captor private ArgumentCaptor argCaptor; + + @BeforeEach + public void setup() throws IOException, JSONException, ParseException { + mvc = + MockMvcBuilders.standaloneSetup(viewsController) + .setControllerAdvice(openHouseExceptionHandler) + .addInterceptors(new DummyTokenInterceptor()) + .addFilter(new CachingRequestBodyFilter()) + .build(); + + mvcThrowingOnUnmappedPath = + MockMvcBuilders.standaloneSetup(viewsController) + .setControllerAdvice(openHouseExceptionHandler) + .addInterceptors(new DummyTokenInterceptor()) + .addFilter(new CachingRequestBodyFilter()) + .addDispatcherServletCustomizer( + dispatcherServlet -> dispatcherServlet.setThrowExceptionIfNoHandlerFound(true)) + .build(); + + DummyTokenInterceptor.DummySecurityJWT dummySecurityJWT = + new DummyTokenInterceptor.DummySecurityJWT("DUMMY_ANONYMOUS_USER"); + jwtAccessToken = dummySecurityJWT.buildNoopJWT(); + } + + @Test + public void getViewReturns200WithPointerBody() throws Exception { + mvc.perform( + MockMvcRequestBuilders.get(VIEWS_PATH + "/my_view") + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)) + .andExpect(status().isOk()) + .andExpect(content().contentType(MediaType.APPLICATION_JSON)) + .andExpect(content().json(ViewModelConstants.pointerResponse().toJson())); + } + + @Test + public void createViewReturns201WithPointerBody() throws Exception { + mvc.perform( + MockMvcRequestBuilders.post(VIEWS_PATH) + .contentType(MediaType.APPLICATION_JSON) + .content(ViewModelConstants.createRequestWithoutBaseVersion().toJson()) + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)) + .andExpect(status().isCreated()) + .andExpect(content().contentType(MediaType.APPLICATION_JSON)) + .andExpect(content().json(ViewModelConstants.pointerResponse().toJson())); + } + + @Test + public void updateViewReplacingExistingViewReturns200() throws Exception { + mvc.perform( + MockMvcRequestBuilders.put(VIEWS_PATH + "/my_view") + .contentType(MediaType.APPLICATION_JSON) + .content(ViewModelConstants.fullyPopulatedRequest().toJson()) + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)) + .andExpect(status().isOk()) + .andExpect(content().contentType(MediaType.APPLICATION_JSON)) + .andExpect(content().json(ViewModelConstants.pointerResponse().toJson())); + } + + @Test + public void updateViewCreatingNewViewReturns201() throws Exception { + mvc.perform( + MockMvcRequestBuilders.put(VIEWS_PATH + "/" + MockViewsApiHandler.PUT_CREATES_VIEW_ID) + .contentType(MediaType.APPLICATION_JSON) + .content( + ViewModelConstants.fullyPopulatedRequest() + .toBuilder() + .viewId(MockViewsApiHandler.PUT_CREATES_VIEW_ID) + .build() + .toJson()) + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)) + .andExpect(status().isCreated()) + .andExpect(content().contentType(MediaType.APPLICATION_JSON)) + .andExpect(content().json(ViewModelConstants.pointerResponse().toJson())); + } + + /** + * The list body is asserted with JSON paths rather than a whole-document comparison: the Gson + * {@code toJson()} helper on {@link + * com.linkedin.openhouse.tables.api.spec.v0.response.GetAllViewsResponseBody} serializes the + * Spring {@code PageImpl} by its internal fields, whereas the response goes out through Jackson + * and its getters. Only the Jackson shape is the wire contract, so it is what this asserts. + */ + @Test + public void getAllViewsReturns200WithSparsePaginatedBody() throws Exception { + mvc.perform( + MockMvcRequestBuilders.get(VIEWS_PATH) + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)) + .andExpect(status().isOk()) + .andExpect(content().contentType(MediaType.APPLICATION_JSON)) + .andExpect(jsonPath("$.pageResults.content", Matchers.hasSize(2))) + .andExpect(jsonPath("$.pageResults.content[0].viewId", Matchers.is("my_view"))) + .andExpect( + jsonPath( + "$.pageResults.content[0].databaseId", Matchers.is(ViewModelConstants.DATABASE_ID))) + .andExpect(jsonPath("$.pageResults.content[1].viewId", Matchers.is("my_other_view"))) + // Sparse by design: list elements populate identifiers only. + .andExpect(jsonPath("$.pageResults.content[0].metadataLocation").doesNotExist()) + .andExpect(jsonPath("$.pageResults.totalElements", Matchers.is(2))) + .andExpect(jsonPath("$.pageResults.size", Matchers.is(50))) + .andExpect(jsonPath("$.pageResults.number", Matchers.is(0))); + } + + @Test + public void deleteViewReturns204WithNoBody() throws Exception { + mvc.perform( + MockMvcRequestBuilders.delete(VIEWS_PATH + "/my_view") + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)) + .andExpect(status().isNoContent()) + .andExpect(content().string("")); + } + + // --------------------------------------------------------------------------------------------- + // Service audit redaction + // --------------------------------------------------------------------------------------------- + + /** + * {@link com.linkedin.openhouse.common.audit.ServiceAuditAspect} records the complete cached + * request body for every controller call, so the create and replace routes would otherwise write + * the caller's SQL text and schema document into a service audit event. {@link + * ViewRequestPayloadRedactor} replaces those values before the event is built; these tests pin + * that, on the success path and on a failure path, and pin that nothing else in the payload is + * disturbed. + * + *

The fixtures carry marker identifiers that appear nowhere else, so the "absent" assertions + * fail loudly if the redaction is removed rather than passing on a coincidence. + */ + private static final String SECRET_SQL_MARKER = "secret_sql_marker_column"; + + private static final String SECRET_SCHEMA_MARKER = "secret_schema_marker_column"; + + private static final String SECRET_SQL = + "SELECT " + SECRET_SQL_MARKER + " FROM my_database.my_table"; + + private static final String SECRET_SCHEMA = + "{\"type\": \"struct\", \"schema-id\": 0, \"fields\": [" + + "{\"id\": 1, \"required\": true, \"name\": \"" + + SECRET_SCHEMA_MARKER + + "\", \"type\": \"string\"}]}"; + + private static CreateUpdateViewRequestBody requestCarryingSecretDefinition() { + return ViewModelConstants.fullyPopulatedRequest() + .toBuilder() + .schema(SECRET_SCHEMA) + .representations( + Collections.singletonList( + ViewRepresentation.builder() + .type(ViewModelConstants.SQL_REPRESENTATION_TYPE) + .sql(SECRET_SQL) + .dialect(ViewModelConstants.SOURCE_DIALECT) + .build())) + .build(); + } + + @Test + public void serviceAuditOnViewCreateRedactsSchemaAndSql() throws Exception { + mvc.perform( + MockMvcRequestBuilders.post(VIEWS_PATH) + .contentType(MediaType.APPLICATION_JSON) + .content(requestCarryingSecretDefinition().toJson()) + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)); + + ServiceAuditEvent event = capturedAuditEvent(); + Assertions.assertEquals(201, event.getStatusCode(), "Precondition: the create must succeed."); + assertViewDefinitionRedacted(event); + } + + @Test + public void serviceAuditOnViewReplaceRedactsSchemaAndSql() throws Exception { + mvc.perform( + MockMvcRequestBuilders.put(VIEWS_PATH + "/my_view") + .contentType(MediaType.APPLICATION_JSON) + .content(requestCarryingSecretDefinition().toJson()) + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)); + + ServiceAuditEvent event = capturedAuditEvent(); + Assertions.assertEquals(200, event.getStatusCode(), "Precondition: the replace must succeed."); + assertViewDefinitionRedacted(event); + } + + /** + * The failure path is the one the reviewer called out: the audit event is emitted from the shared + * exception handler, after the request body has already been cached, so a rejected request writes + * its payload just as an accepted one does. + */ + @Test + public void serviceAuditOnFailedViewCreateRedactsSchemaAndSql() throws Exception { + String failingDatabaseId = MockViewsApiHandler.databaseIdFor(ViewErrorCode.VIEWS_DISABLED); + + mvc.perform( + MockMvcRequestBuilders.post(viewsPath(failingDatabaseId)) + .contentType(MediaType.APPLICATION_JSON) + .content(requestCarryingSecretDefinition().toJson()) + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)); + + ServiceAuditEvent event = capturedAuditEvent(); + Assertions.assertEquals( + 404, event.getStatusCode(), "Precondition: the create must actually be rejected."); + assertViewDefinitionRedacted(event); + } + + private ServiceAuditEvent capturedAuditEvent() { + Mockito.verify(serviceAuditHandler, Mockito.atLeastOnce()).audit(argCaptor.capture()); + return argCaptor.getValue(); + } + + private void assertViewDefinitionRedacted(ServiceAuditEvent event) { + JsonElement payload = event.getRequestPayload(); + Assertions.assertNotNull(payload, "The audit event must still carry a request payload."); + Assertions.assertTrue(payload.isJsonObject(), "The view request payload is a JSON object."); + JsonObject payloadObject = payload.getAsJsonObject(); + + Assertions.assertTrue( + payloadObject.has("schema"), + "The key must survive redaction so an auditor can see the field was sent."); + Assertions.assertEquals( + ServiceAuditPayloadRedactor.REDACTED_VALUE, + payloadObject.get("schema").getAsString(), + "The schema document must not reach the audit event."); + + JsonArray representations = payloadObject.getAsJsonArray("representations"); + Assertions.assertNotNull(representations, "The representations array must survive redaction."); + Assertions.assertEquals(1, representations.size()); + for (JsonElement representation : representations) { + JsonObject representationObject = representation.getAsJsonObject(); + Assertions.assertEquals( + ServiceAuditPayloadRedactor.REDACTED_VALUE, + representationObject.get("sql").getAsString(), + "The SQL text must not reach the audit event."); + // Everything else on the representation is metadata, not caller content. + Assertions.assertEquals( + ViewModelConstants.SQL_REPRESENTATION_TYPE, + representationObject.get("type").getAsString()); + Assertions.assertEquals( + ViewModelConstants.SOURCE_DIALECT, representationObject.get("dialect").getAsString()); + } + + String serializedPayload = payload.toString(); + Assertions.assertFalse( + serializedPayload.contains(SECRET_SQL_MARKER), + "No fragment of the submitted SQL may appear anywhere in the audited payload."); + Assertions.assertFalse( + serializedPayload.contains(SECRET_SCHEMA_MARKER), + "No fragment of the submitted schema may appear anywhere in the audited payload."); + + // The identifying and routing fields are what makes the audit event useful; leave them alone. + Assertions.assertEquals(ViewModelConstants.VIEW_ID, payloadObject.get("viewId").getAsString()); + Assertions.assertEquals( + ViewModelConstants.DATABASE_ID, payloadObject.get("databaseId").getAsString()); + Assertions.assertEquals( + ViewModelConstants.CLUSTER_ID, payloadObject.get("clusterId").getAsString()); + Assertions.assertEquals( + ViewModelConstants.SOURCE_DIALECT, payloadObject.get("sourceDialect").getAsString()); + Assertions.assertEquals( + ViewModelConstants.DEFAULT_CATALOG, payloadObject.get("defaultCatalog").getAsString()); + Assertions.assertEquals( + ViewModelConstants.METADATA_LOCATION, payloadObject.get("baseViewVersion").getAsString()); + Assertions.assertEquals( + ViewModelConstants.DATABASE_ID, + payloadObject.getAsJsonArray("defaultNamespace").get(0).getAsString()); + Assertions.assertEquals( + "openhouse", + payloadObject.getAsJsonObject("viewProperties").get("owner").getAsString(), + "View properties are caller metadata, not view definition, and stay auditable."); + } + + // --------------------------------------------------------------------------------------------- + // Negative paths + // --------------------------------------------------------------------------------------------- + + /** Builds a request against one of the five routes for a given database id. */ + @FunctionalInterface + interface ViewRoute { + MockHttpServletRequestBuilder request(String databaseId); + } + + private static String viewsPath(String databaseId) { + return "/v1/databases/" + databaseId + "/views"; + } + + /** All five routes, so a route cannot quietly skip authentication or exception handling. */ + private static Stream allRoutes() { + return Stream.of( + Arguments.of( + "GET view", + (ViewRoute) + databaseId -> + MockMvcRequestBuilders.get(viewsPath(databaseId) + "/my_view") + .accept(MediaType.APPLICATION_JSON)), + Arguments.of( + "GET views", + (ViewRoute) + databaseId -> + MockMvcRequestBuilders.get(viewsPath(databaseId)) + .accept(MediaType.APPLICATION_JSON)), + Arguments.of( + "POST view", + (ViewRoute) + databaseId -> + MockMvcRequestBuilders.post(viewsPath(databaseId)) + .contentType(MediaType.APPLICATION_JSON) + .content(ViewModelConstants.createRequestWithoutBaseVersion().toJson()) + .accept(MediaType.APPLICATION_JSON)), + Arguments.of( + "PUT view", + (ViewRoute) + databaseId -> + MockMvcRequestBuilders.put(viewsPath(databaseId) + "/my_view") + .contentType(MediaType.APPLICATION_JSON) + .content(ViewModelConstants.fullyPopulatedRequest().toJson()) + .accept(MediaType.APPLICATION_JSON)), + Arguments.of( + "DELETE view", + (ViewRoute) + databaseId -> + MockMvcRequestBuilders.delete(viewsPath(databaseId) + "/my_view") + .accept(MediaType.APPLICATION_JSON))); + } + + /** + * Every internal code selects its declared status and leaves the fixed message untouched. The + * code itself is deliberately absent from the body: the last assertion is the guard that keeps it + * that way, because adding a code field would otherwise be an invisible wire change. + */ + @ParameterizedTest(name = "{0}") + @EnumSource(ViewErrorCode.class) + public void everyInternalErrorCodeSelectsItsStatusAndKeepsTheMessageFixed(ViewErrorCode errorCode) + throws Exception { + mvc.perform( + MockMvcRequestBuilders.get( + viewsPath(MockViewsApiHandler.databaseIdFor(errorCode)) + "/my_view") + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)) + .andExpect(status().is(errorCode.getHttpStatus().value())) + .andExpect(jsonPath("$.message", Matchers.is(MockViewsApiHandler.VIEW_FAILURE_MESSAGE))) + .andExpect(jsonPath("$.status", Matchers.is(errorCode.getHttpStatus().name()))) + .andExpect(jsonPath("$.errorCode").doesNotExist()); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("allRoutes") + public void everyRouteRejectsAMissingBearerTokenWith401(String routeName, ViewRoute route) + throws Exception { + mvc.perform(route.request("d200")).andExpect(status().isUnauthorized()); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("allRoutes") + public void everyRouteRejectsAMalformedBearerTokenWith401(String routeName, ViewRoute route) + throws Exception { + mvc.perform(route.request("d200").header("Authorization", "Bearer not-a-real-jwt")) + .andExpect(status().isUnauthorized()); + } + + /** + * Exercises exception mapping only. {@code AuthorizationInterceptor.check()} unconditionally + * returns an allow decision today, so nothing in this PR can deny a request on privilege grounds; + * what this pins is that when the handler layer eventually does deny one, the shared handler + * turns it into a 403 rather than a 500. + */ + @Test + public void accessDeniedFromTheHandlerIsMappedTo403WithoutPrivilegeEnforcement() + throws Exception { + mvc.perform( + MockMvcRequestBuilders.get( + viewsPath(MockViewsApiHandler.ACCESS_DENIED_DATABASE_ID) + "/my_view") + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)) + .andExpect(status().isForbidden()) + .andExpect(jsonPath("$.message", Matchers.is(MockViewsApiHandler.ACCESS_DENIED_MESSAGE))); + } + + /** An uncoded infrastructure failure still lands on 503 rather than falling through to 500. */ + @Test + public void genericInfrastructureFailureIsMappedTo503() throws Exception { + mvc.perform( + MockMvcRequestBuilders.get( + viewsPath(MockViewsApiHandler.UNAVAILABLE_DATABASE_ID) + "/my_view") + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)) + .andExpect(status().isServiceUnavailable()) + .andExpect(jsonPath("$.message", Matchers.is(MockViewsApiHandler.UNAVAILABLE_MESSAGE))) + .andExpect(jsonPath("$.errorCode").doesNotExist()); + } + + /** + * Malformed JSON fails during message conversion, before any view code runs, so it must stay on + * the shared Jackson path and carry no view vocabulary at all. + */ + @Test + public void malformedJsonBodyIsRejectedByTheSharedHandlerWith400() throws Exception { + mvc.perform( + MockMvcRequestBuilders.post(VIEWS_PATH) + .contentType(MediaType.APPLICATION_JSON) + .content("{\"viewId\": ") + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.message", Matchers.startsWith("Unacceptable JSON"))) + .andExpect(jsonPath("$.errorCode").doesNotExist()); + } + + /** + * Views mount under {@code /v1} alongside every other OpenHouse resource; the {@code /v2} prefix + * they were briefly drafted against must not resolve. {@link ViewsController} is the only class + * that could ever map a {@code /v2} view path, so this fails exactly when a stale mapping is left + * behind or reintroduced. + */ + @Test + public void theSamePathUnderV2DoesNotResolve() throws Exception { + mvcThrowingOnUnmappedPath + .perform( + MockMvcRequestBuilders.get("/v2/databases/d200/views/my_view") + .accept(MediaType.APPLICATION_JSON) + .header("Authorization", "Bearer " + jwtAccessToken)) + .andExpect(jsonPath("$.message", Matchers.containsString("cannot be resolved by server"))) + // Proves the request never reached the handler, which would have answered 200 with a + // pointer body. + .andExpect(jsonPath("$.viewId").doesNotExist()); + } + + // --------------------------------------------------------------------------------------------- + // Published contract: declared response codes + // --------------------------------------------------------------------------------------------- + + /** + * The status set each operation publishes, taken from the status matrix. {@code + * client/tableclient} is generated from the OpenAPI document these annotations produce, so a + * status the routes can return but do not declare is invisible to every generated client. + */ + private static Stream declaredResponseCodes() { + return Stream.of( + Arguments.of("getView", codes("200", "400", "401", "403", "404", "503")), + Arguments.of("getAllViews", codes("200", "400", "401", "403", "404", "503")), + Arguments.of("createView", codes("201", "400", "401", "403", "404", "409", "422", "503")), + Arguments.of( + "updateView", codes("200", "201", "400", "401", "403", "404", "409", "422", "503")), + Arguments.of("deleteView", codes("204", "400", "401", "403", "404", "503"))); + } + + private static Set codes(String... responseCodes) { + return new TreeSet<>(Arrays.asList(responseCodes)); + } + + private static Set declaredResponseCodesOf(String methodName) { + Method method = + Arrays.stream(ViewsController.class.getDeclaredMethods()) + .filter(candidate -> candidate.getName().equals(methodName)) + .findFirst() + .orElseThrow( + () -> + new AssertionError( + "ViewsController has no method named " + methodName + " any more")); + + ApiResponses apiResponses = method.getAnnotation(ApiResponses.class); + Assertions.assertNotNull( + apiResponses, methodName + " must declare its responses for the generated spec"); + + return Arrays.stream(apiResponses.value()) + .map(io.swagger.v3.oas.annotations.responses.ApiResponse::responseCode) + .collect(Collectors.toCollection(TreeSet::new)); + } + + /** + * Pins the exact published status set per operation. + * + *

Asserted off the annotations rather than off a generated document on purpose: booting the + * app to produce the spec needs a free port, and the default spec-generation port is occupied by + * an unrelated stale service in some environments, which silently yields a views-free document. + * The annotations are the sole input to that document, so pinning them pins the contract without + * that failure mode. + */ + @ParameterizedTest(name = "{0}") + @MethodSource("declaredResponseCodes") + public void eachOperationPublishesExactlyTheStatusesItCanReturn( + String methodName, Set expectedCodes) { + Assertions.assertEquals( + expectedCodes, + declaredResponseCodesOf(methodName), + "The published status set for " + + methodName + + " drifted from the statuses the route can actually return. Every status asserted by" + + " the error-code, 401, 403 and 503 tests in this class must also be declared here," + + " because the generated client is built from these annotations."); + } + + /** + * Ties the internal taxonomy to the published contract: a write route can surface any {@link + * ViewErrorCode}, so every status those codes map to must be declared on POST and PUT. Adding a + * code with a new status now fails here instead of silently producing an undeclared response. + */ + @Test + public void everyInternalErrorCodeStatusIsPublishedOnTheWriteRoutes() { + Set codeStatuses = + Arrays.stream(ViewErrorCode.values()) + .map(errorCode -> String.valueOf(errorCode.getHttpStatus().value())) + .collect(Collectors.toCollection(TreeSet::new)); + + Assertions.assertTrue( + declaredResponseCodesOf("createView").containsAll(codeStatuses), + "POST does not publish every status its internal codes can select: " + codeStatuses); + Assertions.assertTrue( + declaredResponseCodesOf("updateView").containsAll(codeStatuses), + "PUT does not publish every status its internal codes can select: " + codeStatuses); + } +} diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/mapper/ViewsMapperTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/mapper/ViewsMapperTest.java new file mode 100644 index 000000000..786616cba --- /dev/null +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/mapper/ViewsMapperTest.java @@ -0,0 +1,105 @@ +package com.linkedin.openhouse.tables.mock.mapper; + +import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateViewRequestBody; +import com.linkedin.openhouse.tables.api.spec.v0.response.GetViewResponseBody; +import com.linkedin.openhouse.tables.dto.mapper.ViewsMapper; +import com.linkedin.openhouse.tables.model.ViewDto; +import com.linkedin.openhouse.tables.model.ViewModelConstants; +import java.util.Arrays; +import java.util.List; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.PageImpl; +import org.springframework.data.domain.PageRequest; + +/** Golden-path mapping coverage for {@link ViewsMapper}. */ +@SpringBootTest +public class ViewsMapperTest { + + @Autowired private ViewsMapper viewsMapper; + + @Test + public void testRequestMapsToViewDtoStoringBaseVersionAsViewVersion() { + CreateUpdateViewRequestBody requestBody = ViewModelConstants.fullyPopulatedRequest(); + + ViewDto viewDto = viewsMapper.toViewDto(requestBody); + + Assertions.assertEquals(requestBody.getViewId(), viewDto.getViewId()); + Assertions.assertEquals(requestBody.getDatabaseId(), viewDto.getDatabaseId()); + Assertions.assertEquals(requestBody.getClusterId(), viewDto.getClusterId()); + Assertions.assertEquals(requestBody.getSchema(), viewDto.getSchema()); + Assertions.assertEquals(requestBody.getRepresentations(), viewDto.getRepresentations()); + Assertions.assertEquals(requestBody.getSourceDialect(), viewDto.getSourceDialect()); + Assertions.assertEquals(requestBody.getDefaultCatalog(), viewDto.getDefaultCatalog()); + Assertions.assertEquals(requestBody.getDefaultNamespace(), viewDto.getDefaultNamespace()); + Assertions.assertEquals(requestBody.getViewProperties(), viewDto.getViewProperties()); + Assertions.assertEquals( + requestBody.getBaseViewVersion(), + viewDto.getViewVersion(), + "The caller's base version is stored as viewVersion so the service can compare it against" + + " the current pointer, mirroring how baseTableVersion maps to tableVersion."); + + // Pointer fields are server-owned and must not be populated from a request. + Assertions.assertNull(viewDto.getViewUri()); + Assertions.assertNull(viewDto.getMetadataLocation()); + Assertions.assertNull(viewDto.getViewCreator()); + Assertions.assertEquals(0L, viewDto.getCreationTime()); + Assertions.assertEquals(0L, viewDto.getLastModifiedTime()); + } + + /** + * Uses distinct sentinels for {@code metadataLocation} and {@code viewVersion}. In production the + * two hold the same value, which would let a swapped mapping pass unnoticed. + */ + @Test + public void testViewDtoMapsToPointerResponseBody() { + ViewDto viewDto = + ViewDto.builder() + .viewId(ViewModelConstants.VIEW_ID) + .databaseId(ViewModelConstants.DATABASE_ID) + .clusterId(ViewModelConstants.CLUSTER_ID) + .viewUri(ViewModelConstants.VIEW_URI) + .metadataLocation(ViewModelConstants.DISTINCT_METADATA_LOCATION) + .viewVersion(ViewModelConstants.DISTINCT_VIEW_VERSION) + .creationTime(ViewModelConstants.CREATION_TIME) + // Definition fields have no counterpart on the pointer-only read contract. + .schema(ViewModelConstants.VIEW_SCHEMA_LITERAL) + .sourceDialect(ViewModelConstants.SOURCE_DIALECT) + .build(); + + GetViewResponseBody responseBody = viewsMapper.toGetViewResponseBody(viewDto); + + Assertions.assertEquals(ViewModelConstants.pointerResponseWithDistinctPointers(), responseBody); + Assertions.assertEquals( + ViewModelConstants.DISTINCT_METADATA_LOCATION, responseBody.getMetadataLocation()); + Assertions.assertEquals( + ViewModelConstants.DISTINCT_VIEW_VERSION, responseBody.getViewVersion()); + } + + @Test + public void testViewDtoPageMapsToSparseResponsePagePreservingMetadata() { + List content = + Arrays.asList( + ViewDto.builder().viewId("my_view").databaseId(ViewModelConstants.DATABASE_ID).build(), + ViewDto.builder() + .viewId("my_other_view") + .databaseId(ViewModelConstants.DATABASE_ID) + .build()); + Page dtoPage = new PageImpl<>(content, PageRequest.of(1, 2), 7); + + Page responsePage = viewsMapper.toGetViewResponseBodyPage(dtoPage); + + Assertions.assertEquals( + ViewModelConstants.sparseListPage().getContent(), responsePage.getContent()); + Assertions.assertEquals(1, responsePage.getNumber()); + Assertions.assertEquals(2, responsePage.getSize()); + Assertions.assertEquals(7, responsePage.getTotalElements()); + Assertions.assertEquals(4, responsePage.getTotalPages()); + Assertions.assertNull( + responsePage.getContent().get(0).getMetadataLocation(), + "List elements stay sparse: only identifiers are populated."); + } +} diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/service/ViewsDisabledServiceTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/service/ViewsDisabledServiceTest.java new file mode 100644 index 000000000..627eb99e4 --- /dev/null +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/mock/service/ViewsDisabledServiceTest.java @@ -0,0 +1,89 @@ +package com.linkedin.openhouse.tables.mock.service; + +import com.linkedin.openhouse.tables.exception.ViewApiException; +import com.linkedin.openhouse.tables.exception.ViewErrorCode; +import com.linkedin.openhouse.tables.model.ViewModelConstants; +import com.linkedin.openhouse.tables.services.ViewsDisabledService; +import com.linkedin.openhouse.tables.services.ViewsService; +import java.util.stream.Stream; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.springframework.http.HttpStatus; + +/** + * Freezes the only behaviour the stub service has: every operation reports that views are disabled. + * + *

Plain instantiation rather than a Spring context: the bean has no collaborators, so a context + * would only slow the test down without exercising anything extra. + */ +public class ViewsDisabledServiceTest { + + /** + * Deliberately duplicated rather than referenced from the production constant, which is + * package-private. Restating the literal is the point: this is the frozen, redacted message that + * reaches the error body and the service audit event, so a change to it must break a test. + */ + private static final String EXPECTED_MESSAGE = "Views are disabled"; + + private static final String ACTING_PRINCIPAL = "DUMMY_ANONYMOUS_USER"; + + /** One entry per {@link ViewsService} method, so a new method cannot silently skip the gate. */ + private static Stream allServiceOperations() { + return Stream.of( + Arguments.of( + "getView", + (ServiceOperation) + service -> + service.getView( + ViewModelConstants.DATABASE_ID, + ViewModelConstants.VIEW_ID, + ACTING_PRINCIPAL)), + Arguments.of( + "getAllViews", + (ServiceOperation) + service -> + service.getAllViews( + ViewModelConstants.DATABASE_ID, 0, 50, null, ACTING_PRINCIPAL)), + Arguments.of( + "putView", + (ServiceOperation) + service -> + service.putView( + ViewModelConstants.createRequestWithoutBaseVersion(), + ACTING_PRINCIPAL, + true)), + Arguments.of( + "deleteView", + (ServiceOperation) + service -> + service.deleteView( + ViewModelConstants.DATABASE_ID, + ViewModelConstants.VIEW_ID, + ACTING_PRINCIPAL))); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("allServiceOperations") + public void everyOperationReportsViewsDisabled(String operationName, ServiceOperation operation) { + ViewsDisabledService service = new ViewsDisabledService(); + + ViewApiException exception = + Assertions.assertThrows(ViewApiException.class, () -> operation.run(service)); + + Assertions.assertEquals( + ViewErrorCode.VIEWS_DISABLED, + exception.getErrorCode(), + "The stub must report the designed disabled code, not a generic failure: an uncoded" + + " unchecked exception would surface as a 500 with a stack trace instead."); + Assertions.assertEquals(HttpStatus.NOT_FOUND, exception.getHttpStatus()); + Assertions.assertEquals(EXPECTED_MESSAGE, exception.getMessage()); + } + + /** Invokes one {@link ViewsService} method; needed because the four have different shapes. */ + @FunctionalInterface + interface ServiceOperation { + void run(ViewsService service); + } +} diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/model/ViewModelConstants.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/model/ViewModelConstants.java new file mode 100644 index 000000000..4cf63968c --- /dev/null +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/model/ViewModelConstants.java @@ -0,0 +1,237 @@ +package com.linkedin.openhouse.tables.model; + +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.INITIAL_TABLE_VERSION; +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.MAX_VIEW_SCHEMA_BYTES; +import static com.linkedin.openhouse.common.api.validator.ValidatorConstants.MAX_VIEW_SQL_BYTES; + +import com.linkedin.openhouse.common.api.validator.ValidatorConstants; +import com.linkedin.openhouse.tables.api.spec.v0.request.CreateUpdateViewRequestBody; +import com.linkedin.openhouse.tables.api.spec.v0.request.components.ViewRepresentation; +import com.linkedin.openhouse.tables.api.spec.v0.response.GetAllViewsResponseBody; +import com.linkedin.openhouse.tables.api.spec.v0.response.GetViewResponseBody; +import java.util.Arrays; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.PageImpl; +import org.springframework.data.domain.PageRequest; + +/** + * Deterministic fixtures for the /v1 views wire surface. Everything here is a fixed literal: no + * random identifiers, no {@code UUID.randomUUID()} and no {@code System.currentTimeMillis()}, so + * contract assertions stay byte-stable across runs. + */ +public final class ViewModelConstants { + + private ViewModelConstants() {} + + public static final String VIEW_ID = "my_view"; + public static final String DATABASE_ID = "my_database"; + public static final String CLUSTER_ID = "my-cluster"; + public static final String VIEW_URI = "my-cluster.my_database.my_view"; + public static final String METADATA_LOCATION = + "file:/tmp/openhouse/my_database/my_view/metadata/00000-fixed.metadata.json"; + public static final String VIEW_VERSION = + "file:/tmp/openhouse/my_database/my_view/metadata/00000-fixed.metadata.json"; + public static final long CREATION_TIME = 1651002318265L; + + /** + * Distinct sentinels for {@code metadataLocation} and {@code viewVersion}. In production the two + * are equal (design doc §5: viewVersion is the current metadataLocation), which is what the + * general fixtures model. That equality would, however, let a serialization bug swap the two + * Jackson property associations undetected, so this pair deliberately breaks it for the + * serialization-freeze test and pins each field independently. + */ + public static final String DISTINCT_METADATA_LOCATION = + "file:/tmp/openhouse/my_database/my_view/metadata/sentinel-metadata-location.metadata.json"; + + public static final String DISTINCT_VIEW_VERSION = + "file:/tmp/openhouse/my_database/my_view/metadata/sentinel-view-version.metadata.json"; + + public static final String SOURCE_DIALECT = "spark"; + public static final String SQL_REPRESENTATION_TYPE = "sql"; + public static final String VIEW_SQL = "SELECT id, name FROM my_database.my_table"; + public static final String DEFAULT_CATALOG = "openhouse"; + + /** Iceberg schema JSON with unique, explicit field ids. */ + public static final String VIEW_SCHEMA_LITERAL = + "{\"type\": \"struct\", \"schema-id\": 0, \"fields\": [" + + "{\"id\": 1, \"required\": true, \"name\": \"id\", \"type\": \"string\"}, " + + "{\"id\": 2, \"required\": true, \"name\": \"name\", \"type\": \"string\"}]}"; + + public static final ViewRepresentation SPARK_REPRESENTATION = + ViewRepresentation.builder() + .type(SQL_REPRESENTATION_TYPE) + .sql(VIEW_SQL) + .dialect(SOURCE_DIALECT) + .build(); + + public static final List DEFAULT_NAMESPACE = + Collections.unmodifiableList(Collections.singletonList(DATABASE_ID)); + + public static final Map VIEW_PROPERTIES; + + static { + Map properties = new LinkedHashMap<>(); + properties.put("owner", "openhouse"); + VIEW_PROPERTIES = Collections.unmodifiableMap(properties); + } + + /** Every field populated, including both nullable optional fields and a replace token. */ + public static CreateUpdateViewRequestBody fullyPopulatedRequest() { + return baseRequestBuilder().baseViewVersion(METADATA_LOCATION).build(); + } + + /** POST create shape where the caller omits the base version entirely. */ + public static CreateUpdateViewRequestBody createRequestWithoutBaseVersion() { + return baseRequestBuilder().build(); + } + + /** POST create shape where the caller sends the table-style initial version token. */ + public static CreateUpdateViewRequestBody createRequestWithInitialBaseVersion() { + return baseRequestBuilder().baseViewVersion(INITIAL_TABLE_VERSION).build(); + } + + /** Fully populated pointer response for an item GET. */ + public static GetViewResponseBody pointerResponse() { + return GetViewResponseBody.builder() + .viewId(VIEW_ID) + .databaseId(DATABASE_ID) + .clusterId(CLUSTER_ID) + .viewUri(VIEW_URI) + .metadataLocation(METADATA_LOCATION) + .viewVersion(VIEW_VERSION) + .creationTime(CREATION_TIME) + .build(); + } + + /** Sparse list element: identifiers only, as the list path populates nothing else. */ + public static GetViewResponseBody sparseListElement(String viewId) { + return GetViewResponseBody.builder().viewId(viewId).databaseId(DATABASE_ID).build(); + } + + /** + * Pointer response whose {@code metadataLocation} and {@code viewVersion} carry distinct + * sentinels rather than the production-equal value, so a serialization freeze can pin the two + * fields independently. Use {@link #pointerResponse()} wherever production equality matters. + */ + public static GetViewResponseBody pointerResponseWithDistinctPointers() { + return pointerResponse() + .toBuilder() + .metadataLocation(DISTINCT_METADATA_LOCATION) + .viewVersion(DISTINCT_VIEW_VERSION) + .build(); + } + + /** Deterministic single page of sparse identifier-only elements. */ + public static Page sparseListPage() { + List content = + Arrays.asList(sparseListElement("my_view"), sparseListElement("my_other_view")); + return new PageImpl<>(content, PageRequest.of(0, 50), content.size()); + } + + public static GetAllViewsResponseBody listResponse() { + return GetAllViewsResponseBody.builder().pageResults(sparseListPage()).build(); + } + + // ----------------------------------------------------------------------------------------- + // Negative-path fixtures + // ----------------------------------------------------------------------------------------- + + /** + * Truncated JSON: Jackson cannot parse it at all, so Iceberg fails inside {@code JsonUtil.parse} + * and reports it as an {@code UncheckedIOException} rather than the {@code + * IllegalArgumentException} it raises for a document that parses but is not an Iceberg schema. + * {@code ViewSchemaParseBoundaryTest} pins that split, which is why the validator catches both. + */ + public static final String MALFORMED_SCHEMA_LITERAL = "{\"type\": \"struct\", \"fields\": ["; + + /** + * The Spark {@code StructType} JSON an engine may reach for by mistake: structurally similar, but + * its fields carry {@code nullable}/{@code metadata} instead of the field ids Iceberg requires. + */ + public static final String SPARK_STRUCT_TYPE_SCHEMA_LITERAL = + "{\"type\": \"struct\", \"fields\": [" + + "{\"name\": \"id\", \"type\": \"string\", \"nullable\": true, \"metadata\": {}}, " + + "{\"name\": \"name\", \"type\": \"string\", \"nullable\": true, \"metadata\": {}}]}"; + + /** + * Two fields sharing field id 1. Iceberg's own parser rejects this while building the schema, so + * the validator deliberately has no duplicate-id check of its own. + */ + public static final String DUPLICATE_FIELD_ID_SCHEMA_LITERAL = + "{\"type\": \"struct\", \"schema-id\": 0, \"fields\": [" + + "{\"id\": 1, \"required\": true, \"name\": \"id\", \"type\": \"string\"}, " + + "{\"id\": 1, \"required\": true, \"name\": \"name\", \"type\": \"string\"}]}"; + + /** + * A valid Iceberg schema of exactly {@link ValidatorConstants#MAX_VIEW_SCHEMA_BYTES} UTF-8 bytes, + * padded with insignificant JSON whitespace so the size boundary can be probed without also + * changing whether the document parses. + * + *

The boundary fixtures are exposed as no-argument methods rather than one helper taking a + * requested size. The size is derived from the limit the validator enforces, so there is no + * argument a caller could get wrong and therefore no precondition to enforce and no failure to + * report. + */ + public static String schemaAtMaxUtf8Size() { + return schemaPaddedTo(MAX_VIEW_SCHEMA_BYTES); + } + + /** The same valid Iceberg schema, one UTF-8 byte past the limit. */ + public static String schemaOneByteOverMaxUtf8Size() { + return schemaPaddedTo(MAX_VIEW_SCHEMA_BYTES + 1); + } + + /** Opaque SQL of exactly {@link ValidatorConstants#MAX_VIEW_SQL_BYTES} UTF-8 bytes. */ + public static String sqlAtMaxUtf8Size() { + return sqlPaddedTo(MAX_VIEW_SQL_BYTES); + } + + /** The same opaque SQL, one UTF-8 byte past the limit. */ + public static String sqlOneByteOverMaxUtf8Size() { + return sqlPaddedTo(MAX_VIEW_SQL_BYTES + 1); + } + + private static String schemaPaddedTo(int totalBytes) { + return "{" + + spaces(totalBytes - VIEW_SCHEMA_LITERAL.length()) + + VIEW_SCHEMA_LITERAL.substring(1); + } + + private static String sqlPaddedTo(int totalBytes) { + return VIEW_SQL + spaces(totalBytes - VIEW_SQL.length()); + } + + /** + * SQL built entirely from a two-byte character, so its character count and its UTF-8 byte count + * differ by a factor of two. Used to prove the size rules count bytes. + */ + public static String multiByteSql(int characterCount) { + char[] characters = new char[characterCount]; + Arrays.fill(characters, 'é'); + return new String(characters); + } + + private static String spaces(int count) { + char[] characters = new char[count]; + Arrays.fill(characters, ' '); + return new String(characters); + } + + private static CreateUpdateViewRequestBody.CreateUpdateViewRequestBodyBuilder + baseRequestBuilder() { + return CreateUpdateViewRequestBody.builder() + .viewId(VIEW_ID) + .databaseId(DATABASE_ID) + .clusterId(CLUSTER_ID) + .schema(VIEW_SCHEMA_LITERAL) + .representations(Collections.singletonList(SPARK_REPRESENTATION)) + .sourceDialect(SOURCE_DIALECT) + .defaultCatalog(DEFAULT_CATALOG) + .defaultNamespace(DEFAULT_NAMESPACE) + .viewProperties(VIEW_PROPERTIES); + } +}