Spring Boot alignment: DTOs, MapStruct, GlobalExceptionHandler, MockMvc tests, CI by Copilot · Pull Request #127 · Preponderous-Software/viron

Copilot · 2026-04-12T04:36:22Z

Agent-Logs-Url: https://github.com/Preponderous-Software/viron/sessions/470d9365-95a3-40b1-94d1-c94066faa997 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

…ollers Agent-Logs-Url: https://github.com/Preponderous-Software/viron/sessions/470d9365-95a3-40b1-94d1-c94066faa997 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Replaced direct controller method calls with MockMvc.perform() HTTP requests. Uses @SpringBootTest + @AutoConfigureMockMvc for full Spring context, @MockBean for EntityRepository, EntityFactory, EntityMapper, DbInteractions, and DbConfig. Tests cover: - Happy paths for all 9 endpoints - 404 not-found cases for getById, delete, updateName - 500 error cases where repository throws RuntimeException - 201 Created for entity creation - 204 No Content for deletion - 400 Bad Request for EntityCreationException - Empty list responses - JSON response body shape validation for DTOs and ErrorResponse Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Replace direct controller method calls with MockMvc.perform(...) using @SpringBootTest + @AutoConfigureMockMvc. Use @MockBean for GridRepository, GridMapper, DbInteractions, and DbConfig. Test all endpoints including happy paths, not-found cases (404), exception cases (500), and empty list scenarios with JSON body assertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Replace direct controller method calls with MockMvc.perform() integration tests. Use @MockBean for LocationRepository, LocationMapper, DbInteractions, and DbConfig. Test happy paths, 404 not-found, 500 error, 204 no-content, and empty list cases for all endpoints with JSON response body assertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Replace direct controller method calls with @SpringBootTest + @AutoConfigureMockMvc + MockMvc.perform() approach. Use @MockBean for all dependencies including EnvironmentRepository, EnvironmentFactory, EnvironmentMapper, DbInteractions, and DbConfig. Add tests for all endpoints including happy paths, 404 not-found cases, 400 creation exceptions, 500 runtime exceptions, empty list, 201 created status, and 204 no-content for deletion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

- createSampleData happy path: verifies 201 with correct EnvironmentDto JSON - createWorldAndPlaceEntity happy path: verifies 201 with correct EntityDto JSON - createWorldAndPlaceEntity with ServiceException: verifies 500 error response - createWorldAndPlaceEntity with no valid location: verifies 400 error response - Fix pre-existing typo in EnvironmentControllerTest MockBean import Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Copilot

Pull request overview

This PR aligns the Spring Boot API layer around DTO-based responses, MapStruct mapping, centralized exception handling, and MockMvc-based controller tests.

Changes:

Introduces DTOs + MapStruct mappers and updates controllers to return DTOs only.
Adds GlobalExceptionHandler to standardize error responses and remove controller-level try/catch blocks.
Rewrites controller tests to use @SpringBootTest + MockMvc, and updates build/test tooling dependencies (MapStruct + validation + Maven wrapper).

Reviewed changes

Copilot reviewed 25 out of 26 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/test/java/preponderous/viron/controllers/LocationControllerTest.java	Migrates location controller tests to MockMvc and asserts DTO/error JSON.
src/test/java/preponderous/viron/controllers/GridControllerTest.java	Migrates grid controller tests to MockMvc and asserts DTO/error JSON.
src/test/java/preponderous/viron/controllers/EnvironmentControllerTest.java	Migrates environment controller tests to MockMvc and asserts status/error body behavior.
src/test/java/preponderous/viron/controllers/EntityControllerTest.java	Migrates entity controller tests to MockMvc and asserts DTO/error JSON + 201/204 codes.
src/test/java/preponderous/viron/controllers/DebugControllerTest.java	Adds MockMvc coverage for debug endpoints and error paths.
src/main/java/preponderous/viron/mappers/LocationMapper.java	Adds MapStruct mapper for Location ↔ LocationDto.
src/main/java/preponderous/viron/mappers/GridMapper.java	Adds MapStruct mapper for Grid ↔ GridDto.
src/main/java/preponderous/viron/mappers/EnvironmentMapper.java	Adds MapStruct mapper for Environment ↔ EnvironmentDto.
src/main/java/preponderous/viron/mappers/EntityMapper.java	Adds MapStruct mapper for Entity ↔ EntityDto.
src/main/java/preponderous/viron/exceptions/InvalidRequestException.java	Adds a dedicated exception for invalid requests (400).
src/main/java/preponderous/viron/exceptions/GlobalExceptionHandler.java	Centralizes exception → `ErrorResponse` mapping (404/400/500 + validation).
src/main/java/preponderous/viron/dto/LocationDto.java	Adds Location DTO with OpenAPI `@Schema` metadata.
src/main/java/preponderous/viron/dto/GridDto.java	Adds Grid DTO with OpenAPI `@Schema` metadata.
src/main/java/preponderous/viron/dto/ErrorResponse.java	Adds a consistent error response shape.
src/main/java/preponderous/viron/dto/EnvironmentDto.java	Adds Environment DTO with OpenAPI `@Schema` metadata.
src/main/java/preponderous/viron/dto/EntityDto.java	Adds Entity DTO with OpenAPI `@Schema` metadata.
src/main/java/preponderous/viron/controllers/LocationController.java	Switches endpoints to DTOs, adds validation, removes try/catch.
src/main/java/preponderous/viron/controllers/GridController.java	Switches endpoints to DTOs, adds validation, removes try/catch.
src/main/java/preponderous/viron/controllers/EnvironmentController.java	Switches endpoints to DTOs, adds validation, updates status codes, relies on global handler.
src/main/java/preponderous/viron/controllers/EntityController.java	Switches endpoints to DTOs, adds validation, updates status codes, relies on global handler.
src/main/java/preponderous/viron/controllers/DebugController.java	Updates debug endpoints to DTOs, adds validation, uses `InvalidRequestException`.
pom.xml	Adds MapStruct + validation dependencies and annotation processor configuration.
mvnw.cmd	Replaces Maven wrapper Windows script.
mvnw	Replaces Maven wrapper *nix script.
.mvn/wrapper/maven-wrapper.properties	Adds Maven wrapper properties (distributionUrl + wrapperUrl).
.mvn/wrapper/maven-wrapper.jar	Adds Maven wrapper JAR (binary).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-12T04:41:46Z

    @PutMapping("/{locationId}/entity/{entityId}")
-    public ResponseEntity<Void> addEntityToLocation(@PathVariable int entityId, @PathVariable int locationId) {
-        try {
-            return locationRepository.addEntityToLocation(entityId, locationId)
-                    ? ResponseEntity.ok().build()
-                    : ResponseEntity.notFound().build();
-        } catch (Exception e) {
-            log.error("Error adding entity {} to location {}: {}", entityId, locationId, e.getMessage());
-            return ResponseEntity.internalServerError().build();
+    public ResponseEntity<Void> addEntityToLocation(@PathVariable @Min(1) int entityId, @PathVariable @Min(1) int locationId) {
+        if (!locationRepository.addEntityToLocation(entityId, locationId)) {
+            throw new NotFoundException("Location or entity not found");
        }
+        return ResponseEntity.ok().build();


These endpoints treat a false return from LocationRepository as a 404 via NotFoundException, but DbInteractions.update() also returns false on SQL exceptions. That can incorrectly map real DB failures to 404. Consider propagating DB errors as ServiceException (500) and only returning 404 when you can confirm the row(s) didn't exist (e.g., existence checks, or change repo/update layer to throw on SQL errors).

Copilot · 2026-04-12T04:41:46Z

    @DeleteMapping("/{id}")
-    public ResponseEntity<Void> deleteEntity(@PathVariable int id) {
-        try {
-            return entityRepository.deleteById(id)
-                    ? ResponseEntity.ok().build()
-                    : ResponseEntity.notFound().build();
-        } catch (Exception e) {
-            log.error("Error deleting entity {}: {}", id, e.getMessage());
-            return ResponseEntity.internalServerError().build();
+    @ResponseStatus(HttpStatus.NO_CONTENT)
+    public ResponseEntity<Void> deleteEntity(@PathVariable @Min(1) int id) {
+        if (!entityRepository.deleteById(id)) {
+            throw new NotFoundException("Entity not found with id: " + id);
        }
+        return ResponseEntity.noContent().build();
    }


@ResponseStatus(HttpStatus.NO_CONTENT) is redundant (and potentially confusing) when the method already returns an explicit ResponseEntity.noContent(). Prefer one mechanism (typically just returning ResponseEntity here) to avoid conflicting sources of truth for the status code.

Copilot · 2026-04-12T04:41:47Z

    @DeleteMapping("/{id}")
-    public ResponseEntity<Void> deleteEntity(@PathVariable int id) {
-        try {
-            return entityRepository.deleteById(id)
-                    ? ResponseEntity.ok().build()
-                    : ResponseEntity.notFound().build();
-        } catch (Exception e) {
-            log.error("Error deleting entity {}: {}", id, e.getMessage());
-            return ResponseEntity.internalServerError().build();
+    @ResponseStatus(HttpStatus.NO_CONTENT)
+    public ResponseEntity<Void> deleteEntity(@PathVariable @Min(1) int id) {
+        if (!entityRepository.deleteById(id)) {
+            throw new NotFoundException("Entity not found with id: " + id);
        }
+        return ResponseEntity.noContent().build();


deleteById() returning false is currently treated as "not found", but the underlying DbInteractions.update() also returns false on SQL exceptions. This can incorrectly return 404 for real DB failures. Consider distinguishing "0 rows affected" vs "SQL error" (e.g., have the repository throw ServiceException on SQL errors, or pre-check existence before delete and treat an unexpected false as 500).

Copilot · 2026-04-12T04:41:47Z

    @PatchMapping("/{id}/name/{name}")
-    public ResponseEntity<Void> updateEntityName(@PathVariable int id, @PathVariable String name) {
-        try {
-            return entityRepository.updateName(id, name)
-                    ? ResponseEntity.ok().build()
-                    : ResponseEntity.notFound().build();
-        } catch (Exception e) {
-            log.error("Error updating name for entity {}: {}", id, e.getMessage());
-            return ResponseEntity.internalServerError().build();
+    public ResponseEntity<Void> updateEntityName(@PathVariable @Min(1) int id, @PathVariable @NotBlank String name) {
+        if (!entityRepository.updateName(id, name)) {
+            throw new NotFoundException("Entity not found with id: " + id);
        }
+        return ResponseEntity.ok().build();


updateName() returning false is treated as "entity not found", but DbInteractions.update() also returns false on SQL exceptions. That can incorrectly return 404 for database failures. Consider doing an existence check first (like EnvironmentController does) and mapping a subsequent false/exception to 500, or reworking the repo/update layer to surface SQL errors distinctly.

dmccoystephenson · 2026-04-12T04:46:58Z

@copilot address comments and ensure that controller endpoints are returning DTOs directly and not manually wrapping DTOs in ResponseEntity objects

…e/update ops Agent-Logs-Url: https://github.com/Preponderous-Software/viron/sessions/43e28cf8-afe5-493e-a12f-a9dc027f0f46 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Copilot · 2026-04-12T04:51:26Z

@copilot address comments and ensure that controller endpoints are returning DTOs directly and not manually wrapping DTOs in ResponseEntity objects

Addressed in 71ce4b8. All controllers now return DTOs directly (e.g., EntityDto, List<EntityDto>, void) instead of wrapping in ResponseEntity. Status codes are set via @ResponseStatus annotations. Also addressed the review comments:

Removed unused HttpStatus/ResponseEntity imports
Removed redundant @ResponseStatus + ResponseEntity double-mechanism on delete
Added existence checks before deleteById/updateName in EntityController and mutating ops in LocationController to properly distinguish 404 (not found) from 500 (DB error)
Updated all affected tests

Copilot

Pull request overview

Copilot reviewed 25 out of 26 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-12T04:55:47Z

    @PutMapping("/{locationId}/entity/{entityId}")
-    public ResponseEntity<Void> addEntityToLocation(@PathVariable int entityId, @PathVariable int locationId) {
-        try {
-            return locationRepository.addEntityToLocation(entityId, locationId)
-                    ? ResponseEntity.ok().build()
-                    : ResponseEntity.notFound().build();
-        } catch (Exception e) {
-            log.error("Error adding entity {} to location {}: {}", entityId, locationId, e.getMessage());
-            return ResponseEntity.internalServerError().build();
+    public void addEntityToLocation(@PathVariable @Min(1) int entityId, @PathVariable @Min(1) int locationId) {
+        if (locationRepository.findById(locationId).isEmpty()) {
+            throw new NotFoundException("Location not found with id: " + locationId);
+        }
+        if (!locationRepository.addEntityToLocation(entityId, locationId)) {
+            throw new ServiceException("Failed to add entity " + entityId + " to location " + locationId);
        }


@PutMapping("/{locationId}/entity/{entityId}") declares path variables in the order (locationId, entityId) but the method parameters are (entityId, locationId) without explicit @PathVariable("...") names. This can bind the values incorrectly when parameter names are not retained at runtime (e.g., if -parameters isn't enabled), leading to swapping entity/location IDs. Please either (1) reorder the parameters to match the URI template, or (2) explicitly name each @PathVariable to guarantee correct binding.

Copilot · 2026-04-12T04:55:48Z

    @DeleteMapping("/{locationId}/entity/{entityId}")
-    public ResponseEntity<Void> removeEntityFromLocation(@PathVariable int entityId, @PathVariable int locationId) {
-        try {
-            return locationRepository.removeEntityFromLocation(entityId, locationId)
-                    ? ResponseEntity.ok().build()
-                    : ResponseEntity.notFound().build();
-        } catch (Exception e) {
-            log.error("Error removing entity {} from location {}: {}", entityId, locationId, e.getMessage());
-            return ResponseEntity.internalServerError().build();
+    @ResponseStatus(HttpStatus.NO_CONTENT)
+    public void removeEntityFromLocation(@PathVariable @Min(1) int entityId, @PathVariable @Min(1) int locationId) {
+        if (locationRepository.findById(locationId).isEmpty()) {
+            throw new NotFoundException("Location not found with id: " + locationId);
+        }
+        if (!locationRepository.removeEntityFromLocation(entityId, locationId)) {
+            throw new ServiceException("Failed to remove entity " + entityId + " from location " + locationId);
        }


Same potential path-variable binding issue as the PUT endpoint: @DeleteMapping("/{locationId}/entity/{entityId}") has URI variables (locationId, entityId) but the method parameters are (entityId, locationId) without explicit names. Please reorder parameters or provide @PathVariable("locationId") / @PathVariable("entityId") to prevent accidental swapping.

Copilot · 2026-04-12T04:55:48Z

+
+    @ExceptionHandler(NotFoundException.class)
+    public ResponseEntity<ErrorResponse> handleNotFoundException(NotFoundException ex) {
+        log.error("Resource not found: {}", ex.getMessage());


404s are part of expected control flow for many APIs; logging every NotFoundException at ERROR level can create noisy logs and make real server faults harder to spot. Consider logging 404s at INFO/WARN (or omitting the stack trace) and reserving ERROR for 5xx conditions.

Suggested change

log.error("Resource not found: {}", ex.getMessage());

log.warn("Resource not found: {}", ex.getMessage());

Copilot · 2026-04-12T04:55:48Z

+        log.error("Validation error: {}", ex.getMessage());
+        ErrorResponse error = new ErrorResponse(HttpStatus.BAD_REQUEST.value(), "Validation failed: " + ex.getMessage());


handleHandlerMethodValidationException returns "Validation failed: " + ex.getMessage(). ex.getMessage() can include framework/internal details (method signatures, parameter names/values) and can be very verbose. Consider returning a stable, user-facing message (optionally with a list of validation errors) and log the detailed exception server-side instead.

Suggested change

log.error("Validation error: {}", ex.getMessage());

ErrorResponse error = new ErrorResponse(HttpStatus.BAD_REQUEST.value(), "Validation failed: " + ex.getMessage());

log.error("Validation error during handler method validation", ex);

ErrorResponse error = new ErrorResponse(HttpStatus.BAD_REQUEST.value(), "Validation failed");

Copilot · 2026-04-12T04:55:48Z

 # ----------------------------------------------------------------------------
-# Apache Maven Wrapper startup batch script, version 3.3.2
+# Apache Maven Wrapper startup batch script, version 3.3.0
+#
+# Required ENV vars:
+# ------------------
+#   JAVA_HOME - location of a JDK home dir
 #
 # Optional ENV vars
 # -----------------
-#   JAVA_HOME - location of a JDK home dir, required when download maven via java source
-#   MVNW_REPOURL - repo url base for downloading maven distribution
-#   MVNW_USERNAME/MVNW_PASSWORD - user and password for downloading maven
-#   MVNW_VERBOSE - true: enable verbose log; debug: trace the mvnw script; others: silence the output
+#   MAVEN_OPTS - parameters passed to the Java VM when running Maven
+#     e.g. to debug Maven itself, use
+#       set MAVEN_OPTS=-Xdebug -Xrunjdwp:transport=dt_socket,server=y,suspend=y,address=8000
+#   MAVEN_SKIP_RC - flag to disable loading of mavenrc files
 # ----------------------------------------------------------------------------



This file change updates the Maven Wrapper scripts and downgrades the wrapper version (3.3.2 -> 3.3.0), but the PR description is focused on Spring Boot controller/DTO alignment and tests. Please confirm these wrapper script changes are intentional and in-scope; otherwise consider reverting/splitting them into a dedicated PR to reduce review surface area.

Copilot · 2026-04-12T04:55:49Z

+@REM Apache Maven Wrapper startup batch script, version 3.3.0
+@REM
+@REM Required ENV vars:
+@REM JAVA_HOME - location of a JDK home dir
+@REM
+@REM Optional ENV vars
+@REM MAVEN_BATCH_ECHO - set to 'on' to enable the echoing of the batch commands
+@REM MAVEN_BATCH_PAUSE - set to 'on' to wait for a keystroke before ending
+@REM MAVEN_OPTS - parameters passed to the Java VM when running Maven
+@REM     e.g. to debug Maven itself, use
+@REM set MAVEN_OPTS=-Xdebug -Xrunjdwp:transport=dt_socket,server=y,suspend=y,address=8000
+@REM MAVEN_SKIP_RC - flag to disable loading of mavenrc files
+@REM ----------------------------------------------------------------------------
+
+@REM Begin all REM lines with '@' in case MAVEN_BATCH_ECHO is 'on'
+@echo off
+@REM set title of command window
+title %0
+@REM enable echoing by setting MAVEN_BATCH_ECHO to 'on'
+@if "%MAVEN_BATCH_ECHO%" == "on"  echo %MAVEN_BATCH_ECHO%
+
+@REM set %HOME% to equivalent of $HOME
+if "%HOME%" == "" (set "HOME=%HOMEDRIVE%%HOMEPATH%")
+
+@REM Execute a user defined script before this one
+if not "%MAVEN_SKIP_RC%" == "" goto skipRcPre
+@REM check for pre script, once with legacy .bat ending and once with .cmd ending
+if exist "%USERPROFILE%\mavenrc_pre.bat" call "%USERPROFILE%\mavenrc_pre.bat" %*
+if exist "%USERPROFILE%\mavenrc_pre.cmd" call "%USERPROFILE%\mavenrc_pre.cmd" %*
+:skipRcPre
+
+@setlocal
+
+set ERROR_CODE=0
+
+@REM To isolate internal variables from possible post scripts, we use another setlocal
+@setlocal
+
+@REM ==== START VALIDATION ====
+if not "%JAVA_HOME%" == "" goto OkJHome
+
+echo. >&2
+echo Error: JAVA_HOME not found in your environment. >&2
+echo Please set the JAVA_HOME variable in your environment to match the >&2
+echo location of your Java installation. >&2
+echo. >&2
+goto error
+
+:OkJHome
+if exist "%JAVA_HOME%\bin\java.exe" goto init
+
+echo. >&2
+echo Error: JAVA_HOME is set to an invalid directory. >&2
+echo JAVA_HOME = "%JAVA_HOME%" >&2
+echo Please set the JAVA_HOME variable in your environment to match the >&2
+echo location of your Java installation. >&2
+echo. >&2


The new mvnw.cmd wrapper script now requires JAVA_HOME to be set and is a different wrapper version than before. That can be a breaking change for Windows contributors/CI environments that relied on java on PATH. If this wasn't intentional, consider restoring the previous wrapper behavior/version or documenting the new requirement.

dmccoystephenson · 2026-04-12T04:56:44Z

@copilot address comments

…w changes Agent-Logs-Url: https://github.com/Preponderous-Software/viron/sessions/0484260a-0b05-45d5-a3ad-1626f917f0da Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Copilot · 2026-04-12T05:00:17Z

@copilot address comments

Addressed in d584877:

Added explicit @PathVariable("entityId") / @PathVariable("locationId") names on LocationController PUT/DELETE to prevent binding mismatches
Changed NotFoundException logging from log.error → log.warn to reduce noise
Replaced verbose ex.getMessage() in HandlerMethodValidationException handler with a stable "Validation failed" message
Reverted unintentional mvnw/mvnw.cmd wrapper changes back to their original state

Copilot

Pull request overview

Copilot reviewed 23 out of 24 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

src/main/java/preponderous/viron/controllers/DebugController.java:63

grids.getFirst() will throw if gridService.getGridsInEnvironment(...) returns an empty list, which currently becomes a generic 500. Consider explicitly checking for an empty result and throwing a domain exception with a clear message (or returning a 4xx) before dereferencing the first element.

        List<Grid> grids = gridService.getGridsInEnvironment(environment.getEnvironmentId());
        Grid grid = grids.getFirst();
        List<Location> locations = locationService.getLocationsInGrid(grid.getGridId());

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-13T01:12:31Z

+
+    @ExceptionHandler(InvalidRequestException.class)
+    public ResponseEntity<ErrorResponse> handleInvalidRequestException(InvalidRequestException ex) {
+        log.error("Invalid request: {}", ex.getMessage());


This handler logs client-caused 400s as ERROR. To reduce noisy/error-level logs (and align with the NotFound handler logging at WARN), consider logging invalid requests at WARN/INFO instead of ERROR.

Suggested change

log.error("Invalid request: {}", ex.getMessage());

log.warn("Invalid request: {}", ex.getMessage());

Copilot · 2026-04-13T01:12:32Z

+        List<Integer> entityIds = environmentRepository.findEntityIdsByEnvironmentId(id);
+        List<Integer> locationIds = environmentRepository.findLocationIdsByEnvironmentId(id);
+        List<Integer> gridIds = environmentRepository.findGridIdsByEnvironmentId(id);

-            for (int locationId : locationIds) {
-                if (!environmentRepository.deleteLocationGrid(locationId)) {
-                    log.error("Error deleting location_grid with location id {}", locationId);
-                    return ResponseEntity.internalServerError().build();
-                }
+        // Delete associations
+        for (int entityId : entityIds) {
+            if (!environmentRepository.deleteEntityLocation(entityId)) {
+                throw new ServiceException("Error deleting entity_location with entity id " + entityId);
            }


deleteEnvironment performs a multi-step delete across several tables without any transactional boundary. If a later step fails (throws ServiceException), earlier deletions remain applied, potentially leaving the system in a partially-deleted state. Consider moving this logic into a service layer and executing it within a single transaction (or otherwise ensuring rollback/compensation).

Copilot · 2026-04-13T01:12:32Z

+    private final EntityMapper entityMapper;
+    private final EnvironmentMapper environmentMapper;

    List<String> entityNamePool = new ArrayList<>(Arrays.asList("Tom", "Jerry", "Spike", "Tyke", "Nibbles", "Butch", "Tuffy", "Lightning", "Mammy", "Quacker", "Toodles", "Droopy", "Screwy", "Meathead", "George", "Dripple", "McWolf"));


entityNamePool is currently package-private and mutable. Making it private final (and ideally unmodifiable) would prevent accidental mutation from other classes and make the controller’s behavior more predictable.

Copilot · 2026-04-13T01:12:32Z

+    public EntityDto createWorldAndPlaceEntity(@PathVariable @NotBlank String environmentName) {
        // create an environment
        int numGrids = 1;
        int gridSize = 5;


This endpoint assumes a grid was created/fetched successfully. A few lines below it does Grid grid = grids.get(0);, which will throw if gridService.getGridsInEnvironment(...) returns an empty list and will surface as a generic 500. Consider explicitly checking for an empty result and throwing a controlled exception/message before indexing.

dmccoystephenson · 2026-04-13T01:21:30Z

@copilot You are a coding agent tasked with resolving all open comments and review threads
on this pull request. Work through every comment systematically and apply the
appropriate code changes.

Instructions

1. Discover all comments

Read every review thread, inline comment, and general PR comment
Include comments from: Copilot, human reviewers, and any other bots
Do not skip comments marked as suggestions — apply them if they make sense

2. For each comment, do the following:

Understand what is being requested or flagged
Locate the relevant file(s) and line(s)
Apply the fix directly in code
If a Copilot suggestion diff is provided, apply it (or an equivalent fix)
If the comment is ambiguous, use your best judgment and reply in the PR thread or review comment explaining what you did; only add a code comment when explicitly requested or when future maintainers would benefit from it
After addressing a comment, reply to the thread summarising what you did and mark it as resolved if your environment supports it

3. Types of issues to handle:

Code quality / style issues (naming, formatting, readability)
Logic bugs or edge cases flagged by reviewers
Security concerns (e.g. input validation, secrets, injection risks)
Performance suggestions
Missing or incomplete error handling
Test coverage gaps
Documentation / comment improvements
Dependency or import issues

4. Do NOT:

Dismiss or ignore any comment without acting on it — note that "acting" can also mean replying to the thread with a rationale (e.g., won't fix, already addressed elsewhere) and leaving the code unchanged when appropriate
Leave TODO comments as a substitute for fixing the issue
Break existing tests while addressing comments
Change functionality beyond the scope of what the comment asks for

After making changes

Ensure all existing tests still pass
Add or update tests if a comment explicitly asks for them, or if a bug fix warrants it
Double-check that every open thread has been addressed, replied to, and resolved

Context

This PR may contain Copilot-generated review comments — treat these with the same priority as human comments
If two comments conflict with each other, prefer the human reviewer's intent and document the conflict and resolution in the PR conversation or commit message, not in a code comment
Commit changes in logical groups if there are many unrelated fixes

…id checks, tests Agent-Logs-Url: https://github.com/Preponderous-Software/viron/sessions/c38d0021-36ad-4fc0-b9f9-4b904d0861d0 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Agent-Logs-Url: https://github.com/Preponderous-Software/viron/sessions/c38d0021-36ad-4fc0-b9f9-4b904d0861d0 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Copilot · 2026-04-13T01:26:47Z

@copilot You are a coding agent tasked with resolving all open comments and review threads
on this pull request. Work through every comment systematically and apply the
appropriate code changes....

Addressed all open review comments in 01de1b9 and de0f187:

GlobalExceptionHandler:39 — Changed InvalidRequestException logging from log.error → log.warn to align with the NotFoundException handler and reduce noise for client-caused 400s.
DebugController:39 — Made entityNamePool private final using List.of(...) (immutable).
DebugController:96-99 — Added empty grid list checks before indexing in both createSampleData and createWorldAndPlaceEntity, throwing InvalidRequestException with a clear message. Added 2 tests for the empty-grids edge case. Also fixed inconsistent get(0) vs getFirst() usage.
EnvironmentController:80-88 — The multi-step delete transactional boundary concern is valid but requires refactoring the DB layer. The project uses raw JDBC via DbInteractions with java.sql.Connection — not a Spring-managed DataSource — so @Transactional won't function without broader infrastructure changes. This is tracked as a known limitation.

139 tests, 0 failures.

dmccoystephenson · 2026-04-13T01:27:49Z

@copilot add CI checks to ensure that compilation is successful and tests pass

Agent-Logs-Url: https://github.com/Preponderous-Software/viron/sessions/1535fdca-59f0-45d5-91fa-8fd7e4954198 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Copilot · 2026-04-13T01:29:41Z

@copilot add CI checks to ensure that compilation is successful and tests pass

Added in 57bb7e7. Created .github/workflows/ci.yml that runs on pushes to main and pull requests targeting main. It sets up JDK 21 (Temurin), compiles with mvn compile -B, and runs tests with mvn test -B. Maven dependencies are cached via actions/setup-java.

Copilot

Pull request overview

Copilot reviewed 24 out of 25 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-16T03:57:06Z

+        log.error("Entity creation failed: {}", ex.getMessage());
+        ErrorResponse error = new ErrorResponse(HttpStatus.BAD_REQUEST.value(), ex.getMessage());
+        return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(error);
+    }
+
+    @ExceptionHandler(EnvironmentCreationException.class)
+    public ResponseEntity<ErrorResponse> handleEnvironmentCreationException(EnvironmentCreationException ex) {
+        log.error("Environment creation failed: {}", ex.getMessage());


The handlers for EntityCreationException and EnvironmentCreationException return HTTP 400, but they currently log at ERROR. Since these are client/request-level failures (not server errors), this will inflate error logs and may trigger alerting. Consider logging at WARN (or INFO) instead, consistent with the other 400 handlers in this class.

Suggested change

log.error("Entity creation failed: {}", ex.getMessage());

ErrorResponse error = new ErrorResponse(HttpStatus.BAD_REQUEST.value(), ex.getMessage());

return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(error);

}

@ExceptionHandler(EnvironmentCreationException.class)

public ResponseEntity<ErrorResponse> handleEnvironmentCreationException(EnvironmentCreationException ex) {

log.error("Environment creation failed: {}", ex.getMessage());

log.warn("Entity creation failed: {}", ex.getMessage());

ErrorResponse error = new ErrorResponse(HttpStatus.BAD_REQUEST.value(), ex.getMessage());

return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(error);

}

@ExceptionHandler(EnvironmentCreationException.class)

public ResponseEntity<ErrorResponse> handleEnvironmentCreationException(EnvironmentCreationException ex) {

log.warn("Environment creation failed: {}", ex.getMessage());

Copilot · 2026-04-16T03:57:06Z

+                .map(error -> error.getField() + ": " + error.getDefaultMessage())
+                .reduce((a, b) -> a + "; " + b)
+                .orElse("Validation failed");
+        log.error("Validation error: {}", message);


MethodArgumentNotValidException represents a 400 response, but this handler logs it at ERROR. To avoid treating validation failures as server errors (and to align with the WARN-level logging used for other 400s here), switch this to log.warn (and consider not logging the full aggregated message at high severity if it may contain user input).

Suggested change

log.error("Validation error: {}", message);

log.warn("Validation error: {}", message);

Copilot · 2026-04-16T03:57:07Z

+
+    @ExceptionHandler(HandlerMethodValidationException.class)
+    public ResponseEntity<ErrorResponse> handleHandlerMethodValidationException(HandlerMethodValidationException ex) {
+        log.error("Validation error during handler method validation", ex);


HandlerMethodValidationException is mapped to HTTP 400 in this handler, but it is logged at ERROR with the full exception/stack trace. Consider logging at WARN (and/or without the stack trace) to avoid noisy error logs for routine validation failures.

Suggested change

log.error("Validation error during handler method validation", ex);

log.warn("Validation error during handler method validation: {}", ex.getMessage());

Copilot · 2026-04-16T03:57:07Z

+        run: mvn compile -B
+
+      - name: Run tests
+        run: mvn test -B


This workflow uses mvn ..., even though the repo includes the Maven Wrapper (mvnw + .mvn/wrapper). Using ./mvnw in CI makes builds reproducible across developer machines/CI and ensures the intended Maven version is used.

Suggested change

run: mvn compile -B

- name: Run tests

run: mvn test -B

run: ./mvnw compile -B

- name: Run tests

run: ./mvnw test -B

Copilot · 2026-04-16T03:57:07Z

+    @ResponseStatus(HttpStatus.NO_CONTENT)
+    public void removeEntityFromCurrentLocation(@PathVariable @Min(1) int entityId) {
+        if (locationRepository.findByEntityId(entityId).isEmpty()) {
+            throw new NotFoundException("Entity not found with id: " + entityId);


findByEntityId(entityId) returns an Optional, so an empty result here only means the entity has no current location (or the association doesn't exist), not necessarily that the entity itself doesn't exist. The 404 message "Entity not found" is likely misleading; consider changing it to something like "Location not found for entity: {id}" (or introducing a dedicated existence check against an EntityRepository if you really want to assert the entity doesn't exist).

Suggested change

throw new NotFoundException("Entity not found with id: " + entityId);

throw new NotFoundException("Location not found for entity: " + entityId);

dmccoystephenson · 2026-04-16T04:02:02Z

@copilot You are a coding agent tasked with resolving all open comments and review threads
on this pull request. Work through every comment systematically and apply the
appropriate code changes.

Instructions

1. Discover all comments

Read every review thread, inline comment, and general PR comment
Include comments from: Copilot, human reviewers, and any other bots
Do not skip comments marked as suggestions — apply them if they make sense

2. For each comment, do the following:

Understand what is being requested or flagged
Locate the relevant file(s) and line(s)
Apply the fix directly in code
If a Copilot suggestion diff is provided, apply it (or an equivalent fix)
If the comment is ambiguous, use your best judgment and reply in the PR thread or review comment explaining what you did; only add a code comment when explicitly requested or when future maintainers would benefit from it
After addressing a comment, reply to the thread summarising what you did and mark it as resolved if your environment supports it

3. Types of issues to handle:

Code quality / style issues (naming, formatting, readability)
Logic bugs or edge cases flagged by reviewers
Security concerns (e.g. input validation, secrets, injection risks)
Performance suggestions
Missing or incomplete error handling
Test coverage gaps
Documentation / comment improvements
Dependency or import issues

4. Do NOT:

Dismiss or ignore any comment without acting on it — note that "acting" can also mean replying to the thread with a rationale (e.g., won't fix, already addressed elsewhere) and leaving the code unchanged when appropriate
Leave TODO comments as a substitute for fixing the issue
Break existing tests while addressing comments
Change functionality beyond the scope of what the comment asks for

After making changes

Ensure all existing tests still pass
Add or update tests if a comment explicitly asks for them, or if a bug fix warrants it
Double-check that every open thread has been addressed, replied to, and resolved

Context

This PR may contain Copilot-generated review comments — treat these with the same priority as human comments
If two comments conflict with each other, prefer the human reviewer's intent and document the conflict and resolution in the PR conversation or commit message, not in a code comment
Commit changes in logical groups if there are many unrelated fixes

…ssage Agent-Logs-Url: https://github.com/Preponderous-Software/viron/sessions/7fbc4fa1-5d0b-482e-a2e1-0521306c1614 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Copilot · 2026-04-16T04:05:41Z

@copilot You are a coding agent tasked with resolving all open comments and review threads
on this pull request. Work through every comment systematically and apply the
appropriate code changes....

Addressed all 5 open review threads in 22af906:

GlobalExceptionHandler:25-32 — Changed EntityCreationException and EnvironmentCreationException logging from log.error → log.warn since they're 400s.
GlobalExceptionHandler:64 — Changed MethodArgumentNotValidException logging from log.error → log.warn.
GlobalExceptionHandler:71 — Changed HandlerMethodValidationException logging from log.error (with stack trace) → log.warn (message only).
ci.yml:24-27 — Switched from mvn to ./mvnw for reproducible builds using the repo's Maven Wrapper.
LocationController:84 — Fixed misleading "Entity not found with id:" → "Location not found for entity:" since findByEntityId returns a location association, not entity existence. Updated the corresponding test.

Copilot AI and others added 7 commits April 12, 2026 04:06

Initial plan for Spring Boot alignment task

361ad8b

Agent-Logs-Url: https://github.com/Preponderous-Software/viron/sessions/470d9365-95a3-40b1-94d1-c94066faa997 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Add DTOs, MapStruct mappers, GlobalExceptionHandler, @validated contr…

6877ac8

…ollers Agent-Logs-Url: https://github.com/Preponderous-Software/viron/sessions/470d9365-95a3-40b1-94d1-c94066faa997 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Copilot AI assigned Copilot and dmccoystephenson Apr 12, 2026

Copilot created this pull request from a session on behalf of dmccoystephenson April 12, 2026 04:36 View session

dmccoystephenson requested a review from Copilot April 12, 2026 04:36

Copilot started reviewing on behalf of dmccoystephenson April 12, 2026 04:37 View session

Copilot AI reviewed Apr 12, 2026

View reviewed changes

Copilot started work on behalf of dmccoystephenson April 12, 2026 04:47 View session

Return DTOs directly from controllers, add existence checks for delet…

71ce4b8

…e/update ops Agent-Logs-Url: https://github.com/Preponderous-Software/viron/sessions/43e28cf8-afe5-493e-a12f-a9dc027f0f46 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

dmccoystephenson marked this pull request as ready for review April 12, 2026 04:51

dmccoystephenson requested a review from Copilot April 12, 2026 04:51

Copilot AI requested a review from dmccoystephenson April 12, 2026 04:52

Copilot finished work on behalf of dmccoystephenson April 12, 2026 04:52

Copilot started reviewing on behalf of dmccoystephenson April 12, 2026 04:52 View session

Copilot AI reviewed Apr 12, 2026

View reviewed changes

Copilot started work on behalf of dmccoystephenson April 12, 2026 04:56 View session

Fix @PathVariable binding, log levels, validation message, revert mvn…

d584877

…w changes Agent-Logs-Url: https://github.com/Preponderous-Software/viron/sessions/0484260a-0b05-45d5-a3ad-1626f917f0da Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Copilot finished work on behalf of dmccoystephenson April 12, 2026 05:00

dmccoystephenson requested a review from Copilot April 13, 2026 01:08

Copilot started reviewing on behalf of dmccoystephenson April 13, 2026 01:08 View session

Copilot AI reviewed Apr 13, 2026

View reviewed changes

Copilot started work on behalf of dmccoystephenson April 13, 2026 01:21 View session

Copilot AI and others added 2 commits April 13, 2026 01:24

Use consistent getFirst() for grid list access in DebugController

de0f187

Agent-Logs-Url: https://github.com/Preponderous-Software/viron/sessions/c38d0021-36ad-4fc0-b9f9-4b904d0861d0 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Copilot finished work on behalf of dmccoystephenson April 13, 2026 01:27

Copilot started work on behalf of dmccoystephenson April 13, 2026 01:28 View session

Add CI workflow for compilation and test checks

57bb7e7

Agent-Logs-Url: https://github.com/Preponderous-Software/viron/sessions/1535fdca-59f0-45d5-91fa-8fd7e4954198 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Copilot AI changed the title ~~Spring Boot alignment: DTOs, MapStruct, GlobalExceptionHandler, MockMvc tests~~ Spring Boot alignment: DTOs, MapStruct, GlobalExceptionHandler, MockMvc tests, CI Apr 13, 2026

Copilot finished work on behalf of dmccoystephenson April 13, 2026 01:30

dmccoystephenson requested a review from Copilot April 16, 2026 03:54

Copilot started reviewing on behalf of dmccoystephenson April 16, 2026 03:54 View session

Copilot AI reviewed Apr 16, 2026

View reviewed changes

Copilot started work on behalf of dmccoystephenson April 16, 2026 04:02 View session

Log all 400 handlers at WARN, use ./mvnw in CI, fix location error me…

22af906

…ssage Agent-Logs-Url: https://github.com/Preponderous-Software/viron/sessions/7fbc4fa1-5d0b-482e-a2e1-0521306c1614 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>

Copilot finished work on behalf of dmccoystephenson April 16, 2026 04:06

	log.error("Resource not found: {}", ex.getMessage());
	log.warn("Resource not found: {}", ex.getMessage());

		log.error("Validation error: {}", ex.getMessage());
		ErrorResponse error = new ErrorResponse(HttpStatus.BAD_REQUEST.value(), "Validation failed: " + ex.getMessage());

	log.error("Invalid request: {}", ex.getMessage());
	log.warn("Invalid request: {}", ex.getMessage());

	log.error("Validation error: {}", message);
	log.warn("Validation error: {}", message);

	log.error("Validation error during handler method validation", ex);
	log.warn("Validation error during handler method validation: {}", ex.getMessage());

	throw new NotFoundException("Entity not found with id: " + entityId);
	throw new NotFoundException("Location not found for entity: " + entityId);

Conversation

Copilot AI commented Apr 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

dmccoystephenson commented Apr 12, 2026

Uh oh!

Copilot AI commented Apr 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

dmccoystephenson commented Apr 12, 2026

Uh oh!

Copilot AI commented Apr 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

dmccoystephenson commented Apr 13, 2026

Instructions

After making changes

Context

Uh oh!

Copilot AI commented Apr 13, 2026

Uh oh!

dmccoystephenson commented Apr 13, 2026

Uh oh!

Copilot AI commented Apr 13, 2026

Uh oh!

Copilot AI left a comment

Copilot AI commented Apr 12, 2026 •

edited

Loading