Fix bad request handling (#179)

* update controller to use ParameterObject

As used in the metadata endpoint we want Swagger to be  able to interpret and automatically generate pagination parameters like page, size, and sort.

* Add error handling for incorrect sort value

* Add test for error handling

* Fix workflow to only release main branch to maven central

* Update CHANGELOG.md

* Add unit test for BeeekeeperExceptionHandler

Testing the request response and the errorResponses

* Implement builder pattern for ErrorResponse

---------

Co-authored-by: Hamza Jugon <[email protected]>
Co-authored-by: Jay Green-Stevens <[email protected]>

Author: 3 people

.github/workflows/release.yml

@@ -1,11 +1,8 @@
 name: Release to Maven Central
+
 on:
   workflow_dispatch:
-    inputs:
-      branch:
-        description: "The branch to use to release from."
-        required: true
-        default: "main"
+
 jobs:
   release:
     name: Release to Maven Central
@@ -16,8 +13,7 @@ jobs:
         uses: actions/checkout@v2
         with:
           fetch-depth: 0
-          ref: ${{ github.event.inputs.branch }}
-          # We need a personal access token to be able to push to a protected branch
+          ref: main  # Hardcoded to main branch
           token: ${{ secrets.GH_PERSONAL_ACCESS_TOKEN }}
 
       - name: Set up JDK

CHANGELOG.md

@@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [3.5.7] - 2024-10-25
+### Changed
+- Added error handling for bad requests with incorrect sort parameters.
+- Added automatic pagination handling in the `/unreferenced-paths` endpoint for improved Swagger documentation. 
+- Updated the Maven Central release workflow to run exclusively from the main branch.
+
 ## [3.5.6] - 2024-06-14
 ### Fixed
 - Added aws-sts-jdk dependency in `beekeeper-metadata-cleanup`, `beekeeper-path-cleanup`, `beekeeper-scheduler-apiary` to solve IRSA unable to assume role issue.

beekeeper-api/src/main/java/com/expediagroup/beekeeper/api/BeekeeperApiApplication.java

@@ -29,7 +29,8 @@
 @ComponentScan(basePackages = {
     "com.expediagroup.beekeeper.api.conf",
     "com.expediagroup.beekeeper.api.controller",
-    "com.expediagroup.beekeeper.api.service" })
+    "com.expediagroup.beekeeper.api.service",
+    "com.expediagroup.beekeeper.api.error" })
 public class BeekeeperApiApplication {
   public static void main(String[] args) {
     new SpringApplicationBuilder(BeekeeperApiApplication.class)

beekeeper-api/src/main/java/com/expediagroup/beekeeper/api/controller/BeekeeperController.java

@@ -106,7 +106,7 @@ public ResponseEntity<Page<HousekeepingPathResponse>> getAllPaths(
           @Spec(path = "cleanupTimestamp", params = "deleted_after", spec = GreaterThan.class),
           @Spec(path = "creationTimestamp", params = "registered_before", spec = LessThan.class),
           @Spec(path = "creationTimestamp", params = "registered_after", spec = GreaterThan.class) }) Specification<HousekeepingPath> spec,
-      Pageable pageable) {
+      @ParameterObject Pageable pageable) {
     return ResponseEntity.ok(housekeepingEntityService.getAllPaths(spec, pageable));
   }
 

beekeeper-api/src/main/java/com/expediagroup/beekeeper/api/error/BeekeeperExceptionHandler.java

@@ -0,0 +1,55 @@
+/**
+ * Copyright (C) 2019-2024 Expedia, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.expediagroup.beekeeper.api.error;
+
+import javax.servlet.http.HttpServletRequest;
+
+import org.springframework.data.mapping.PropertyReferenceException;
+import org.springframework.http.HttpStatus;
+import org.springframework.http.ResponseEntity;
+import org.springframework.web.bind.annotation.ExceptionHandler;
+import org.springframework.web.bind.annotation.RestControllerAdvice;
+
+import java.time.LocalDateTime;
+
+@RestControllerAdvice
+public class BeekeeperExceptionHandler {
+
+  /**
+   * Handles invalid sort parameters.
+   *
+   * @param exception  the exception is thrown when an invalid property is referenced
+   * @param request the HTTP request
+   * @return a ResponseEntity containing the error response
+   */
+
+  @ExceptionHandler(PropertyReferenceException.class)
+  public ResponseEntity<ErrorResponse> handlePropertyReferenceException(
+      PropertyReferenceException exception, HttpServletRequest request) {
+
+    ErrorResponse errorResponse = ErrorResponse.builder()
+        .timestamp(LocalDateTime.now().toString())
+        .status(HttpStatus.BAD_REQUEST.value())
+        .error(HttpStatus.BAD_REQUEST.getReasonPhrase())
+        .message(exception.getMessage())
+        .path(request.getRequestURI())
+        .build();
+
+    return new ResponseEntity<>(errorResponse, HttpStatus.BAD_REQUEST);
+  }
+
+}

beekeeper-api/src/main/java/com/expediagroup/beekeeper/api/error/ErrorResponse.java

@@ -0,0 +1,30 @@
+/**
+ * Copyright (C) 2019-2024 Expedia, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.expediagroup.beekeeper.api.error;
+
+import lombok.Builder;
+import lombok.Getter;
+
+@Getter
+@Builder
+public class ErrorResponse {
+  private final String timestamp;
+  private final int status;
+  private final String error;
+  private final String message;
+  private final String path;
+}

beekeeper-api/src/test/java/com/expediagroup/beekeeper/api/error/BeekeeperExceptionHandlerTest.java

@@ -0,0 +1,62 @@
+/**
+ * Copyright (C) 2019-2023 Expedia, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.expediagroup.beekeeper.api.error;
+
+import org.junit.jupiter.api.Test;
+import org.springframework.http.HttpStatus;
+import org.springframework.http.ResponseEntity;
+import org.springframework.mock.web.MockHttpServletRequest;
+import org.springframework.data.mapping.PropertyReferenceException;
+import org.springframework.data.mapping.PropertyPath;
+import org.springframework.data.util.ClassTypeInformation;
+import org.springframework.data.util.TypeInformation;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import com.expediagroup.beekeeper.api.error.BeekeeperExceptionHandler;
+import com.expediagroup.beekeeper.api.error.ErrorResponse;
+import com.expediagroup.beekeeper.core.model.HousekeepingPath;
+
+import java.util.Collections;
+import java.util.List;
+
+public class BeekeeperExceptionHandlerTest {
+
+  private final BeekeeperExceptionHandler exceptionHandler = new BeekeeperExceptionHandler();
+
+  @Test
+  public void handlePropertyReferenceException_ShouldReturnBadRequest() {
+    String propertyName = "string";
+    TypeInformation<?> typeInformation = ClassTypeInformation.from(HousekeepingPath.class);
+    List<PropertyPath> baseProperty = Collections.emptyList();
+    PropertyReferenceException exception = new PropertyReferenceException(propertyName, typeInformation, baseProperty);
+
+    MockHttpServletRequest request = new MockHttpServletRequest();
+    request.setRequestURI("/api/v1/database/testDb/table/testTable/unreferenced-paths");
+
+    ResponseEntity<ErrorResponse> response = exceptionHandler.handlePropertyReferenceException(exception, request);
+
+    assertThat(response.getStatusCode()).isEqualTo(HttpStatus.BAD_REQUEST);
+
+    ErrorResponse errorResponse = response.getBody();
+    assertThat(errorResponse).isNotNull();
+    assertThat(errorResponse.getStatus()).isEqualTo(HttpStatus.BAD_REQUEST.value());
+    assertThat(errorResponse.getError()).isEqualTo("Bad Request");
+    assertThat(errorResponse.getMessage()).isEqualTo(exception.getMessage());
+    assertThat(errorResponse.getPath()).isEqualTo("/api/v1/database/testDb/table/testTable/unreferenced-paths");
+    assertThat(errorResponse.getTimestamp()).isNotNull();
+  }
+}

beekeeper-integration-tests/src/test/java/com/expediagroup/beekeeper/integration/api/BeekeeperApiIntegrationTest.java

@@ -43,6 +43,7 @@
 import org.springframework.boot.SpringApplication;
 import org.springframework.context.ConfigurableApplicationContext;
 import org.springframework.data.domain.Page;
+import org.springframework.http.HttpStatus;
 import org.springframework.util.SocketUtils;
 
 import com.fasterxml.jackson.core.type.TypeReference;
@@ -51,6 +52,7 @@
 import com.fasterxml.jackson.module.paramnames.ParameterNamesModule;
 
 import com.expediagroup.beekeeper.api.BeekeeperApiApplication;
+import com.expediagroup.beekeeper.api.error.ErrorResponse;
 import com.expediagroup.beekeeper.api.response.HousekeepingMetadataResponse;
 import com.expediagroup.beekeeper.api.response.HousekeepingPathResponse;
 import com.expediagroup.beekeeper.core.model.HousekeepingMetadata;
@@ -111,7 +113,7 @@ public final void afterEach() {
 
   @Test
   public void testGetMetadataWhenTableNotFoundReturnsEmptyList()
-    throws SQLException, InterruptedException, IOException {
+      throws SQLException, InterruptedException, IOException {
 
     for (HousekeepingMetadata testMetadata : Arrays.asList(testMetadataB, testMetadataC)) {
       insertExpiredMetadata(testMetadata);
@@ -302,4 +304,23 @@ private HousekeepingPath createHousekeepingPath(
         .build();
   }
 
+  @Test
+  public void testInvalidSortParameter() throws SQLException, IOException, InterruptedException {
+    insertExpiredMetadata(testMetadataA);
+
+    String filters = "?sort=nonExistentProperty,asc";
+    HttpResponse<String> response = testClient.getMetadata(someDatabase, someTable, filters);
+
+    assertThat(response.statusCode()).isEqualTo(HttpStatus.BAD_REQUEST.value());
+
+    String body = response.body();
+    ErrorResponse errorResponse = mapper.readValue(body, ErrorResponse.class);
+
+    assertThat(errorResponse.getStatus()).isEqualTo(HttpStatus.BAD_REQUEST.value());
+    assertThat(errorResponse.getMessage()).isEqualTo("No property 'nonExistentProperty' found for type 'HousekeepingMetadata'");
+    assertThat(errorResponse.getError()).isEqualTo("Bad Request");
+    assertThat(errorResponse.getPath()).contains("/api/v1/database/some_database/table/some_table/metadata");
+    assertThat(errorResponse.getTimestamp()).isNotNull();
+  }
+
 }