Merge dev to prod #9275
Merge dev to prod #9275
Conversation
…nAI response information; updated tests for OpenAI functionality
…lasses, added extra checks to service level methods
…rs via environment variable
…ty-user-refactor-changelogs' into 8321/split-greencity-and-greencity-user-refactor-changelogs
…logs' of https://github.com/ita-social-projects/GreenCity into 8321/split-greencity-and-greencity-user-refactor-changelogs
* Add filter for hidden EcoNews entries * Always use predicate-based filtering for econews to filter out hidden news * Update predicates for filtering EcoNews in EcoNewsServiceImplTest.java
…k; created tables and entities for tag's coherence calculations
…ng EnableCaching annotations
…relevant news obtaining process in cache
…ent-notification Updated notification message for event comment
* Update Java dependencies * Update Go dependencies
…#9073) * removed chatId field and chat_rooms table in query from User (past removed functionality)
…k-functionality
…er-extrnal-secret Transferred appropriate config files for cluster to GreenCityUser
…pecifications (#9015) * removed EcoNewsSearchRepo and its implementation from dao module; moved logic into service and used existed repo method with specification instead * removed CustomNotificationRepo and its implementation from dao module; moved logic into service and used existed repo method with specification instead * removed CustomUserRepo and its implementation from dao module; moved logic into service instead * removed EventSearchRepo and its implementation from dao module; moved logic into service and used existed repo method with specification instead * removed PlaceSearchRepo and its implementation from dao module; moved logic into service and used existed repo method with specification instead * moved author predicate creation method from general MySpecification interface to EcoNewsSpecification where it was only used
…9099) * changed all find requests to GreenCityUser by id to requests by email; changed request parameter email to path variable parameter * fixed exception when getting user as friend (removed chat id and chat room mentions) * updated GreenCityRemoteClient links after changing user synchronization from id to email
…nts with comments (#9160) * updated CurrentUserId parameters with required=false for permitted all endpoints * fixed exception with null value in event's filter dto
* split user status: added ACTIVATED, DEACTIVATED, BLOCKED and DELETED statuses * added new endpoint for updating user's email in external service * updated encoding filter in UserRemoteClientConfig * fixed bug with empty tags for relevance request
* added role field to UserFilterDto and disabled select in management page * updated UserFilterDto creation in UserServiceImpl
* added plus symbol encoding interceptor to old RestTemplateConfig * moved duplicated code in utility class with package access
* clean pom.xml. update various dependencies. * cleanup * remove unused class * sonarqube * bump ALL dependencies * fix test behavior * refactor test, remove junit4 dependency * restore web api endpoints * resolve dev branch conflict * remove redundant test * format files * move liquibase dependency to core and remove jpamodelgen from deps
* Fix input field names for achievement management in Ukrainian and English * Refactor sorting mechanism in achievement management to use pageable sort attributes * Refactor tests * improve test coverage and reduce code duplication * format code
* deleted check for empty to do list * deleted test * Removed unused constant --------- Co-authored-by: BohdanLys <[email protected]>
…-env-variables-from-azure-key-vault Feature #1843 obtain env variables from azure key vault
* refactored enums and dtos from dao module to service-api
…tatistics-server-crash-when-exporting-user-rating-statistics-to-excel Fixed problem with server crash while downloading excel file
…-prod-keyvault Feature #1843 renamed prod keyvault
WalkthroughComprehensive platform modernization upgrading Java 11 to 21, migrating Swagger 2 to OpenAPI 3, and transitioning from javax to jakarta validation. Introduces user identity resolution via custom annotations, refactors comment/event APIs for consistency, adds new AI and subscription features, and consolidates multiple controllers while removing legacy endpoints. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as REST Controller
participant Resolver as UserIdArgumentResolver
participant JwtTool as JwtTool
participant Service as Business Service
participant DB as Database
Client->>Controller: GET /users/{id}<br/>(with JWT in header)
activate Controller
Controller->>Resolver: [Resolve `@CurrentUserId` parameter]
activate Resolver
Resolver->>JwtTool: Extract JWT from request
activate JwtTool
JwtTool-->>Resolver: Return userId from claims
deactivate JwtTool
Resolver-->>Controller: Return userId (Long)
deactivate Resolver
rect rgb(200, 220, 255)
note right of Controller: User identity<br/>resolved and injected
Controller->>Service: processRequest(userId, ...)
end
activate Service
Service->>DB: Query user data
activate DB
DB-->>Service: Return result
deactivate DB
Service-->>Controller: Return data
deactivate Service
Controller-->>Client: 200 OK + Response
deactivate Controller
sequenceDiagram
participant Client as Client
participant Controller as CommentController
participant Multipart as Multipart Resolver
participant Validator as ImageArrayValidator
participant Service as CommentService
participant FileService as File Service
Client->>Controller: POST /comments<br/>(request + images + locale)
activate Controller
note over Multipart: Extract multipart parts
Controller->>Multipart: Resolve `@RequestPart` parts
activate Multipart
Multipart-->>Controller: AddCommentDtoRequest + MultipartFile[]
deactivate Multipart
rect rgb(200, 220, 255)
note right of Validator: Validate image types<br/>and count (max 5)
Controller->>Validator: `@ImageArrayValidation` check
end
Controller->>Service: Create comment with images
activate Service
Service->>FileService: Upload images
activate FileService
FileService-->>Service: Image URLs
deactivate FileService
Service->>Service: Associate images with comment
Service-->>Controller: CommentDto with images
deactivate Service
Controller-->>Client: 201 Created + CommentDto
deactivate Controller
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Key areas requiring detailed attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
core/src/main/java/greencity/controller/PlaceCommentController.java (1)
136-140: Critical: Missing parameter binding annotation.The
Long idparameter lacks a binding annotation (@PathVariableor@RequestParam), which means Spring will not bind any value to it. This will result in a nullidbeing passed to the service, causing the delete operation to fail.Apply this diff to add the missing annotation (using path variable as the standard REST pattern):
- @DeleteMapping("comments") - public ResponseEntity<Object> delete(Long id) { + @DeleteMapping("comments/{id}") + public ResponseEntity<Object> delete(@PathVariable Long id) { placeCommentService.deleteById(id); return ResponseEntity.ok().build();Alternatively, if a query parameter is preferred:
@DeleteMapping("comments") - public ResponseEntity<Object> delete(Long id) { + public ResponseEntity<Object> delete(@RequestParam Long id) { placeCommentService.deleteById(id); return ResponseEntity.ok().build();core/src/main/java/greencity/dto/breaktime/BreakTimeDto.java (1)
15-17: Critical: @EqualsAndHashCode excludes all fields, breaking equality semantics.The
@EqualsAndHashCode(exclude = {"startTime", "endTime"})annotation excludes the only two fields in this class, which means allBreakTimeDtoinstances will be considered equal regardless of their time values. This breaks:
- Set/Map collections (all instances treated as duplicates)
- Logical equality checks
- Hash-based data structures
Apply this diff to fix the issue by removing the exclusion:
@Data @NoArgsConstructor @AllArgsConstructor @Builder -@EqualsAndHashCode(exclude = {"startTime", "endTime"}) +@EqualsAndHashCode public class BreakTimeDto {Alternatively, if there's a specific reason to exclude time fields, please clarify the design intent and consider including other identifying fields in equality checks.
.github/workflows/main.yml (1)
8-23: Remove duplicateworkflow_dispatchdeclaration.The
workflow_dispatchtrigger is declared twice (line 14 and line 23), which creates a malformed YAML structure and may cause the workflow to fail parsing.Apply this diff to remove the duplicate:
on: # Triggers the workflow on push or pull request events but only for the dev branch push: branches: [ dev ] pull_request: branches: [ dev ] workflow_dispatch: env: repository: greencity userRepoName: core onbootJarName: app.jar dockerRepoName: skyere/greencity - # Allows you to run this workflow manually from the Actions tab - workflow_dispatch:core/src/main/java/greencity/security/providers/JwtAuthenticationProvider.java (1)
42-59: Optimize JWT parsing to avoid redundant token verification.The JWT token is parsed twice: once to extract the email (lines 44-48) and again to extract authorities (lines 50-54). Each parsing involves cryptographic signature verification, which is unnecessary overhead.
Apply this diff to parse the token once:
@Override public Authentication authenticate(Authentication authentication) { SecretKey key = Keys.hmacShaKeyFor(jwtTool.getAccessTokenKey().getBytes()); - String email = Jwts.parser() + var claims = Jwts.parser() .verifyWith(key).build() .parseSignedClaims(authentication.getName()) - .getPayload() - .getSubject(); + .getPayload(); + String email = claims.getSubject(); @SuppressWarnings({"unchecked, rawtype"}) - List<String> authorities = (List<String>) Jwts.parser() - .verifyWith(key).build() - .parseSignedClaims(authentication.getName()) - .getPayload() - .get(ROLE); + List<String> authorities = (List<String>) claims.get(ROLE); return new UsernamePasswordAuthenticationToken( email, "", authorities.stream().map(SimpleGrantedAuthority::new).collect(Collectors.toList())); }
🧹 Nitpick comments (35)
core/src/main/java/greencity/controller/PlaceCommentController.java (2)
58-63: Consider using a typed response instead ofResponseEntity<Object>.The OpenAPI annotation indicates this method returns
PlaceCommentResponseDto, but the method signature uses the genericObjecttype. Using a properly typed response improves type safety and IDE support.Apply this diff to use a typed response:
- public ResponseEntity<Object> save(@PathVariable Long placeId, + public ResponseEntity<PlaceCommentResponseDto> save(@PathVariable Long placeId, @Valid @RequestBody PlaceCommentRequestDto placeCommentRequestDto, @Parameter(hidden = true) @AuthenticationPrincipal Principal principal) { return ResponseEntity - .status(HttpStatus.CREATED).body(placeCommentService.save(placeId, placeCommentRequestDto, - principal.getName())); + .status(HttpStatus.CREATED) + .body(placeCommentService.save(placeId, placeCommentRequestDto, principal.getName()));
87-89: Consider using a typed response instead ofResponseEntity<Object>.Similar to the
savemethod, this endpoint's OpenAPI annotation indicates it returnsPlaceCommentResponseDto, but the method signature uses the genericObjecttype.Apply this diff:
- public ResponseEntity<Object> getCommentById(@PathVariable Long id) { + public ResponseEntity<PlaceCommentResponseDto> getCommentById(@PathVariable Long id) { return ResponseEntity.status(HttpStatus.OK) .body(placeCommentService.findById(id));core/src/main/java/greencity/controller/HabitInvitationController.java (1)
70-76: Consider using PATCH instead of DELETE for semantic correctness.The javadoc (lines 54-55) states this operation "changes status from REQUEST to REJECTED," which is a state transition rather than a resource deletion. The DELETE HTTP method semantically implies removing a resource, while PATCH is designed for partial updates and state changes.
For consistency and RESTful semantics, consider changing this endpoint to use PATCH:
- @DeleteMapping("/{invitationId}/reject") + @PatchMapping("/{invitationId}/reject") public ResponseEntity<Void> rejectHabitInvitation(This would also create symmetry with the
acceptHabitInvitationendpoint, which correctly uses PATCH for a similar state change operation.core/src/main/java/greencity/converters/ZonedDateTimeTypeAdapter.java (2)
15-16: Consider zero-padding the day component for consistent formatting.The pattern uses
dinstead ofdd, which means single-digit days (1-9) will serialize without zero-padding (e.g., "5" vs "05"). While parsing is forgiving and accepts both formats, serialization consistency improves readability and alignment with ISO-8601 standards.Additionally, the
::double-colon separators are non-standard. Consider whether a more conventional pattern (e.g., standard ISO format) would better serve your use case.Apply this diff if you'd like to adopt zero-padded days and standard separators:
- private static final DateTimeFormatter formatter = - DateTimeFormatter.ofPattern("d::MMM::uuuu HH::mm::ss.SSSSSSSSSXXX[VV]"); + private static final DateTimeFormatter formatter = + DateTimeFormatter.ofPattern("dd::MMM::uuuu HH::mm::ss.SSSSSSSSSXXX[VV]");Or for a more standard ISO-like format:
- private static final DateTimeFormatter formatter = - DateTimeFormatter.ofPattern("d::MMM::uuuu HH::mm::ss.SSSSSSSSSXXX[VV]"); + private static final DateTimeFormatter formatter = DateTimeFormatter.ISO_ZONED_DATE_TIME;
23-27: Consider wrapping parse exceptions with more context.The deserialize method allows
JsonParseExceptionto propagate directly. Depending on how these adapters are used, adding a try-catch block with a more descriptive error message (including the problematic input) could aid debugging.Example enhancement:
@Override public ZonedDateTime deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context) throws JsonParseException { - return ZonedDateTime.parse(json.getAsString(), formatter); + try { + return ZonedDateTime.parse(json.getAsString(), formatter); + } catch (Exception e) { + throw new JsonParseException("Failed to parse ZonedDateTime: " + json.getAsString(), e); + } }core/src/main/java/greencity/converters/LocalDateTimeTypeAdapter.java (2)
15-15: Consider zero-padding the day component for consistent formatting.Similar to
ZonedDateTimeTypeAdapter, the pattern usesdinstead ofdd, resulting in non-zero-padded single-digit days. While parsing handles both formats, consistent serialization output improves readability and aligns with common date-time formatting conventions.Apply this diff for zero-padded days:
- private static final DateTimeFormatter formatter = DateTimeFormatter.ofPattern("d::MMM::uuuu HH::mm::ss.SSSSSSSSS"); + private static final DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd::MMM::uuuu HH::mm::ss.SSSSSSSSS");Alternatively, consider ISO_LOCAL_DATE_TIME for standardization:
- private static final DateTimeFormatter formatter = DateTimeFormatter.ofPattern("d::MMM::uuuu HH::mm::ss.SSSSSSSSS"); + private static final DateTimeFormatter formatter = DateTimeFormatter.ISO_LOCAL_DATE_TIME;
22-26: Consider enhancing error messages in deserialize.As with
ZonedDateTimeTypeAdapter, wrapping parse exceptions with additional context (like the input string) can improve debugging when invalid date-time strings are encountered.Example:
@Override public LocalDateTime deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context) throws JsonParseException { - return LocalDateTime.parse(json.getAsString(), formatter); + try { + return LocalDateTime.parse(json.getAsString(), formatter); + } catch (Exception e) { + throw new JsonParseException("Failed to parse LocalDateTime: " + json.getAsString(), e); + } }core/src/main/java/greencity/controller/FavoritePlaceController.java (7)
38-46: Update Javadoc to reflect @CurrentUserId instead of Principal.Lines 39-40 still mention "Parameter principal are ignored because Spring automatically provide the Principal object," but the method now uses
@CurrentUserId Long userId. The Javadoc should be updated to reflect this change for clarity./** * Update {@link FavoritePlaceVO} name for {@link UserVO}. Parameter principal - * are ignored because Spring automatically provide the Principal object. + * is provided via the @CurrentUserId annotation. * * @param favoritePlaceDto - dto for {@link FavoritePlaceVO} entity * @param userId - current user id
65-73: Update Javadoc to remove outdated Principal references.Lines 66-68 still reference "principal" and "Principal object," but the method now uses
@CurrentUserId Long userId./** - * Find all {@link PlaceByBoundsDto} by {@link UserVO} email. Parameter - * principal are ignored because Spring automatically provide the Principal - * object . + * Find all {@link PlaceByBoundsDto} by {@link UserVO} id. * * @param userId - current user id * @return list of {@link PlaceByBoundsDto}
82-84: Consider renaming method to match its implementation.The method name
findAllByUserEmailsuggests it takes an email parameter, but it actually usesuserIdand callsfindAllByUserId(userId). Consider renaming tofindAllByUserIdfor consistency.-public ResponseEntity<List<PlaceByBoundsDto>> findAllByUserEmail( +public ResponseEntity<List<PlaceByBoundsDto>> findAllByUserId( @Parameter(hidden = true) @CurrentUserId Long userId) { return ResponseEntity.status(HttpStatus.OK).body(favoritePlaceService.findAllByUserId(userId)); }
74-80: Consider updating the operation summary for accuracy.The summary says "by user email" but the method now uses userId instead of email.
-@Operation(summary = "Get list favorite places by user email") +@Operation(summary = "Get list favorite places by user id")
87-96: Update Javadoc to remove Principal reference.Lines 89-90 still mention "Principal object," but the method now uses
@CurrentUserId Long userId./** * Delete {@link FavoritePlaceVO} by {@link UserVO} email and {@link PlaceVO} id - * Parameter principal are ignored because Spring automatically provide the - * Principal object. + * using the current user's id. * * @param placeId - {@link PlaceVO} id * @param userId - current user id
97-111: Consider updating method name and summary to reflect userId usage.The method name
deleteByUserEmailAndPlaceIdand the operation summary "by userEmail" suggest email-based identification, but the method usesuserIdand callsdeleteByUserIdAndPlaceId. Consider renaming for consistency.-@Operation(summary = "Delete favorite place by userEmail") +@Operation(summary = "Delete favorite place by userId") ... @DeleteMapping("/{placeId}") -public ResponseEntity<Long> deleteByUserEmailAndPlaceId(@PathVariable Long placeId, +public ResponseEntity<Long> deleteByUserIdAndPlaceId(@PathVariable Long placeId, @Parameter(hidden = true) @CurrentUserId Long userId) { return ResponseEntity.status(HttpStatus.OK).body(favoritePlaceService .deleteByUserIdAndPlaceId(placeId, userId)); }
114-123: Update Javadoc to remove Principal reference.Lines 116-117 still mention "Principal object," but the method now uses
@CurrentUserId Long userId./** * Controller to get {@link PlaceByBoundsDto} coordinates, id and name. - * Parameter principal is ignored because Spring automatically provide the - * Principal object. + * Uses the current user's id provided via @CurrentUserId. * * @param placeId - {@link PlaceVO} id * @param userId - current user idcore/src/main/java/greencity/controller/SubscriptionController.java (2)
64-67: UseResponseEntity<Void>instead ofResponseEntity<Object>for clearer API contract.The return type
ResponseEntity<Object>is ambiguous for API documentation and consumers. Since the endpoint returns an empty body,ResponseEntity<Void>more accurately represents the contract.Apply this diff:
- public ResponseEntity<Object> unsubscribe(@PathVariable UUID unsubscribeToken) { + public ResponseEntity<Void> unsubscribe(@PathVariable UUID unsubscribeToken) { subscriptionService.deleteSubscription(unsubscribeToken); return ResponseEntity.ok().build(); }
63-64: Consider adding@Parameterannotation for better API documentation.Adding OpenAPI's
@Parameterannotation would improve the generated API documentation by describing the path variable's purpose.+import io.swagger.v3.oas.annotations.Parameter; @DeleteMapping("/{unsubscribeToken}") -public ResponseEntity<Object> unsubscribe(@PathVariable UUID unsubscribeToken) { +public ResponseEntity<Void> unsubscribe( + @Parameter(description = "Unique token for unsubscribing", required = true) + @PathVariable UUID unsubscribeToken) {core/src/main/java/greencity/controller/HabitStatisticController.java (2)
198-217: LGTM! Follows established external service pattern.This endpoint aligns with the pattern used in
EcoNewsControllerandUserControllerfor external service integration. The email-based lookup is appropriate for external consumers.Consider adding validation to the email parameter for robustness:
- public ResponseEntity<Long> findAmountOfAcquiredHabits(@RequestParam String email) { + public ResponseEntity<Long> findAmountOfAcquiredHabits( + @RequestParam @jakarta.validation.constraints.Email @jakarta.validation.constraints.NotBlank String email) {
241-261: LGTM! Consistent external service endpoint.This endpoint maintains consistency with the newly added acquired habits external endpoint and follows the established codebase pattern.
As with the previous external endpoint, consider adding email validation:
- public ResponseEntity<Long> findAmountOfHabitsInProgress(@RequestParam String email) { + public ResponseEntity<Long> findAmountOfHabitsInProgress( + @RequestParam @jakarta.validation.constraints.Email @jakarta.validation.constraints.NotBlank String email) {core/src/main/java/greencity/controller/ToDoListItemController.java (1)
208-215: Simplify regex quantifier to match codebase convention.Line 211: The regex pattern uses a possessive quantifier
++, which is the only occurrence in the codebase. All other@Patternvalidations use standard greedy quantifiers. For consistency, change the pattern to^\\d+(,\\d+)+$(removing the second+):@Pattern( regexp = "^\\d+(,\\d+)+$", message = ValidationConstants.BAD_COMMA_SEPARATED_NUMBERS)Both patterns match comma-separated numbers identically, but using the standard greedy quantifier maintains alignment with the established codebase style and improves readability.
core/src/main/java/greencity/annotations/ValidAccessToken.java (1)
11-11: Consider following Bean Validation convention for null handling.The
AccessTokenValidatorcurrently returnsfalsefor null tokens, coupling null-checking with format validation. Bean Validation convention suggests that constraint validators should returntruefor null values and rely on@NotNull(applied separately) when null is not allowed. This separation provides better flexibility for optional fields that must match the pattern only when present.To implement this, the validator logic could be updated:
@Override public boolean isValid(String accessToken, ConstraintValidatorContext constraintValidatorContext) { if (accessToken == null) { return true; // Let @NotNull handle null validation separately } return accessToken.matches("^[a-zA-Z0-9-._~+]+$"); }Then use
@NotNull @ValidAccessTokenwhen null values should be rejected.core/src/main/java/greencity/controller/FactOfTheDayController.java (1)
27-41: Align the Javadoc with the actual return type.
The method now returnsFactOfTheDayTranslationDTO, but the Javadoc still highlightsFactOfTheDayVO. That mismatch can confuse consumers (and forces the lingering VO import purely for the doc). Please update the comment (and drop the unused VO import once the doc refers to the translation DTO) so it reflects the current contract.core/src/main/java/greencity/controller/FriendController.java (6)
70-74: UseResponseEntity<Void>for bodiless responses.
ResponseEntity.status(...).build()returns aResponseEntity<Void>, so this method signature should reflect that. Declaring it asResponseEntity<ResponseEntity.BodyBuilder>not only misrepresents the payload, it also leads to unchecked assignment noise during compilation. Please switch the return type toResponseEntity<Void>(and optionally align the other bodiless endpoints for consistency). (docs.spring.io)
169-170: Drop redundant@PageableDefault.Our custom pageable resolver already seeds page = 0 / size = 20, so this annotation adds no value here (and can confuse future maintainers who think a different default is intended). Let’s remove it to rely on the central configuration instead. Based on learnings
198-199: Same note on@PageableDefault.This pageable parameter inherits the global defaults; the local annotation is redundant. Please trim it for clarity. Based on learnings
228-229: Same note on@PageableDefault.Given the shared pageable defaults, this annotation can be dropped without changing behavior. Based on learnings
351-353: Same note on@PageableDefault.You can remove this redundant annotation and rely on the centralized pageable configuration. Based on learnings
317-319: Same note on@PageableDefault.Let’s lean on the global pageable defaults and delete this redundant annotation. Based on learnings
README.md (1)
92-143: Consider improving markdown quality for images and links.The Swagger UI workflow documentation is comprehensive and helpful. However, static analysis has flagged a few markdown quality issues:
- Missing alt text on images (lines 118, 129, 132, 133)
- Non-descriptive link text "here" at line 126
- Blank line inside blockquote at line 100
Consider adding descriptive alt text to images and making link text more descriptive for better accessibility and maintainability.
Example improvements:
-6. Register user and click "Execute". The example of user data can be seen above - +6. Register user and click "Execute". The example of user data can be seen above + -you can generate one [here](https://gc-captcha-generator-2.pages.dev). +you can generate one using the [GreenCity CAPTCHA generator](https://gc-captcha-generator-2.pages.dev).core/src/main/java/greencity/controller/HabitAssignController.java (1)
379-387: Return an empty-body response explicitlyThis method always returns
.build(), so the runtime body isVoid. Keeping the signature asResponseEntity<ResponseEntity.BodyBuilder>is misleading and creates awkward type parameters for callers. Please switch the signature toResponseEntity<Void>(or simplyResponseEntity<?>) so the declared type matches the actual response.core/src/main/java/greencity/converters/UserArgumentResolver.java (1)
19-50: Remove the unusedModelMapperfieldAfter switching to
findNotDeactivatedByEmail, theModelMapperdependency is no longer used anywhere in this resolver. Dropping the field keeps the constructor leaner and avoids injecting beans you don’t need.core/src/main/java/greencity/exception/handler/ValidationExceptionDto.java (1)
29-33: Fix the constructor JavadocThe documentation still says “using the provided FieldError,” but this overload now takes explicit
name/messagestrings. Please update the description so it reflects the actual parameters.core/src/main/java/greencity/exception/helper/ExceptionResponseBuilder.java (1)
9-18: Remove unnecessary@Componentannotation.This is a utility class with only static methods and a private constructor. The
@Componentannotation causes Spring to unnecessarily create a bean instance that will never be used.Apply this diff:
-@Component public class ExceptionResponseBuilder { private static final String TIMESTAMP = "timestamp"; private static final String STATUS = "status";core/src/main/java/greencity/annotations/ImageArrayValidation.java (1)
43-43: Consider removing duplicate MIME type.The
allowedTypesarray includes both"image/jpeg"and"image/jpg". The official MIME type isimage/jpeg, and whileimage/jpgmay appear in some contexts, it's non-standard. Consider keeping onlyimage/jpegunless there's a specific requirement for backwards compatibility.Apply this diff if you choose to remove the duplicate:
- String[] allowedTypes() default {"image/jpeg", "image/png", "image/jpg", "image/gif"}; + String[] allowedTypes() default {"image/jpeg", "image/png", "image/gif"};az-cd-pipeline.yml (1)
39-49: Consider using a more robust approach for XML generation.The script uses
echocommands to generate the Mavensettings.xmlfile. While this works for typical tokens, it could fail if the token contains XML special characters (<,>,&, quotes).If you want to make this more robust, consider using a template file or a tool that handles XML escaping:
- task: FileTransform@1 displayName: "Generate settings.xml from template" inputs: folderPath: '$(Build.SourcesDirectory)' fileType: 'xml' targetFiles: 'settings.xml'However, since GitHub tokens are typically alphanumeric, the current approach is acceptable.
core/src/main/java/greencity/controller/AIController.java (1)
44-58: Consider renaming the method for clarity.The method name
creatingEcoNewsuses a present participle, which is slightly awkward. Consider renaming togenerateEcoNewsto match the endpoint path and make the intent clearer.- public ResponseEntity<String> creatingEcoNews(@Parameter(hidden = true) @CurrentUser UserVO userVO, + public ResponseEntity<String> generateEcoNews(@Parameter(hidden = true) @CurrentUser UserVO userVO, @RequestParam(required = false) String query) {
| - name: Set up JDK 21 | ||
| uses: actions/setup-java@v1 | ||
| with: | ||
| java-version: 11 | ||
| java-version: 21 |
There was a problem hiding this comment.
Upgrade deprecated actions/setup-java action.
The actions/setup-java@v1 action is outdated and may fail on current GitHub Actions runners. The action should be upgraded to @v3 or later.
Apply this diff:
- - name: Set up JDK 21
- uses: actions/setup-java@v1
- with:
- java-version: 21
+ - name: Set up JDK 21
+ uses: actions/setup-java@v3
+ with:
+ java-version: '21'
+ distribution: 'temurin'Note: The distribution parameter is required in v3+.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Set up JDK 21 | |
| uses: actions/setup-java@v1 | |
| with: | |
| java-version: 11 | |
| java-version: 21 | |
| - name: Set up JDK 21 | |
| uses: actions/setup-java@v3 | |
| with: | |
| java-version: '21' | |
| distribution: 'temurin' |
🧰 Tools
🪛 actionlint (1.7.8)
34-34: the runner of "actions/setup-java@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/main.yml around lines 33 to 36, update the deprecated
actions/setup-java@v1 to a current release (use actions/setup-java@v3) and add
the required distribution key for v3+; replace the uses line with
actions/setup-java@v3 and add with: java-version: 21 and distribution: 'temurin'
(or your chosen distribution) so the workflow uses the supported action version
and includes the required distribution parameter.
| .requestMatchers(HttpMethod.DELETE, | ||
| COMMENTS) | ||
| .hasAnyRole(ADMIN) | ||
| .requestMatchers(HttpMethod.GET, | ||
| ACHIEVEMENTS + "/all", | ||
| ACHIEVEMENTS + "/user-achievements/**", | ||
| USERS + "/**") | ||
| .hasAnyRole(ADMIN) | ||
| .anyRequest().permitAll()) |
There was a problem hiding this comment.
Restore the admin-only fallback rule
The new chain ends with .anyRequest().permitAll(). Previously, anything not explicitly matched fell through to hasAnyRole(ADMIN), ensuring we didn't silently expose new endpoints. This change turns every unmatched path into a public endpoint, which is a serious security regression. Please restore the restrictive fallback.
- .anyRequest().permitAll())
+ .anyRequest().hasAnyRole(ADMIN))Based on learnings.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .requestMatchers(HttpMethod.DELETE, | |
| COMMENTS) | |
| .hasAnyRole(ADMIN) | |
| .requestMatchers(HttpMethod.GET, | |
| ACHIEVEMENTS + "/all", | |
| ACHIEVEMENTS + "/user-achievements/**", | |
| USERS + "/**") | |
| .hasAnyRole(ADMIN) | |
| .anyRequest().permitAll()) | |
| .requestMatchers(HttpMethod.DELETE, | |
| COMMENTS) | |
| .hasAnyRole(ADMIN) | |
| .requestMatchers(HttpMethod.GET, | |
| ACHIEVEMENTS + "/all", | |
| ACHIEVEMENTS + "/user-achievements/**", | |
| USERS + "/**") | |
| .hasAnyRole(ADMIN) | |
| .anyRequest().hasAnyRole(ADMIN)) |
🤖 Prompt for AI Agents
In core/src/main/java/greencity/config/SecurityConfig.java around lines 488 to
496, the chain currently ends with .anyRequest().permitAll() which makes all
unmatched endpoints public; replace that permissive fallback with the previous
restrictive rule (.anyRequest().hasAnyRole(ADMIN)) so any route not explicitly
matched requires ADMIN role, and verify the explicit public endpoints remain
listed before the final anyRequest() call to preserve the intended ordering.
| + "\t\"id\":\0,\n" | ||
| + "\t\"title\":\"string\",\n" | ||
| + "\t\"description\":\"string\",\n" | ||
| + "\t\"description\":\"string7string4string\",\n" | ||
| + "\t\"datesLocations\":[\n" | ||
| + "\t\t{\n" | ||
| + "\t\t\t\"id\":\0,\n" | ||
| + "\t\t\t{\n" | ||
| + "\t\t\t\t\"id\":\0,\n" | ||
| + "\t\t\t\t\"title\":\"string\",\n" | ||
| + "\t\t\t\t{\n" | ||
| + "\t\t\t\t\t\"id\":\0,\n" | ||
| + "\t\t\t\t\t\"title\":\"string\",\n" | ||
| + "\t\t\t\t\t{\n" | ||
| + "\t\t\t\t\t\t\"id\":\0,\n" | ||
| + "\t\t\t\t\t\t\"name\":\"string\",\n" | ||
| + "\t\t\t\t\t\t\"organizerRating\":\0,\n" | ||
| + "\t\t\t\t\t}\n" | ||
| + "\t\t\t\t\t\"creationDate\":\"2023-05-27\",\n" | ||
| + "\t\t\t\t\t\"description\":\"string\",\n" | ||
| + "\t\t\t\t\t\"dates\":[\n" | ||
| + "\t\t\t\t\t\t{\n" | ||
| + "\t\t\t\t\t\t\t\"id\":\0,\n" | ||
| + "\t\t\t\t\t\t\t\"startDate\":\"2023-05-27T15:00:00Z\",\n" | ||
| + "\t\t\t\t\t\t\t\"finishDate\":\"2023-05-27T17:00:00Z\",\n" | ||
| + "\t\t\t\t\t\t\t\"onlineLink\":\"string\",\n" | ||
| + "\t\t\t\t\t\t\t\"coordinates\":{\n" | ||
| + "\t\t\t\t\t\t\t\t\"latitude\":1,\n" | ||
| + "\t\t\t\t\t\t\t\t\"longitude\":1\n" | ||
| + "\t\t\t\t\t\t\t},\n" | ||
| + "\t\t\t\t\t\t}\n" | ||
| + "\t\t\t\t\t\"tags\":[\n" | ||
| + "\t\t\t\t\t\t{\n" | ||
| + "\t\t\t\t\t\t\t\"id\":\0,\n" | ||
| + "\t\t\t\t\t\t\t\"nameUa\":\"string\",\n" | ||
| + "\t\t\t\t\t\t\t\"nameEn\":\"string\",\n" | ||
| + "\t\t\t\t\t\t}\n" | ||
| + "\t\t\t\t\t\"titleImage\":\"string\",\n" | ||
| + "\t\t\t\t\t\"additionalImages\":[\"string\"]\n" | ||
| + "\t\t\t\t\t\"isOpen\":true\n" | ||
| + "\t\t\t\t\t\"isSubscribed\":true\n" | ||
| + "\t\t\t\t\t\"isFavourite\":true\n" | ||
| + "\t\t\t\t}\n" | ||
| + "\t\t\t\"startDate\":\"2023-05-27T15:00:00Z\",\n" | ||
| + "\t\t\t\"finishDate\":\"2023-05-27T17:00:00Z\",\n" | ||
| + "\t\t\t\"onlineLink\":\"string\",\n" | ||
| + "\t\t\t\"onlineLink\":\"http://localhost:8080/swagger-ui.html#/events-controller\",\n" | ||
| + "\t\t\t\"coordinates\":{\n" | ||
| + "\t\t\t\t\"latitude\":1,\n" | ||
| + "\t\t\t\t\"longitude\":1\n" | ||
| + "\t\t\t},\n" | ||
| + "\t\t\t}\n" | ||
| + "\t\t}\n" | ||
| + "\t],\n" | ||
| + "\t\"titleImage\":\"string\"\n" | ||
| + "\t\"additionalImages\":[\"string\"]\n" | ||
| + "\t\"imagesToDelete\":[\"string\"]\n" | ||
| + "\t\"tags\":[\"Social\"]\n" | ||
| + "\t\"isOpen\":true\n" | ||
| + "\t\"titleImage\":\"string\",\n" | ||
| + "\t\"additionalImages\":[\"string\"],\n" | ||
| + "\t\"tags\":[\"Social\"],\n" | ||
| + "\t\"open\":true\n" |
There was a problem hiding this comment.
Replace the null character in the UPDATE_EVENT sample.
Line 87 currently escapes 0 as \0, emitting a NUL byte instead of the numeral. That will either truncate or misrender the example payload in Swagger UI. Please change it to the literal 0 (i.e., "id":0) so the sample remains valid JSON.
- + "\t\"id\":\0,\n"
+ + "\t\"id\":0,\n"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + "\t\"id\":\0,\n" | |
| + "\t\"title\":\"string\",\n" | |
| + "\t\"description\":\"string\",\n" | |
| + "\t\"description\":\"string7string4string\",\n" | |
| + "\t\"datesLocations\":[\n" | |
| + "\t\t{\n" | |
| + "\t\t\t\"id\":\0,\n" | |
| + "\t\t\t{\n" | |
| + "\t\t\t\t\"id\":\0,\n" | |
| + "\t\t\t\t\"title\":\"string\",\n" | |
| + "\t\t\t\t{\n" | |
| + "\t\t\t\t\t\"id\":\0,\n" | |
| + "\t\t\t\t\t\"title\":\"string\",\n" | |
| + "\t\t\t\t\t{\n" | |
| + "\t\t\t\t\t\t\"id\":\0,\n" | |
| + "\t\t\t\t\t\t\"name\":\"string\",\n" | |
| + "\t\t\t\t\t\t\"organizerRating\":\0,\n" | |
| + "\t\t\t\t\t}\n" | |
| + "\t\t\t\t\t\"creationDate\":\"2023-05-27\",\n" | |
| + "\t\t\t\t\t\"description\":\"string\",\n" | |
| + "\t\t\t\t\t\"dates\":[\n" | |
| + "\t\t\t\t\t\t{\n" | |
| + "\t\t\t\t\t\t\t\"id\":\0,\n" | |
| + "\t\t\t\t\t\t\t\"startDate\":\"2023-05-27T15:00:00Z\",\n" | |
| + "\t\t\t\t\t\t\t\"finishDate\":\"2023-05-27T17:00:00Z\",\n" | |
| + "\t\t\t\t\t\t\t\"onlineLink\":\"string\",\n" | |
| + "\t\t\t\t\t\t\t\"coordinates\":{\n" | |
| + "\t\t\t\t\t\t\t\t\"latitude\":1,\n" | |
| + "\t\t\t\t\t\t\t\t\"longitude\":1\n" | |
| + "\t\t\t\t\t\t\t},\n" | |
| + "\t\t\t\t\t\t}\n" | |
| + "\t\t\t\t\t\"tags\":[\n" | |
| + "\t\t\t\t\t\t{\n" | |
| + "\t\t\t\t\t\t\t\"id\":\0,\n" | |
| + "\t\t\t\t\t\t\t\"nameUa\":\"string\",\n" | |
| + "\t\t\t\t\t\t\t\"nameEn\":\"string\",\n" | |
| + "\t\t\t\t\t\t}\n" | |
| + "\t\t\t\t\t\"titleImage\":\"string\",\n" | |
| + "\t\t\t\t\t\"additionalImages\":[\"string\"]\n" | |
| + "\t\t\t\t\t\"isOpen\":true\n" | |
| + "\t\t\t\t\t\"isSubscribed\":true\n" | |
| + "\t\t\t\t\t\"isFavourite\":true\n" | |
| + "\t\t\t\t}\n" | |
| + "\t\t\t\"startDate\":\"2023-05-27T15:00:00Z\",\n" | |
| + "\t\t\t\"finishDate\":\"2023-05-27T17:00:00Z\",\n" | |
| + "\t\t\t\"onlineLink\":\"string\",\n" | |
| + "\t\t\t\"onlineLink\":\"http://localhost:8080/swagger-ui.html#/events-controller\",\n" | |
| + "\t\t\t\"coordinates\":{\n" | |
| + "\t\t\t\t\"latitude\":1,\n" | |
| + "\t\t\t\t\"longitude\":1\n" | |
| + "\t\t\t},\n" | |
| + "\t\t\t}\n" | |
| + "\t\t}\n" | |
| + "\t],\n" | |
| + "\t\"titleImage\":\"string\"\n" | |
| + "\t\"additionalImages\":[\"string\"]\n" | |
| + "\t\"imagesToDelete\":[\"string\"]\n" | |
| + "\t\"tags\":[\"Social\"]\n" | |
| + "\t\"isOpen\":true\n" | |
| + "\t\"titleImage\":\"string\",\n" | |
| + "\t\"additionalImages\":[\"string\"],\n" | |
| + "\t\"tags\":[\"Social\"],\n" | |
| + "\t\"open\":true\n" | |
| "\t\"id\":0,\n" | |
| "\t\"title\":\"string\",\n" | |
| "\t\"description\":\"string7string4string\",\n" | |
| "\t\"datesLocations\":[\n" | |
| "\t\t{\n" | |
| "\t\t\t\"startDate\":\"2023-05-27T15:00:00Z\",\n" | |
| "\t\t\t\"finishDate\":\"2023-05-27T17:00:00Z\",\n" | |
| "\t\t\t\"onlineLink\":\"http://localhost:8080/swagger-ui.html#/events-controller\",\n" | |
| "\t\t\t\"coordinates\":{\n" | |
| "\t\t\t\t\"latitude\":1,\n" | |
| "\t\t\t\t\"longitude\":1\n" | |
| "\t\t\t}\n" | |
| "\t\t}\n" | |
| "\t],\n" | |
| "\t\"titleImage\":\"string\",\n" | |
| "\t\"additionalImages\":[\"string\"],\n" | |
| "\t\"tags\":[\"Social\"],\n" | |
| "\t\"open\":true\n" |
🤖 Prompt for AI Agents
In core/src/main/java/greencity/constant/SwaggerExampleModel.java around lines
87 to 104, the UPDATE_EVENT JSON sample contains an escaped NUL character
sequence `\0` for the id field which produces a NUL byte and invalid/misrendered
JSON; replace the escaped sequence with the literal numeral 0 so the entry reads
with a numeric id (i.e., use "id":0) ensuring the example is valid JSON and
displays correctly in Swagger UI.
| public ResponseEntity<Integer> getAchievementCount( | ||
| @Parameter(hidden = true) @CurrentUserId Long userId, | ||
| @Parameter(hidden = true) Principal principal, | ||
| @Parameter(description = "Available values : ACHIEVED, UNACHIEVED." | ||
| + " Leave this field empty if you need items with any status") @RequestParam( | ||
| required = false) String achievementStatus) { | ||
| return ResponseEntity.status(HttpStatus.OK) | ||
| .body(achievementService.findAllByType(principal.getName(), achievementStatus)); | ||
| required = false) AchievementStatus achievementStatus, | ||
| @RequestParam(required = false) Long achievementCategoryId) { | ||
| return ResponseEntity.ok() | ||
| .body(achievementService.findAchievementCountByTypeAndCategory(userId, principal.getName(), | ||
| achievementStatus, achievementCategoryId)); |
There was a problem hiding this comment.
Stop depending on Principal for achievement count
With the new @CurrentUserClaims resolver in this PR, relying on Principal risks a NullPointerException (when no principal is attached) or passing the wrong identifier if the authentication name is no longer the email. All other endpoints in this controller now use UserClaims; this one should too so we always hand the service the authenticated user’s id/email from the same source.
- public ResponseEntity<Integer> getAchievementCount(
- @Parameter(hidden = true) @CurrentUserId Long userId,
- @Parameter(hidden = true) Principal principal,
+ public ResponseEntity<Integer> getAchievementCount(
+ @Parameter(hidden = true) @CurrentUserClaims UserClaims userClaims,
@Parameter(description = "Available values : ACHIEVED, UNACHIEVED."
+ " Leave this field empty if you need items with any status") @RequestParam(
required = false) AchievementStatus achievementStatus,
@RequestParam(required = false) Long achievementCategoryId) {
return ResponseEntity.ok()
- .body(achievementService.findAchievementCountByTypeAndCategory(userId, principal.getName(),
+ .body(achievementService.findAchievementCountByTypeAndCategory(userClaims.userId(),
+ userClaims.userEmail(),
achievementStatus, achievementCategoryId));
}🤖 Prompt for AI Agents
In core/src/main/java/greencity/controller/AchievementController.java around
lines 86 to 95, stop accepting Principal and instead inject the new current-user
claims resolver: replace the Principal parameter with @CurrentUserClaims
UserClaims userClaims (keep the existing @CurrentUserId Long userId), and pass
userClaims.getEmail() to
achievementService.findAchievementCountByTypeAndCategory in place of
principal.getName(); this ensures the service receives the authenticated user
identifier from the same UserClaims source used by other endpoints and avoids
possible NPEs or mismatched identifiers.
| @GetMapping("/{userId}/{habitId}") | ||
| public ResponseEntity<List<CustomToDoListItemResponseDto>> getAllAvailableCustomToDoListItems( | ||
| @PathVariable Long userId, @PathVariable Long habitId) { | ||
| return ResponseEntity.status(HttpStatus.OK) | ||
| .body(customToDoListItemService.findAllAvailableCustomToDoListItems(userId, habitId)); | ||
| } |
There was a problem hiding this comment.
Add authorization validation to prevent unauthorized data access.
This endpoint accepts userId as a path variable without @ValidCurrentUserId validation, unlike all other endpoints in this controller (lines 108, 137, 162, 188, 210). This could allow authenticated users to access other users' custom to-do list items by manipulating the URL.
Apply this diff to add the missing validation:
@GetMapping("/{userId}/{habitId}")
public ResponseEntity<List<CustomToDoListItemResponseDto>> getAllAvailableCustomToDoListItems(
- @PathVariable Long userId, @PathVariable Long habitId) {
+ @Parameter(description = "Id of current user. Cannot be empty.") @PathVariable @ValidCurrentUserId Long userId,
+ @PathVariable Long habitId) {
return ResponseEntity.status(HttpStatus.OK)
.body(customToDoListItemService.findAllAvailableCustomToDoListItems(userId, habitId));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @GetMapping("/{userId}/{habitId}") | |
| public ResponseEntity<List<CustomToDoListItemResponseDto>> getAllAvailableCustomToDoListItems( | |
| @PathVariable Long userId, @PathVariable Long habitId) { | |
| return ResponseEntity.status(HttpStatus.OK) | |
| .body(customToDoListItemService.findAllAvailableCustomToDoListItems(userId, habitId)); | |
| } | |
| @GetMapping("/{userId}/{habitId}") | |
| public ResponseEntity<List<CustomToDoListItemResponseDto>> getAllAvailableCustomToDoListItems( | |
| @Parameter(description = "Id of current user. Cannot be empty.") @PathVariable @ValidCurrentUserId Long userId, | |
| @PathVariable Long habitId) { | |
| return ResponseEntity.status(HttpStatus.OK) | |
| .body(customToDoListItemService.findAllAvailableCustomToDoListItems(userId, habitId)); | |
| } |
🤖 Prompt for AI Agents
In core/src/main/java/greencity/controller/CustomToDoListItemController.java
around lines 55 to 60, the method getAllAvailableCustomToDoListItems accepts
userId as a PathVariable without the @ValidCurrentUserId validation; add the
same authorization annotation used on other endpoints by annotating the userId
parameter with @ValidCurrentUserId (i.e., change the parameter declaration to
include @ValidCurrentUserId alongside @PathVariable) so requests are validated
against the authenticated user before calling the service.
| @GetMapping("/{userId}/get-all-inprogress") | ||
| public ResponseEntity<List<ToDoListItemDto>> findInProgressByUserId( | ||
| @PathVariable @ValidCurrentUserId Long userId, @RequestParam(name = "lang") String code) { | ||
| return ResponseEntity.status(HttpStatus.OK) | ||
| .body(toDoListItemService.findInProgressByUserIdAndLanguageCode(userId, code)); | ||
| } |
There was a problem hiding this comment.
Potential authorization bypass: endpoint accepts any userId.
Line 236: Unlike other endpoints in this controller that use @CurrentUserId to restrict access to the authenticated user's data, this endpoint accepts any userId via path variable. This allows users to query any other user's in-progress to-do items, potentially exposing private data.
If this endpoint should only return the current user's items, use @CurrentUserId and remove userId from the path. If access to other users' data is intentional (e.g., for admin users), add explicit authorization checks in the service layer.
If this should be restricted to the current user, apply this diff:
- @GetMapping("/{userId}/get-all-inprogress")
+ @GetMapping("/get-all-inprogress")
public ResponseEntity<List<ToDoListItemDto>> findInProgressByUserId(
- @PathVariable @ValidCurrentUserId Long userId, @RequestParam(name = "lang") String code) {
+ @Parameter(hidden = true) @CurrentUserId Long userId,
+ @RequestParam(name = "lang") String code) {
return ResponseEntity.status(HttpStatus.OK)
.body(toDoListItemService.findInProgressByUserIdAndLanguageCode(userId, code));
}🤖 Prompt for AI Agents
In core/src/main/java/greencity/controller/ToDoListItemController.java around
lines 234 to 239, the endpoint currently accepts any userId via @PathVariable
which allows querying other users' data; replace the path-variable userId with
the authenticated user's id by removing userId from the URL mapping and method
parameters and using @CurrentUserId Long currentUserId (i.e., change the mapping
to not include "{userId}/" and change the controller method to accept
@CurrentUserId Long currentUserId), then call
toDoListItemService.findInProgressByUserIdAndLanguageCode(currentUserId, code);
if instead cross-user access is intentional, keep the path but add explicit
authorization checks in the service layer to restrict access to admins or
authorized users and update any affected tests/routes accordingly.
| @PatchMapping("/{id}/location") | ||
| public ResponseEntity<Void> setLocationForUser( | ||
| @PathVariable(name = "id") Long userId, | ||
| @RequestBody UserProfileDtoRequest userProfileDtoRequest) { | ||
| userService.setLocationForUser(userId, userProfileDtoRequest); | ||
| return ResponseEntity.status(HttpStatus.OK).build(); | ||
| } |
There was a problem hiding this comment.
Restore DTO validation with @Valid.
Right now the controller receives UserProfileDtoRequest without @Valid, so none of the jakarta bean constraints on that DTO (or on UserProfileDtoRequest’s nested objects) will run. The same regression affects the other request bodies in this controller (setLocationForUser by email, both increaseUserRating variants, updateUserCredoByUserId, createUser, etc.). This lets malformed payloads slip into the service layer, where we either persist inconsistent data or fail later with less helpful errors. Please add @Valid to every @RequestBody in this controller.
- @RequestBody UserProfileDtoRequest userProfileDtoRequest) {
+ @Valid @RequestBody UserProfileDtoRequest userProfileDtoRequest) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @PatchMapping("/{id}/location") | |
| public ResponseEntity<Void> setLocationForUser( | |
| @PathVariable(name = "id") Long userId, | |
| @RequestBody UserProfileDtoRequest userProfileDtoRequest) { | |
| userService.setLocationForUser(userId, userProfileDtoRequest); | |
| return ResponseEntity.status(HttpStatus.OK).build(); | |
| } | |
| @PatchMapping("/{id}/location") | |
| public ResponseEntity<Void> setLocationForUser( | |
| @PathVariable(name = "id") Long userId, | |
| @Valid @RequestBody UserProfileDtoRequest userProfileDtoRequest) { | |
| userService.setLocationForUser(userId, userProfileDtoRequest); | |
| return ResponseEntity.status(HttpStatus.OK).build(); | |
| } |
🤖 Prompt for AI Agents
In core/src/main/java/greencity/controller/UserController.java around lines 126
to 132, the controller currently accepts UserProfileDtoRequest (and other
request bodies in this class) without bean validation, so jakarta bean
constraints are not executed; add @Valid to every method parameter annotated
with @RequestBody (e.g., setLocationForUser, setLocationForUser by email, both
increaseUserRating overloads, updateUserCredoByUserId, createUser, etc.), and
ensure you import jakarta.validation.Valid at the top of the file so nested DTO
constraints are validated before reaching the service layer.
| @ExceptionHandler(OpenAIRequestException.class) | ||
| public final ResponseEntity<Object> handleOpenAIResponseException(OpenAIRequestException ex, WebRequest request) { | ||
| log.error(ex.getMessage(), ex); | ||
| ExceptionResponse exceptionResponse = new ExceptionResponse(getErrorAttributes(request)); | ||
| log.trace(ex.getMessage(), ex); | ||
| return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(exceptionResponse); | ||
| exceptionResponse.setMessage(ex.getMessage()); | ||
|
|
||
| return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(exceptionResponse); | ||
| } | ||
|
|
There was a problem hiding this comment.
Restore handling for OpenAI response failures.
The handler named handleOpenAIResponseException is only wired to OpenAIRequestException, so any OpenAIResponseException (thrown when parsing fails in OpenAIServiceImpl) will now bypass our custom mapping and fall through to the default 500 response. Please register both exception types so callers continue to receive the structured error payload instead of an unexpected stack trace.
- @ExceptionHandler(OpenAIRequestException.class)
+ @ExceptionHandler({OpenAIRequestException.class, OpenAIResponseException.class})
public final ResponseEntity<Object> handleOpenAIResponseException(OpenAIRequestException ex, WebRequest request) {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In core/src/main/java/greencity/exception/handler/CustomExceptionHandler.java
around lines 775-783, the handler is only registered for OpenAIRequestException
so OpenAIResponseException bypasses it; update the @ExceptionHandler to register
both exceptions and make the method accept a common type (e.g., Exception) so it
handles either. Change the annotation to
@ExceptionHandler({OpenAIRequestException.class, OpenAIResponseException.class})
and adjust the method signature parameter from OpenAIRequestException to
Exception (or a suitable common superclass) so the existing logging,
ExceptionResponse creation, and ResponseEntity return remain unchanged.
|
|
||
| setCell(row, 0, dto.getId().toString(), sheet); | ||
| setCell(row, 1, dto.getRatingCalculationEnum().toString(), sheet); | ||
| setCell(row, 1, dto.getRatingPoints().getName(), sheet); |
There was a problem hiding this comment.
🧩 Analysis chain
Add null safety to prevent potential NPE.
The chained call dto.getRatingPoints().getName() will throw a NullPointerException if getRatingPoints() returns null. Consider adding null-safe handling, especially since the current exception handler only catches IOException.
Additionally, verify that the change from enum stringification to getName() produces the expected values for Excel consumers and doesn't break existing integrations.
Apply this diff to add null safety:
- setCell(row, 1, dto.getRatingPoints().getName(), sheet);
+ setCell(row, 1, dto.getRatingPoints() != null ? dto.getRatingPoints().getName() : "N/A", sheet);To verify the impact of this change and null safety, run:
🏁 Script executed:
#!/bin/bash
# Description: Check RatingStatisticsDto structure and verify getRatingPoints() nullability
# Search for RatingStatisticsDto class definition and getRatingPoints method
ast-grep --pattern $'class RatingStatisticsDto {
$$$
}'
# Search for any null checks on getRatingPoints in the codebase
rg -n -C3 'getRatingPoints\(\)\s*[!=]=\s*null'
# Find usages of RatingExcelExporter to understand impact on consumers
rg -n -C3 'RatingExcelExporter'Length of output: 4406
🏁 Script executed:
#!/bin/bash
# Find RatingStatisticsDto, RatingPoints classes and examine RatingExcelExporter method
# Find all Java files related to rating statistics DTOs
echo "=== Finding RatingStatisticsDto.java ==="
fd -type f -name "*RatingStatistics*.java" | head -20
echo -e "\n=== Finding RatingPoints.java or enum ==="
fd -type f -name "*RatingPoints*.java" | head -20
# View RatingExcelExporter method around line 54
echo -e "\n=== RatingExcelExporter export() method context ==="
cat core/src/main/java/greencity/exporter/RatingExcelExporter.java | head -100Length of output: 3458
🏁 Script executed:
#!/bin/bash
# Find and examine RatingStatisticsDto and RatingPoints classes
echo "=== Finding RatingStatisticsDto.java ==="
find . -name "*RatingStatistics*Dto.java" 2>/dev/null
echo -e "\n=== Finding RatingPoints.java or similar ==="
find . -name "*RatingPoints*.java" 2>/dev/null
echo -e "\n=== Search for getRatingPoints method definition ==="
rg -n "getRatingPoints\(\)" --type java -A2 | head -30Length of output: 2468
🏁 Script executed:
#!/bin/bash
# Alternative: search in greencity dto package structure
find . -path "*/dto/ratingstatistics/*" -name "*.java" 2>/dev/null | head -20Length of output: 517
🏁 Script executed:
#!/bin/bash
# Examine the DTO and entity class definitions
echo "=== RatingStatisticsDto.java ==="
cat service-api/src/main/java/greencity/dto/ratingstatistics/RatingStatisticsDto.java
echo -e "\n=== RatingPoints.java entity ==="
cat dao/src/main/java/greencity/entity/RatingPoints.java
echo -e "\n=== RatingPointsDto.java ==="
cat service-api/src/main/java/greencity/dto/ratingstatistics/RatingPointsDto.javaLength of output: 2368
Add null safety checks to prevent NullPointerException on null getRatingPoints().
The field ratingPoints in RatingStatisticsDto lacks nullability annotations, allowing getRatingPoints() to return null without type-level indication. Calling getName() on a null result will throw NullPointerException at line 54, which won't be caught by the existing IOException handler.
This pattern also affects line 56–57 (chained calls on getUser()). Consider one of these approaches:
- Use Optional for explicit null handling
- Add
@NonNullannotations if null is never expected - Implement a null-safe operator pattern consistent with your codebase
Example:
- setCell(row, 1, dto.getRatingPoints().getName(), sheet);
+ setCell(row, 1, Optional.ofNullable(dto.getRatingPoints()).map(RatingPointsDto::getName).orElse("N/A"), sheet);🤖 Prompt for AI Agents
In core/src/main/java/greencity/exporter/RatingExcelExporter.java around lines
54 and 56–57, the code calls dto.getRatingPoints().getName() and chained
dto.getUser() calls without null checks which can cause NullPointerException;
update the method to guard each chained call (ratingPoints and user and their
nested fields) before invoking getName()/getFirstName()/getLastName() — e.g.,
use Optional.ofNullable(...) or explicit null checks and supply a safe fallback
(empty string or "N/A") when a component is null, then pass that safe value to
setCell; alternatively add @NonNull to DTO fields if nulls are impossible, but
prefer adding defensive null checks here to prevent runtime exceptions.
| log.info("Token has expired: {}", token); | ||
| } catch (Exception e) { | ||
| log.info("Access denied with token: " + e.getMessage()); | ||
| log.info("Access denied with token: {}", e.getMessage()); |
There was a problem hiding this comment.
Avoid logging raw access tokens
Logging the entire JWT at Line 75 leaks a bearer credential into the logs, which is a security risk (tokens may be harvested from log storage). Let’s switch to contextual data that doesn’t expose the secret; we can still keep helpful diagnostics by logging the request path and exception message. While we’re here, the follow-up log message still says “with token” but only prints the exception text, so updating that keeps the wording accurate.
Here’s an example adjustment:
- log.info("Token has expired: {}", token);
+ log.info("Token has expired for URI {}", request.getRequestURI());
} catch (Exception e) {
- log.info("Access denied with token: {}", e.getMessage());
+ log.info("Access denied for URI {}: {}", request.getRequestURI(), e.getMessage());This keeps logs useful without disclosing credentials.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log.info("Token has expired: {}", token); | |
| } catch (Exception e) { | |
| log.info("Access denied with token: " + e.getMessage()); | |
| log.info("Access denied with token: {}", e.getMessage()); | |
| log.info("Token has expired for URI {}", request.getRequestURI()); | |
| } catch (Exception e) { | |
| log.info("Access denied for URI {}: {}", request.getRequestURI(), e.getMessage()); |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes & Improvements
Infrastructure