Skip to content
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

feat: 퀘스트 리스트 조회 #41

Merged
merged 6 commits into from
Nov 18, 2024
Merged

feat: 퀘스트 리스트 조회 #41

merged 6 commits into from
Nov 18, 2024

Conversation

koosco
Copy link
Collaborator

@koosco koosco commented Nov 18, 2024

🚀 개요
close #40
🔍 변경사항
변경사항 1: quest 조회 기능 추가

변경사항 2: quest content 속성 제거

변경사항 3
⏳ 작업 내용
작업 내용 1
작업 내용 2
작업 내용 3
📝 논의사항

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced new database structure with multiple tables for managing quests, goals, and members.
    • Added functionality to query quests associated with specific goals for members.
    • Implemented a new controller to handle HTTP requests for quest information.
  • Bug Fixes

    • Adjusted the maximum length for quest content to improve data integrity.
  • Documentation

    • Updated comments for clarity in the JobCommandController.

These changes enhance the app's ability to manage and retrieve quest-related information effectively.

Goal이 아닌 MemberGoal이 완료 상태를 갖도록 수정했습니다.
조회 효율성을 위해 MemberGoal에 memberId, goalId를 추가했습니다.
Copy link

coderabbitai bot commented Nov 18, 2024

Walkthrough

The changes introduced in this pull request include modifications to the SQL initialization script, which alters the structure of the quest table and creates several new tables relevant to the application. Additionally, new Java classes are added to support querying quests, managing goals, and configuring JSON serialization. Notable additions include the QuestQueryService, QuestQueryController, and various entity classes such as Goal, Quest, and MemberGoal. These changes enhance the application's ability to manage quests and their associated data.

Changes

File Path Change Summary
sql/init.sql Altered quest table: content column changed from VARCHAR(255) to VARCHAR(50). Created tables: member, job, interest_job, resume, goal, member_goal, quest, schedule. Dropped existing tables if they exist.
src/main/java/com/groom/orbit/config/web/JacksonConfig.java Added JacksonConfig class to customize Jackson ObjectMapper with SNAKE_CASE naming strategy.
src/main/java/com/groom/orbit/goal/app/QuestQueryService.java Added QuestQueryService class with method findQuestsByGoalId(Long memberId, Long goalId).
src/main/java/com/groom/orbit/goal/app/dto/QuestInfoResponseDto.java Added QuestInfoResponseDto record class with fields: id, title, isComplete.
src/main/java/com/groom/orbit/goal/controller/QuestQueryController.java Added QuestQueryController class with method findQuest(Long memberId, @RequestParam("goal_id") Long goalId).
src/main/java/com/groom/orbit/goal/dao/QuestRepository.java Added QuestRepository interface with method findByMemberIdAndGoalId(Long memberId, Long goalId).
src/main/java/com/groom/orbit/goal/dao/entity/Goal.java Added Goal entity class with fields: goalId, title, category, count and a static method create.
src/main/java/com/groom/orbit/goal/dao/entity/MemberGoal.java Added MemberGoal entity class with fields and methods for managing member goals.
src/main/java/com/groom/orbit/goal/dao/entity/MemberGoalId.java Added MemberGoalId class for composite key representation.
src/main/java/com/groom/orbit/goal/dao/entity/Quest.java Added Quest entity class with various fields for quest management.
src/main/java/com/groom/orbit/job/controller/JobCommandController.java Updated comment formatting in JobCommandController related to addInterestJob method.

Poem

🐰 In the land of quests and goals so bright,
New tables spring forth, a wondrous sight!
With Jackson's magic, names take flight,
While quests and goals dance in the light.
Hooray for the changes, let’s hop with glee,
For every new feature brings joy, you see! 🌟


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 12

🧹 Outside diff range and nitpick comments (12)
src/main/java/com/groom/orbit/goal/dao/entity/MemberGoalId.java (2)

8-11: Add class and field documentation

Add JavaDoc to explain the purpose of this class and its fields.

Add documentation:

+/**
+ * Composite primary key class for MemberGoal entity.
+ * Used to represent the combined key of member and goal IDs in the member_goal table.
+ */
 @EqualsAndHashCode
 public class MemberGoalId implements Serializable {
     private static final long serialVersionUID = 1L;
 
+    /** ID of the member */
     private Long member;
+    /** ID of the goal */
     private Long goal;

9-10: Consider more descriptive field names

The current field names are quite generic. Consider renaming them to be more specific about what they represent.

-    private Long member;
-    private Long goal;
+    private Long memberId;
+    private Long goalId;
src/main/java/com/groom/orbit/goal/app/QuestQueryService.java (1)

13-15: Add class-level documentation.

Consider adding Javadoc documentation to describe the service's purpose and responsibilities.

+/**
+ * Service responsible for querying quest-related information.
+ * Provides methods to retrieve quests associated with specific goals and members.
+ */
 @Service
 @RequiredArgsConstructor
 public class QuestQueryService {
src/main/java/com/groom/orbit/goal/controller/QuestQueryController.java (1)

16-19: Add API versioning and documentation.

Consider the following improvements:

  1. Add API versioning to the base path (e.g., /api/v1/quest)
  2. Include OpenAPI/Swagger annotations for API documentation
 @RestController
 @RequiredArgsConstructor
-@RequestMapping("/api/quest")
+@RequestMapping("/api/v1/quest")
+@Tag(name = "Quest", description = "Quest retrieval APIs")
src/main/java/com/groom/orbit/goal/dao/entity/Goal.java (2)

24-28: Add validation annotations for title and category fields.

Consider adding Jakarta Validation annotations to enforce business rules:

  1. For title: Add pattern validation if specific format is required
  2. For category: Add value validation if specific categories are allowed
+import jakarta.validation.constraints.Pattern;
+import jakarta.validation.constraints.NotBlank;

 @Column(nullable = false, length = 50)
+@NotBlank(message = "Title cannot be blank")
+@Pattern(regexp = "^[\\p{L}\\s\\d]+$", message = "Title can only contain letters, numbers and spaces")
 private String title;

 @Column(nullable = false, length = 10)
+@NotBlank(message = "Category cannot be blank")
 private String category;

14-41: Add audit fields and documentation for better maintainability.

Consider the following architectural improvements:

  1. Add audit fields for entity tracking
  2. Add class-level documentation
+import org.springframework.data.annotation.CreatedDate;
+import org.springframework.data.annotation.LastModifiedDate;
+import org.springframework.data.jpa.domain.support.AuditingEntityListener;
+
+/**
+ * Represents a goal entity in the system.
+ * Goals are user-defined objectives that can be tracked and completed through quests.
+ */
 @Entity
 @Getter
 @Table(name = "goal")
+@EntityListeners(AuditingEntityListener.class)
 public class Goal {
     // ... existing fields ...
+
+    @CreatedDate
+    @Column(updatable = false)
+    private LocalDateTime createdAt;
+
+    @LastModifiedDate
+    private LocalDateTime updatedAt;
src/main/java/com/groom/orbit/goal/dao/entity/Quest.java (4)

22-32: Consider using database-level default for isComplete.

While setting the default value in the entity is valid, consider also adding a database-level default constraint to ensure consistency even when bypassing JPA.

ALTER TABLE quest ALTER COLUMN is_complete SET DEFAULT false;

41-47: Specify fetch strategy and cascade behavior.

Consider adding:

  1. Explicit LAZY fetch strategy for better performance
  2. OnDelete cascade specification for referential integrity
- @ManyToOne
+ @ManyToOne(fetch = FetchType.LAZY)
+ @OnDelete(action = OnDeleteAction.CASCADE)
  @JoinColumn(name = "member_id", nullable = false)
  private Member member;

- @ManyToOne
+ @ManyToOne(fetch = FetchType.LAZY)
+ @OnDelete(action = OnDeleteAction.CASCADE)
  @JoinColumn(name = "goal_id", nullable = false)
  private Goal goal;

Don't forget to add:

import org.hibernate.annotations.OnDelete;
import org.hibernate.annotations.OnDeleteAction;
import jakarta.persistence.FetchType;

49-53: Consider alternatives to shadow fields.

While shadow fields can be useful for queries, they increase maintenance burden. Consider:

  1. Using @Formula for derived properties
  2. Creating getter methods that access the relationship
  3. Using projections in your repositories

Example alternative:

// Option 1: Using @Formula
@Formula("member_id")
private Long derivedMemberId;

// Option 2: Using getter method
public Long getMemberId() {
    return member != null ? member.getId() : null;
}

20-20: Add version field for optimistic locking.

Since this entity might be subject to concurrent updates (e.g., completing quests, updating notifications), consider adding optimistic locking:

public class Quest extends BaseTimeEntity {
+    @Version
+    private Long version;

Don't forget to add:

import jakarta.persistence.Version;
src/main/java/com/groom/orbit/goal/dao/entity/MemberGoal.java (2)

46-52: Enhance the factory method with validation and initialization.

The factory method should validate inputs and initialize all fields.

Consider this improvement:

  public static MemberGoal create(Member member, Goal goal) {
+   if (member == null || goal == null) {
+     throw new IllegalArgumentException("Member and Goal must not be null");
+   }
    MemberGoal memberGoal = new MemberGoal();
    memberGoal.member = member;
    memberGoal.goal = goal;
+   memberGoal.isComplete = false;

    return memberGoal;
  }

1-59: Consider adding audit fields for goal tracking analytics.

For a goal tracking system, it would be valuable to track when goals were assigned and completed. Consider adding audit fields like createdAt and completedAt to enable analytics and progress tracking features.

Example fields to consider:

@Column(nullable = false, updatable = false)
private LocalDateTime createdAt;

@Column
private LocalDateTime completedAt;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 7b37529 and d0078d7.

📒 Files selected for processing (11)
  • sql/init.sql (0 hunks)
  • src/main/java/com/groom/orbit/config/web/JacksonConfig.java (1 hunks)
  • src/main/java/com/groom/orbit/goal/app/QuestQueryService.java (1 hunks)
  • src/main/java/com/groom/orbit/goal/app/dto/QuestInfoResponseDto.java (1 hunks)
  • src/main/java/com/groom/orbit/goal/controller/QuestQueryController.java (1 hunks)
  • src/main/java/com/groom/orbit/goal/dao/QuestRepository.java (1 hunks)
  • src/main/java/com/groom/orbit/goal/dao/entity/Goal.java (1 hunks)
  • src/main/java/com/groom/orbit/goal/dao/entity/MemberGoal.java (1 hunks)
  • src/main/java/com/groom/orbit/goal/dao/entity/MemberGoalId.java (1 hunks)
  • src/main/java/com/groom/orbit/goal/dao/entity/Quest.java (1 hunks)
  • src/main/java/com/groom/orbit/job/controller/JobCommandController.java (1 hunks)
💤 Files with no reviewable changes (1)
  • sql/init.sql
✅ Files skipped from review due to trivial changes (3)
  • src/main/java/com/groom/orbit/config/web/JacksonConfig.java
  • src/main/java/com/groom/orbit/goal/app/dto/QuestInfoResponseDto.java
  • src/main/java/com/groom/orbit/job/controller/JobCommandController.java
🔇 Additional comments (5)
src/main/java/com/groom/orbit/goal/dao/QuestRepository.java (1)

11-11: Consider adding documentation and index optimization

The method follows Spring Data JPA naming conventions correctly. However, consider these improvements:

  1. Add Javadoc to document the method's purpose and parameters
  2. Consider adding @query annotation for better control over the generated query
  3. Ensure proper database indexing for memberId and goalId columns for query performance

Add documentation and optional @query annotation:

+  /**
+   * Retrieves all quests associated with a specific member and goal.
+   * @param memberId the ID of the member
+   * @param goalId the ID of the goal
+   * @return list of quests matching the criteria
+   */
+  @Query("SELECT q FROM Quest q WHERE q.member.id = :memberId AND q.goal.id = :goalId")
   List<Quest> findByMemberIdAndGoalId(Long memberId, Long goalId);
#!/bin/bash
# Description: Check for existing database indexes on quest table
# Expected: Find SQL files that might contain index definitions

# Search for SQL files containing quest table definition or modifications
rg -l "CREATE TABLE.*quest|ALTER TABLE.*quest" --type sql
src/main/java/com/groom/orbit/goal/app/QuestQueryService.java (1)

20-20: Verify repository method implementation.

Ensure that the repository method findByMemberIdAndGoalId is properly defined in the QuestRepository interface.

✅ Verification successful

Method is correctly defined in repository interface

The repository method findByMemberIdAndGoalId is properly defined in QuestRepository interface with the correct signature:

  • Takes two parameters: Long memberId, Long goalId
  • Returns List<Quest>
  • Extends JpaRepository<Quest, Long>
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the repository method definition

# Search for the repository interface and its method definition
ast-grep --pattern 'interface QuestRepository {
  $$$
  List<Quest> findByMemberIdAndGoalId($$$);
  $$$
}'

Length of output: 108


Script:

#!/bin/bash
# Let's try a different approach to find the repository interface and method

# First, find the repository file
fd -e java "QuestRepository"

# Then let's use ripgrep to search for the method definition
rg "findByMemberIdAndGoalId" -A 2 -B 2

Length of output: 1018

src/main/java/com/groom/orbit/goal/controller/QuestQueryController.java (2)

21-21: LGTM! Proper dependency injection.

The use of constructor injection with final field follows Spring best practices.


23-28: ⚠️ Potential issue

Address security, validation, and error handling concerns.

Several critical issues need to be addressed:

  1. The TODO comment suggests missing annotation - likely referring to security annotation
  2. The memberId parameter lacks proper annotation and source
  3. Missing input validation
  4. No error handling for invalid inputs or not found scenarios

Apply these improvements:

-  /** TODO add annotation */
+  @Operation(summary = "Find quests by goal ID", description = "Retrieves all quests for a given goal")
+  @SecurityRequirement(name = "Bearer Authentication")
+  @PreAuthorize("@memberSecurityService.isResourceOwner(#memberId)")
   @GetMapping
   public ResponseDto<List<QuestInfoResponseDto>> findQuest(
-      Long memberId, @RequestParam("goal_id") Long goalId) {
+      @Parameter(hidden = true) @AuthenticationPrincipal Long memberId,
+      @RequestParam("goal_id") @Positive(message = "Goal ID must be positive") Long goalId) {
+    if (memberId == null) {
+      throw new UnauthorizedException("User must be authenticated");
+    }
     return ResponseDto.ok(questQueryService.findQuestsByGoalId(memberId, goalId));
   }

Let's verify if proper security measures are implemented elsewhere:

src/main/java/com/groom/orbit/goal/dao/entity/Quest.java (1)

1-20: LGTM! Clean class setup with proper inheritance.

The class is well-structured with appropriate annotations and follows JPA entity best practices.

Comment on lines +8 to +11
public class MemberGoalId implements Serializable {
private Long member;
private Long goal;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

ID class fields need to match entity's ID column names

The MemberGoalId class fields don't match the column names used in the MemberGoal entity. The fields should be named memberId and goalId to match the @JoinColumn annotations in the entity class.

  • Change field names in MemberGoalId.java:
public class MemberGoalId implements Serializable {
  private Long memberId;  // instead of member
  private Long goalId;    // instead of goal
}
🔗 Analysis chain

Verify ID class usage and database mapping

Let's verify that this ID class is properly configured with its corresponding entity class and database schema.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for MemberGoal entity class and its ID class usage
ast-grep --pattern 'class MemberGoal {
  $$$
}'

# Check for database table definition
rg -g '*.sql' -g '*.java' 'CREATE TABLE.*member_goal|@Table.*member_goal'

Length of output: 254


Script:

#!/bin/bash
# Check the content of MemberGoal entity class
cat src/main/java/com/groom/orbit/goal/dao/entity/MemberGoal.java

# Check the table definition in SQL
rg -A 10 'CREATE TABLE member_goal' sql/init.sql

Length of output: 2008


⚠️ Potential issue

Add essential components for ID class functionality

The class is missing required components to function properly as a JPA ID class:

  1. Needs a no-args constructor for JPA
  2. Needs getters/setters or @DaTa annotation
  3. Should include serialVersionUID for serialization compatibility

Apply these changes:

 @EqualsAndHashCode
 public class MemberGoalId implements Serializable {
+    private static final long serialVersionUID = 1L;
+
     private Long member;
     private Long goal;
+
+    public MemberGoalId() {}
+
+    public MemberGoalId(Long member, Long goal) {
+        this.member = member;
+        this.goal = goal;
+    }
+
+    public Long getMember() {
+        return member;
+    }
+
+    public void setMember(Long member) {
+        this.member = member;
+    }
+
+    public Long getGoal() {
+        return goal;
+    }
+
+    public void setGoal(Long goal) {
+        this.goal = goal;
+    }
 }

Alternatively, you could use Lombok's @Data annotation instead of manually adding getters/setters:

-@EqualsAndHashCode
+@Data
 public class MemberGoalId implements Serializable {
     private static final long serialVersionUID = 1L;
 
     private Long member;
     private Long goal;
 
     public MemberGoalId() {}
 
     public MemberGoalId(Long member, Long goal) {
         this.member = member;
         this.goal = goal;
     }
 }
📝 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.

Suggested change
public class MemberGoalId implements Serializable {
private Long member;
private Long goal;
}
@Data
public class MemberGoalId implements Serializable {
private static final long serialVersionUID = 1L;
private Long member;
private Long goal;
public MemberGoalId() {}
public MemberGoalId(Long member, Long goal) {
this.member = member;
this.goal = goal;
}
}


import com.groom.orbit.goal.dao.entity.Quest;

public interface QuestRepository extends JpaRepository<Quest, Long> {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Quest entity requires proper relationship mappings with Member and Goal

Based on the verification results:

  • Quest entity is properly defined with basic fields and JPA annotations
  • SQL schema shows foreign keys member_id and goal_id
  • However, the Quest entity is missing JPA relationship mappings (@ManyToOne/@joincolumn) for these foreign keys
  • This mismatch could cause issues with JPA's entity management

Required changes in Quest entity:

  • Add @manytoone relationship with Member entity
  • Add @manytoone relationship with Goal entity
  • Add corresponding @joincolumn annotations for member_id and goal_id
🔗 Analysis chain

Verify Quest entity structure and relationships

The repository interface is correctly defined. However, let's verify the Quest entity structure to ensure proper mapping.


Let me gather more information about the Quest entity and its relationships.


Let me gather one final piece of information to check the relationship mappings in the Quest entity.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify Quest entity structure and its relationships
# Expected: Find Quest entity with proper JPA annotations and required fields

# Search for Quest entity definition
ast-grep --pattern 'class Quest {
  $$$
}'

# Search for related entities referenced by Quest
rg -l "class (Member|Goal)" --type java

Length of output: 513


Script:

#!/bin/bash
# Search for Quest entity definition and its annotations
rg -A 20 "class Quest" --type java

# Search for Quest table definition in SQL
rg "CREATE TABLE.*quest" -A 10 --type sql

# Search for relationships with Member and Goal
rg "Quest.*Member|Quest.*Goal|Member.*Quest|Goal.*Quest" --type java

Length of output: 5602


Script:

#!/bin/bash
# Search for JPA relationship annotations in Quest entity
rg -A 5 "@(ManyToOne|OneToMany|OneToOne|ManyToMany).*Quest|@JoinColumn.*Quest" --type java

# Search for foreign key constraints in SQL
rg "FOREIGN KEY.*quest" --type sql

Length of output: 129

Comment on lines +19 to +28
public List<QuestInfoResponseDto> findQuestsByGoalId(Long memberId, Long goalId) {
List<Quest> quests = questRepository.findByMemberIdAndGoalId(memberId, goalId);

return quests.stream()
.map(
quest ->
new QuestInfoResponseDto(
quest.getQuestId(), quest.getTitle(), quest.getIsComplete()))
.toList();
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add input validation, error handling, and pagination.

The current implementation has several areas for improvement:

  1. Add input validation for memberId and goalId
  2. Implement error handling for repository exceptions
  3. Consider adding pagination for large result sets
  4. Add method documentation

Here's a suggested implementation:

+/**
+ * Retrieves a list of quests associated with a specific goal and member.
+ *
+ * @param memberId The ID of the member
+ * @param goalId The ID of the goal
+ * @return List of QuestInfoResponseDto containing quest information
+ * @throws IllegalArgumentException if memberId or goalId is null or negative
+ * @throws QuestNotFoundException if no quests are found
+ */
 public List<QuestInfoResponseDto> findQuestsByGoalId(Long memberId, Long goalId) {
+    if (memberId == null || memberId < 0) {
+        throw new IllegalArgumentException("Member ID must be a positive number");
+    }
+    if (goalId == null || goalId < 0) {
+        throw new IllegalArgumentException("Goal ID must be a positive number");
+    }
+
+    try {
         List<Quest> quests = questRepository.findByMemberIdAndGoalId(memberId, goalId);
+        
+        if (quests.isEmpty()) {
+            throw new QuestNotFoundException("No quests found for member " + memberId + " and goal " + goalId);
+        }

         return quests.stream()
             .map(
                 quest ->
                     new QuestInfoResponseDto(
                         quest.getQuestId(), quest.getTitle(), quest.getIsComplete()))
             .toList();
+    } catch (Exception e) {
+        throw new QuestServiceException("Error retrieving quests", e);
+    }
 }

Consider implementing pagination for better performance with large datasets:

public Page<QuestInfoResponseDto> findQuestsByGoalId(Long memberId, Long goalId, Pageable pageable) {
    // Implementation with pagination
}

Comment on lines +30 to +32
@ColumnDefault("0")
@Column(nullable = false)
private Integer count;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation for count field and consider type optimization.

  1. Add validation to prevent negative values
  2. Consider using a more appropriate type if count is just for simple increments
+import jakarta.validation.constraints.PositiveOrZero;

 @ColumnDefault("0")
 @Column(nullable = false)
+@PositiveOrZero(message = "Count cannot be negative")
 private Integer count;

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +14 to +17
@Entity
@Getter
@Table(name = "goal")
public class Goal {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add essential JPA annotations for better entity management.

Consider adding the following annotations:

  1. @NoArgsConstructor: Required by JPA for entity instantiation
  2. @Version: For optimistic locking to prevent concurrent modifications
 @Entity
 @Getter
 @Table(name = "goal")
+@NoArgsConstructor(access = AccessLevel.PROTECTED)
 public class Goal {
+    @Version
+    private Long version;

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +35 to +39
@Column(nullable = false)
private Integer sequence;

@Column(nullable = false, columnDefinition = "false")
private Boolean isNotificationSend = false;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation and simplify boolean definition.

  1. The sequence field should validate for positive values
  2. The isNotificationSend definition can be simplified
  @Column(nullable = false)
+ @Min(0)
  private Integer sequence;

- @Column(nullable = false, columnDefinition = "false")
+ @Column(nullable = false)
  private Boolean isNotificationSend = false;

Don't forget to add:

import jakarta.validation.constraints.Min;

@Column(nullable = false)
private Boolean isComplete = false;

private java.sql.Timestamp deadline;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Modernize date-time handling and clarify nullability.

Consider these improvements:

  1. Replace java.sql.Timestamp with java.time.LocalDateTime for better date-time handling
  2. Explicitly specify nullability constraint
-  private java.sql.Timestamp deadline;
+  @Column(nullable = true)
+  private java.time.LocalDateTime deadline;
📝 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.

Suggested change
private java.sql.Timestamp deadline;
@Column(nullable = true)
private java.time.LocalDateTime deadline;

Comment on lines +36 to +38
@ColumnDefault("false")
@Column(nullable = false)
private Boolean isComplete;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Initialize isComplete field and use primitive boolean.

Two suggestions for the completion status field:

  1. @ColumnDefault only affects the database column. Initialize the field in Java too.
  2. Use primitive boolean instead of Boolean since it's marked as non-null.

Apply this change:

  @ColumnDefault("false")
  @Column(nullable = false)
- private Boolean isComplete;
+ private boolean isComplete = false;
📝 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.

Suggested change
@ColumnDefault("false")
@Column(nullable = false)
private Boolean isComplete;
@ColumnDefault("false")
@Column(nullable = false)
private boolean isComplete = false;

Comment on lines +54 to +58
public void validateMember(Member member) {
if (!memberId.equals(member.getId())) {
throw new CommonException(ErrorCode.ACCESS_DENIED);
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add null check and improve error message in validation.

The validation method should handle null input and provide more context in the error.

Consider this improvement:

  public void validateMember(Member member) {
+   if (member == null) {
+     throw new IllegalArgumentException("Member must not be null");
+   }
    if (!memberId.equals(member.getId())) {
-     throw new CommonException(ErrorCode.ACCESS_DENIED);
+     throw new CommonException(ErrorCode.ACCESS_DENIED,
+       String.format("Member %d not authorized to access goal %d", member.getId(), goalId));
    }
  }

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +20 to +24
@Entity
@Getter
@Table(name = "member_goal")
@IdClass(MemberGoalId.class)
public class MemberGoal {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add a protected default constructor for JPA.

JPA requires a default no-args constructor. While Lombok can generate one, it's better to explicitly declare it for JPA entities.

Add this constructor:

@IdClass(MemberGoalId.class)
public class MemberGoal {
+    protected MemberGoal() {
+        // Required by JPA
+    }
📝 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.

Suggested change
@Entity
@Getter
@Table(name = "member_goal")
@IdClass(MemberGoalId.class)
public class MemberGoal {
@Entity
@Getter
@Table(name = "member_goal")
@IdClass(MemberGoalId.class)
public class MemberGoal {
protected MemberGoal() {
// Required by JPA
}

@koosco koosco merged commit 66b110d into dev Nov 18, 2024
1 check passed
@koosco koosco deleted the feat/#40 branch December 6, 2024 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Quest 리스트 조회
1 participant