Fix bad request handling #179

HamzaJugon · 2024-10-23T12:31:00Z

Fixes #

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.

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

HamzaJugon · 2024-10-23T12:34:04Z

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) {


Added @ParameterObject to the Pageable parameter, this allows Swagger to automatically generate and display pagination parameters (page, size, sort) for /unreferenced-paths. This will make it consistent with /metadata and reduces manual parameter definitions.

isn't this for requests params e.g. url/bla?myRequestParam=foo
I thought the Pageable is part of the request body in this API?

The Pageable parameter is populated from url query params. The GET request won't have a request body but the json format in swagger is misleading . .

The @ParameterObject will ensure swagger generates better documentation for the query params, we now have separate fields for Page, Size & Sort. It also resolves the Sort filter issue by making it clearer and ensuring it's optional by default, which wasn’t the case in the JSON format.

We already have this in the /metadata endpoint so not sure why it wasn't done for this one, you can see how the layout differs between endpoints in swagger.

cool thanks for explaning!

HamzaJugon · 2024-10-23T12:42:43Z

.github/workflows/release.yml

@@ -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


Prior it was possible to push to maven central from any branch which can lead to accidental pushes to maven central

HamzaJugon · 2024-10-23T12:44:28Z

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

+    errorResponse.setPath(request.getRequestURI());
+
+    return new ResponseEntity<>(errorResponse, HttpStatus.BAD_REQUEST);
+  }


Handles PropertyReferenceException (e.g., invalid sort parameters) globally and returns a custom BAD_REQUEST error response with details.

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

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

...ts/src/test/java/com/expediagroup/beekeeper/integration/api/BeekeeperApiIntegrationTest.java

Testing the request response and the errorResponses

…or/ErrorResponse.java Co-authored-by: Jay Green-Stevens <[email protected]>

…keeper/integration/api/BeekeeperApiIntegrationTest.java Co-authored-by: Jay Green-Stevens <[email protected]>

…or/BeekeeperExceptionHandler.java Co-authored-by: Jay Green-Stevens <[email protected]>

CHANGELOG.md

patduin · 2024-10-24T16:49:31Z

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

+        .timestamp(LocalDateTime.now().toString())
+        .status(HttpStatus.BAD_REQUEST.value())
+        .error(HttpStatus.BAD_REQUEST.getReasonPhrase())
+        .message("Invalid sort parameter: " + exception.getMessage())


This can be many things right doesn't always have to be invalid sort parameter. maybe we should just set the exception.message()?

Yeah, too specific.
I have updated to make it more general .message(exception.getMessage())

patduin

Nice one!

Hamza Jugon added 5 commits October 23, 2024 12:35

update controller to use ParameterObject

4edf413

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

637b622

Add test for error handling

2c11c8c

Fix workflow to only release main branch to maven central

70ca05c

Update CHANGELOG.md

8118937

HamzaJugon commented Oct 23, 2024

View reviewed changes

tidy

903469a

HamzaJugon commented Oct 23, 2024

View reviewed changes

JayGreeeen reviewed Oct 23, 2024

View reviewed changes

Hamza Jugon and others added 8 commits October 23, 2024 15:44

Add unit test for BeeekeeperExceptionHandler

592e370

Testing the request response and the errorResponses

Update BeekeeperExceptionHandlerTest.java

d4570b7

Update beekeeper-api/src/main/java/com/expediagroup/beekeeper/api/err…

fb5c76e

…or/ErrorResponse.java Co-authored-by: Jay Green-Stevens <[email protected]>

Update beekeeper-integration-tests/src/test/java/com/expediagroup/bee…

77c6a62

…keeper/integration/api/BeekeeperApiIntegrationTest.java Co-authored-by: Jay Green-Stevens <[email protected]>

Update beekeeper-api/src/main/java/com/expediagroup/beekeeper/api/err…

b840ae2

…or/BeekeeperExceptionHandler.java Co-authored-by: Jay Green-Stevens <[email protected]>

Update beekeeper-api/src/main/java/com/expediagroup/beekeeper/api/err…

93853d0

…or/BeekeeperExceptionHandler.java Co-authored-by: Jay Green-Stevens <[email protected]>

Update beekeeper-api/src/main/java/com/expediagroup/beekeeper/api/err…

15ededd

…or/BeekeeperExceptionHandler.java Co-authored-by: Jay Green-Stevens <[email protected]>

Update BeekeeperExceptionHandler.java

bce723a

JayGreeeen reviewed Oct 24, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Hamza Jugon added 2 commits October 24, 2024 11:24

Update CHANGELOG.md

55fab02

Implement builder pattern for ErrorResponse

5135664

haroldjimenez approved these changes Oct 24, 2024

View reviewed changes

JayGreeeen approved these changes Oct 24, 2024

View reviewed changes

patduin reviewed Oct 24, 2024

View reviewed changes

Hamza Jugon and others added 3 commits October 24, 2024 23:26

Make error message less specific in BeekeeperExceptionHandler

d1ee567

Update package name and fix test in BeekeeperExceptionHandler

0cda7d7

Updating integration test for newer generic error thrown

d7cb03e

patduin approved these changes Oct 25, 2024

View reviewed changes

Update CHANGELOG.md

23572a2

patduin approved these changes Oct 25, 2024

View reviewed changes

HamzaJugon merged commit 17084bd into main Oct 25, 2024
2 checks passed

HamzaJugon deleted the fix/incorrect-request-handling branch October 25, 2024 07:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bad request handling #179

Fix bad request handling #179

HamzaJugon commented Oct 23, 2024

HamzaJugon Oct 23, 2024

patduin Oct 24, 2024

HamzaJugon Oct 24, 2024

patduin Oct 25, 2024

HamzaJugon Oct 23, 2024

HamzaJugon Oct 23, 2024

patduin Oct 24, 2024

HamzaJugon Oct 24, 2024

patduin left a comment

Fix bad request handling #179

Fix bad request handling #179

Conversation

HamzaJugon commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patduin left a comment

Choose a reason for hiding this comment