-
Notifications
You must be signed in to change notification settings - Fork 0
Dev #14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds GET /kubernetes/me returning the caller's Kubernetes key by authenticated Member UUID; replaces active member lists of Strings with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant K8sController
participant K8sService
participant K8sRepository
participant Database
Note over Client,K8sController: GET /kubernetes/me (authenticated)
Client->>K8sController: HTTP GET (Member principal)
K8sController->>K8sService: getKubernetesKeyByUUID(member.getUUID())
K8sService->>K8sRepository: findKubernetesKeyByUUID(UUID)
K8sRepository->>Database: SELECT kubernetes_key WHERE member_uuid=UUID
Database-->>K8sRepository: Optional<String> (key or empty)
alt key found
K8sRepository-->>K8sService: key
K8sService-->>K8sController: key
K8sController-->>Client: 200 + GetMyKubernetesKeyResponseDto{key}
else not found
K8sRepository-->>K8sService: empty
K8sService-->>K8sController: throw IllegalArgumentException / NotFound
K8sController-->>Client: 404 / error response
end
Estimated code review effortπ― 3 (Moderate) | β±οΈ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touchesβ Failed checks (1 warning, 1 inconclusive)
β Passed checks (1 passed)
β¨ Finishing touches
π§ͺ Generate unit tests
π Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro π Files selected for processing (2)
π Additional comments (3)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (6)
src/main/java/org/poolc/api/kubernetes/controller/KubernetesController.java(2 hunks)src/main/java/org/poolc/api/kubernetes/dto/ActiveMemberDto.java(1 hunks)src/main/java/org/poolc/api/kubernetes/dto/GetKubernetesResponseDto.java(1 hunks)src/main/java/org/poolc/api/kubernetes/dto/GetMyKubernetesKeyResponseDto.java(1 hunks)src/main/java/org/poolc/api/kubernetes/repository/KubernetesRepository.java(2 hunks)src/main/java/org/poolc/api/kubernetes/service/KubernetesService.java(3 hunks)
π Additional comments (2)
src/main/java/org/poolc/api/kubernetes/dto/ActiveMemberDto.java (1)
3-6: Projection interface fits the JPA projection use case well.The minimal interface with accessor methods keeps the projection lightweight and should plug straight into the repository layer.
src/main/java/org/poolc/api/kubernetes/dto/GetKubernetesResponseDto.java (1)
10-14: Confirm downstream consumers handle the new payload shape.
activeMembersused to be aList<String>; it is now a list of objects. Please double-check that any clients (web/app/automation) parsing this endpoint have been updated so we donβt break them at deploy time.
src/main/java/org/poolc/api/kubernetes/controller/KubernetesController.java
Show resolved
Hide resolved
| import com.fasterxml.jackson.annotation.JsonCreator; | ||
| import lombok.Getter; | ||
|
|
||
| @Getter | ||
| public class GetMyKubernetesKeyResponseDto { | ||
|
|
||
| private final String key; | ||
|
|
||
| @JsonCreator | ||
| public GetMyKubernetesKeyResponseDto(String key){ | ||
| this.key = key; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard Jackson constructor binding with @JsonProperty.
With @JsonCreator on a single-arg constructor, Jackson switches to delegating mode unless the argument is explicitly named. Without @JsonProperty("key"), ObjectMapper.readValue("{\"key\":\"value\"}", GetMyKubernetesKeyResponseDto.class) will throw InvalidDefinitionException (unless we happen to register ParameterNamesModule everywhere). Please annotate the parameter (and add the import) to keep deserialization safe.
-import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
@@
- @JsonCreator
- public GetMyKubernetesKeyResponseDto(String key){
+ @JsonCreator
+ public GetMyKubernetesKeyResponseDto(@JsonProperty("key") String key){
this.key = key;
}π 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.
| import com.fasterxml.jackson.annotation.JsonCreator; | |
| import lombok.Getter; | |
| @Getter | |
| public class GetMyKubernetesKeyResponseDto { | |
| private final String key; | |
| @JsonCreator | |
| public GetMyKubernetesKeyResponseDto(String key){ | |
| this.key = key; | |
| } | |
| import com.fasterxml.jackson.annotation.JsonCreator; | |
| import com.fasterxml.jackson.annotation.JsonProperty; | |
| import lombok.Getter; | |
| @Getter | |
| public class GetMyKubernetesKeyResponseDto { | |
| private final String key; | |
| @JsonCreator | |
| public GetMyKubernetesKeyResponseDto(@JsonProperty("key") String key) { | |
| this.key = key; | |
| } | |
| } |
π€ Prompt for AI Agents
In src/main/java/org/poolc/api/kubernetes/dto/GetMyKubernetesKeyResponseDto.java
around lines 3 to 14, the @JsonCreator single-arg constructor is not guarded
with @JsonProperty, which causes Jackson to use delegating mode and fail
deserialization unless ParameterNamesModule is registered; add the import for
com.fasterxml.jackson.annotation.JsonProperty and annotate the constructor
parameter with @JsonProperty("key") so Jackson can bind the "key" JSON property
to the constructor parameter during deserialization.
| @Query(value = "SELECT DISTINCT T2.MEMBER_UUID\n" + | ||
| " , (SELECT login_id" + | ||
| " FROM MEMBER T1" + | ||
| " WHERE T1.UUID = T2.MEMBER_UUID)\n" + | ||
| "FROM ROLES T2\n" + | ||
| "WHERE T2.MEMBER_UUID IN (\n" + | ||
| " SELECT MEMBER_UUID\n" + | ||
| " FROM ROLES\n" + | ||
| " WHERE ROLES = 'MEMBER'\n" + | ||
| ")\n" + | ||
| "AND MEMBER_UUID NOT IN (\n" + | ||
| "AND T2.MEMBER_UUID NOT IN (\n" + | ||
| " SELECT MEMBER_UUID\n" + | ||
| " FROM ROLES\n" + | ||
| " WHERE ROLES = 'INACTIVE'\n" + | ||
| ")", nativeQuery = true) | ||
| List<String> findAllActiveMembers(); | ||
| List<ActiveMemberDto> findAllActiveMembers(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alias the selected columns for ActiveMemberDto projection
Line 17 currently returns the login ID via a scalar subquery without an alias. In Postgres (and most RDBMSs) that yields a ?column? heading, so Spring canβt map it onto the ActiveMemberDto getters and youβll hit an IllegalArgumentException at runtime. Please alias both selected columns to match the DTO contract (and consider replacing the subquery with a join for clarity). For example:
- @Query(value = "SELECT DISTINCT T2.MEMBER_UUID\n" +
- " , (SELECT login_id" +
- " FROM MEMBER T1" +
- " WHERE T1.UUID = T2.MEMBER_UUID)\n" +
+ @Query(value = "SELECT DISTINCT T2.MEMBER_UUID AS memberUuid\n" +
+ " , (SELECT login_id" +
+ " FROM MEMBER T1" +
+ " WHERE T1.UUID = T2.MEMBER_UUID) AS loginId\n" +
"FROM ROLES T2\n" +
"WHERE T2.MEMBER_UUID IN (\n" +
" SELECT MEMBER_UUID\n" +
" FROM ROLES\n" +
" WHERE ROLES = 'MEMBER'\n" +
")\n" +
"AND T2.MEMBER_UUID NOT IN (\n" +
" SELECT MEMBER_UUID\n" +
" FROM ROLES\n" +
" WHERE ROLES = 'INACTIVE'\n" +
")", nativeQuery = true)π€ Prompt for AI Agents
In src/main/java/org/poolc/api/kubernetes/repository/KubernetesRepository.java
around lines 16 to 31, the native query selects MEMBER_UUID and a scalar
subquery for login_id without aliases which prevents Spring from mapping the
result to ActiveMemberDto; modify the SELECT to alias both columns to the DTO
property names (e.g., memberUuid or member_uuid and loginId or login_id matching
ActiveMemberDto getters/constructor) so the projection maps correctly, and
optionally replace the scalar subquery with an equivalent JOIN to MEMBER for
clarity and performance.
| @Query(value = "SELECT kubernetesKey FROM kubernetes_mappings WHERE UUID = :UUID") | ||
| Optional<String> findKubernetesKeyByUUID(String UUID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restore nativeQuery = true (and bind the UUID properly)
Line 38 is still raw SQL against kubernetes_mappings, but the @Query annotation no longer sets nativeQuery = true, so Spring will parse it as JPQL and blow up at runtime. Please reinstate the native flag and bind the parameter explicitly:
- @Query(value = "SELECT kubernetesKey FROM kubernetes_mappings WHERE UUID = :UUID")
- Optional<String> findKubernetesKeyByUUID(String UUID);
+ @Query(value = "SELECT kubernetesKey FROM kubernetes_mappings WHERE UUID = :uuid", nativeQuery = true)
+ Optional<String> findKubernetesKeyByUUID(@Param("uuid") String uuid);π 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.
| @Query(value = "SELECT kubernetesKey FROM kubernetes_mappings WHERE UUID = :UUID") | |
| Optional<String> findKubernetesKeyByUUID(String UUID); | |
| @Query(value = "SELECT kubernetesKey FROM kubernetes_mappings WHERE UUID = :uuid", nativeQuery = true) | |
| Optional<String> findKubernetesKeyByUUID(@Param("uuid") String uuid); |
π€ Prompt for AI Agents
In src/main/java/org/poolc/api/kubernetes/repository/KubernetesRepository.java
around lines 38-39, the @Query is raw SQL but missing nativeQuery = true and the
UUID parameter isn't explicitly bound; restore nativeQuery = true on the @Query
and bind the parameter by annotating the method parameter with @Param("UUID")
(or change the placeholder and param name consistently) so the native SQL runs
correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (3)
src/main/java/org/poolc/api/comment/controller/CommentController.java(1 hunks)src/main/java/org/poolc/api/kubernetes/controller/KubernetesController.java(2 hunks)src/main/java/org/poolc/api/kubernetes/service/KubernetesService.java(3 hunks)
π Additional comments (1)
src/main/java/org/poolc/api/comment/controller/CommentController.java (1)
57-58: LGTM on the whitespace tweak.No functional impact; formatting stays consistent.
π Issue Number
Close #
π μμ λ΄μ
π μμ μ ν
π 체ν¬λ¦¬μ€νΈ
π PR νΉμ΄ μ¬ν
Summary by CodeRabbit
New Features
Refactor