Skip to content

Conversation

@kwon204
Copy link
Contributor

@kwon204 kwon204 commented Mar 31, 2025

#️⃣ 연관된 이슈>

📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)

  • long polling api를 구현했습니다.
    • 타임아웃은 1분으로 설정했습니다.
    • 타임아웃 전에 이벤트가 발생하면 "sync"를 응답합니다.
    • 타임아웃 전에 이벤트가 발생하면 변경된 일정 데이터를 응답합니다.
    • 타임아웃이 일어나면 "timeout"을 응답합니다.
    • "sync" 응답을 받으면 syncToken과 함께 get events 요청을 하여 변경된 이벤트를 받습니다.
  • 동기화 시 필요한 syncToken을 구현했습니다.
    • 토큰에 저장된 시간데이터를 사용하여 마지막 동기화 시각 이후에 변경된 데이터를 응답합니다.

🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요

  • 삭제된 이벤트에 대한 동작이 부실하다고 생각되는데 좋은 의견이 있다면 공유하면 좋을 거 같습니다!
  • => 삭제된 일정에 대해서는 다음의 응답이 전송됩니다.
{
	"googleEventId": "id",
	"titile": "title",
	"startDateTime":  "2025-04-06:12:00:00",
	"endDateTime": "2025-04-06:12:10:00" ,
	"status": "cancelled"
}

Summary by CodeRabbit

  • New Features

    • Introduced a new endpoint to enable real-time synchronization of personal events for more responsive updates.
    • Added a new record for synchronization responses, enhancing the structure of event data handling.
  • Refactor

    • Streamlined integration of external calendar events with improved asynchronous processing.
    • Replaced previous real-time subscription functionality with a more efficient notification mechanism, simplifying event updates.
  • Bug Fixes

    • Adjusted method implementations for consistency and clarity in handling personal events.
  • Tests

    • Updated test cases to align with the refined event synchronization logic.

@kwon204 kwon204 added the 🛠️ BE Backend label Mar 31, 2025
@kwon204 kwon204 added this to the 💪8차 스프린트 milestone Mar 31, 2025
@kwon204 kwon204 self-assigned this Mar 31, 2025
@kwon204 kwon204 requested a review from efdao as a code owner March 31, 2025 01:22
@coderabbitai
Copy link

coderabbitai bot commented Mar 31, 2025

Walkthrough

This pull request introduces several changes to the personal events synchronization flow. In the backend, new asynchronous handling is implemented using a DeferredResultManager and a new /sync endpoint via an added poll() method. The changes update and simplify methods in both the controller and service layers, modify the control flow in event synchronization, and remove the previous SSE-based implementation. Additionally, new DTO records (SyncPersonalEvent and SyncResponse) have been added, and tests adjusted accordingly.

Changes

File(s) Change Summary
backend/.../PersonalEventController.java Added new imports, fields (DeferredResultManager, UserService), and a new poll() method; modified getPersonalEvents implementation for formatting consistency.
backend/.../PersonalEventService.java Updated method signatures: changed the return type of syncWithGoogleEvents to void and removed changedDates parameter from upsertPersonalEventByGoogleEvent and deletePersonalEventByGoogleEvent; improved switch-based control flow; adjusted parameter formatting in listPersonalEvents.
backend/.../dto/SyncPersonalEvent.java Introduced new record SyncPersonalEvent with five fields and a static factory method from(GoogleEvent event).
backend/.../event/handler/PersonalEventHandler.java Replaced SseEmitters with DeferredResultManager; updated sync method to create and set results using SyncResponse.sync instead of SSE handling.
backend/.../dto/SyncResponse.java Added new record SyncResponse with two static factory methods: timeout() and sync(List<SyncPersonalEvent> events).
backend/.../global/util/DeferredResultManager.java Added new service class to manage asynchronous DeferredResult instances for user responses.
backend/.../CalendarEventController.java and backend/.../SseEmitters.java Removed classes related to SSE-based event streaming and subscriptions.
backend/.../PersonalEventServiceTest.java Commented out mock setups for event end times in the test, modifying assertions accordingly.

Possibly related PRs

Suggested reviewers

  • efdao

Poem

I'm a bunny in the code field, hopping with delight,
Syncing events with a flick, turning old to new at night.
With deferred results that race,
I leave behind the SSE trace.
A joyful hop, a quirky beat—changes make my day so bright!
🐰✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 04c9a8a and 7e115ba.

📒 Files selected for processing (1)
  • backend/src/main/java/endolphin/backend/global/util/DeferredResultManager.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • backend/src/main/java/endolphin/backend/global/util/DeferredResultManager.java

🪧 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 generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @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: 2

🧹 Nitpick comments (15)
backend/src/test/java/endolphin/backend/global/util/SyncTokenUtilTest.java (1)

11-17: Test verifies token generation but could be more specific.

The test confirms that a token is generated and is not blank, which is good. However, you might consider adding assertions that verify additional properties of the generated token, such as format validation or expected length.

 @Test
 public void generateSyncTokenTest() {
     LocalDateTime now = LocalDateTime.now();
     String token = SyncTokenUtil.generateSyncToken(now);
 
     assertThat(token).isNotBlank();
+    // Check that token is valid Base64
+    assertThat(token).matches("^[A-Za-z0-9+/]*={0,2}$");
 }
backend/src/main/java/endolphin/backend/domain/personal_event/event/handler/PersonalEventHandler.java (1)

38-38: Error handling for notification may be needed.

The notification call to the controller is outside the try-catch block that handles other errors. Consider adding specific error handling for the notification call to ensure failures don't impact the rest of the sync process.

     try {
         List<GoogleEvent> events = event.events();
         User user = event.user();
         log.info("Syncing personal events for user {}", user.getId());
         String googleCalendarId = event.googleCalendarId();
         Set<LocalDate> changedDates = personalEventService.syncWithGoogleEvents(events, user,
             googleCalendarId);

         sseEmitters.sendToUser(user.getId(), changedDates);
+        try {
             personalEventController.notify(user, "sync");
+        } catch (Exception e) {
+            log.error("Failed to notify user of sync completion", e);
+        }
     } catch (Exception e) {
         log.error("Failed to sync personal events", e);
     }
backend/src/main/java/endolphin/backend/global/util/SyncTokenUtil.java (1)

7-21: Consider handling null or empty tokens.

The utility methods don't check for null or empty input values, which could lead to NullPointerExceptions or meaningless tokens.

 public static String generateSyncToken(LocalDateTime dateTime) {
+    if (dateTime == null) {
+        throw new IllegalArgumentException("DateTime cannot be null");
+    }
     String tokenData = dateTime.toString();
 
     return Base64.getEncoder().encodeToString(tokenData.getBytes(StandardCharsets.UTF_8));
 }

 public static LocalDateTime getLastSyncTime(String token) {
+    if (token == null || token.isEmpty()) {
+        throw new IllegalArgumentException("Token cannot be null or empty");
+    }
     try {
         byte[] decodedBytes = Base64.getDecoder().decode(token);
         String decoded = new String(decodedBytes, StandardCharsets.UTF_8);
 
         return LocalDateTime.parse(decoded);
     } catch (IllegalArgumentException e) {
         // Log the error
         throw new IllegalArgumentException("Invalid sync token format", e);
     }
 }
backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventRepository.java (1)

67-74: Consider indexing updatedAt for performance.

If updatedAt is frequently queried for synchronization, adding a database index on this field could significantly enhance performance for large data volumes.

backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventServiceTest.java (2)

92-93: Suggest verifying sync token behavior in tests.

You might consider asserting on the returned sync token to ensure it is correctly generated and utilized, adding clarity to the syncing logic under test.


96-96: Improve test clarity.

Consider including an explanatory message in the assertion to clarify why the size is expected to be exactly 2, aiding future maintainers.

backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventService.java (8)

60-84: Refined listPersonalEvents for synchronization.

Breaking out logic for initial vs. incremental sync is clear. Consider logging the sync path chosen to help diagnose issues in production.


109-109: Preprocessing loop calls.

If there is a risk of partial failures mid-loop, consider adding handling or logging per iteration for resilience.

Also applies to: 111-111


119-137: createPersonalEventsForParticipants method.

Extracting participant event creation into a helper method or builder could reduce duplication and improve clarity.


172-176: syncUpdate condition.

As a minor enhancement, consider using an early return style to simplify the conditional.

-private boolean syncUpdate(...) {
-    return request.syncWithGoogleCalendar() && ( isDateTimeChanged(...) || ... );
+private boolean syncUpdate(...) {
+    if (!request.syncWithGoogleCalendar()) {
+        return false;
+    }
+    return isDateTimeChanged(...) || !StringUtils.equals(...);
+}

185-198: updatePersonalEvent method.

Capturing the old event for preprocessing is helpful. Logging changes here could aid in debugging user-reported issues.


209-209: deleteWithRequest sync deletion logic.

Consider extracting a helper method to handle the Google calendar deletion for clarity and potential future expansion.

Also applies to: 214-215


243-243: syncWithGoogleEvents expansion potential.

If synchronization logic becomes more complex, moving it into its own service or utility class could keep PersonalEventService concise.

Also applies to: 245-245


269-290: Upsert flow for Google events.

Returning the newly created or updated PersonalEvent might help avoid re-fetching, but this is optional for now.

backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventController.java (1)

156-159: Prevent NullPointerException by verifying map retrieval
If no DeferredResult is found in the map for the given user, calling setResult on a null reference will raise a NullPointerException. Consider adding a null check.

-        DeferredResult<String> deferredResult = deferredResults.get(user.getId());
-        deferredResult.setResult(response);
+        DeferredResult<String> deferredResult = deferredResults.get(user.getId());
+        if (deferredResult != null) {
+            deferredResult.setResult(response);
+        }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5aa4921 and 8d8f6dde1d072c4f26439e64d17561bed0b6b0ef.

📒 Files selected for processing (8)
  • backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventController.java (6 hunks)
  • backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventRepository.java (1 hunks)
  • backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventService.java (12 hunks)
  • backend/src/main/java/endolphin/backend/domain/personal_event/dto/SyncPersonalEvent.java (1 hunks)
  • backend/src/main/java/endolphin/backend/domain/personal_event/event/handler/PersonalEventHandler.java (3 hunks)
  • backend/src/main/java/endolphin/backend/global/util/SyncTokenUtil.java (1 hunks)
  • backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventServiceTest.java (2 hunks)
  • backend/src/test/java/endolphin/backend/global/util/SyncTokenUtilTest.java (1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
backend/src/test/java/endolphin/backend/global/util/SyncTokenUtilTest.java (1)
backend/src/main/java/endolphin/backend/global/util/SyncTokenUtil.java (1)
  • SyncTokenUtil (7-21)
🔇 Additional comments (21)
backend/src/main/java/endolphin/backend/domain/personal_event/dto/SyncPersonalEvent.java (1)

5-9: Record implementation looks clean and appropriate.

The SyncPersonalEvent record provides a clean way to encapsulate the list of events along with their synchronization token. This immutable data structure is well-suited for this purpose and follows Java best practices for DTOs.

backend/src/test/java/endolphin/backend/global/util/SyncTokenUtilTest.java (1)

19-28: Test properly verifies token parsing.

This test correctly checks if the token can be parsed back to the original LocalDateTime. Good job!

backend/src/main/java/endolphin/backend/domain/personal_event/event/handler/PersonalEventHandler.java (1)

23-23: Injected dependency looks correct.

Adding the PersonalEventController as a dependency allows for notifying the controller when sync events occur.

backend/src/main/java/endolphin/backend/global/util/SyncTokenUtil.java (1)

9-13: Token generation implementation is simple but effective.

The implementation uses Base64 encoding which is appropriate for a sync token. It's simple, readable, and serves the purpose well.

backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventServiceTest.java (1)

7-7: New import looks good.

backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventService.java (9)

9-9: Imports for sync logic.

These imports appropriately bring in utilities for generating and parsing sync tokens.

Also applies to: 25-26, 28-28


92-98: Builder pattern usage.

Using the request fields directly ensures clarity. No issues found here.


144-145: updateWithRequest includes sync logic.

Implementation properly handles the decision between inserting or updating Google events based on existing IDs. Looks good.

Also applies to: 157-157


178-182: syncInsert logic is solid.

Checks for missing googleEventId and calendarId before insertion.


232-232: Personal event not found exception.

Raising an ApiException with a well-defined ErrorCode is consistent with the rest of the error handling strategy.


237-237: preprocessPersonalEvents usage.

Relying on discussion.getDateRangeStart()/End() is logically consistent.


307-308: isDateTimeChanged correctness.

Straightforward comparison of start/end times is correct.


345-350: restorePersonalEvents method.

The existing approach properly re-preprocesses overlapping events after fetching them by discussion ID.


293-303:

Details

✅ Verification successful

deletePersonalEventByGoogleEvent verification needed.

Check that we never encounter partially deleted or missing events.


🏁 Script executed:

#!/bin/bash
# Searching references to confirm if partial or missing events are handled gracefully.
rg -A 5 "deletePersonalEventByGoogleEvent"

Length of output: 1778


deletePersonalEventByGoogleEvent deletion flow verified.

The grep search confirms that the method is only invoked when a corresponding event is found via an Optional check. This means that deletion is only attempted on fully fetched events, preventing any partial deletion due to missing events. One point to double-check is that the inner call to deletePersonalEvent is handled transactionally (or with proper error management) to ensure the complete deletion of an event without side effects.

backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventController.java (7)

5-7: Imports for new functionality appear correct and necessary.
No concerns here.


21-23: Usage of Map and ConcurrentHashMap
Importing these classes is appropriate for storing and managing concurrency-sensitive data structures.


35-35: Import for DeferredResult
DeferredResult is suitable for asynchronous request handling.


44-44: UserService injection
Injecting UserService to fetch user's information looks correct.


60-65: Validate syncToken parameter
Ensure that the syncToken parameter is not empty or malformed, as any invalid token could lead to unexpected behaviors or errors during event retrieval.


129-131: ConcurrentHashMap concurrency considerations
Storing a single DeferredResult per user may overwrite any existing DeferredResult if a second poll request is made by the same user. Verify if only one active poll request per user is expected.


133-154: Check multiple poll requests and error handling
If multiple concurrent poll requests are initiated by the same user, the last-saved DeferredResult will take precedence. Also, confirm how userService.getCurrentUser() behaves if the user is unauthenticated, to avoid unexpected exceptions.

Comment on lines 15 to 20
Copy link

@coderabbitai coderabbitai bot Mar 31, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding error handling for token parsing.

The getLastSyncTime method doesn't handle potential exceptions that could occur when decoding an invalid Base64 string or parsing an invalid LocalDateTime format. This could lead to unexpected runtime exceptions.

 public static LocalDateTime getLastSyncTime(String token) {
+    try {
         byte[] decodedBytes = Base64.getDecoder().decode(token);
         String decoded = new String(decodedBytes, StandardCharsets.UTF_8);
 
         return LocalDateTime.parse(decoded);
+    } catch (IllegalArgumentException e) {
+        // Log the error
+        throw new IllegalArgumentException("Invalid sync token format", e);
+    }
 }
📝 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 static LocalDateTime getLastSyncTime(String token) {
byte[] decodedBytes = Base64.getDecoder().decode(token);
String decoded = new String(decodedBytes, StandardCharsets.UTF_8);
return LocalDateTime.parse(decoded);
}
public static LocalDateTime getLastSyncTime(String token) {
try {
byte[] decodedBytes = Base64.getDecoder().decode(token);
String decoded = new String(decodedBytes, StandardCharsets.UTF_8);
return LocalDateTime.parse(decoded);
} catch (IllegalArgumentException e) {
// Log the error
throw new IllegalArgumentException("Invalid sync token format", e);
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

p1; 토큰 검증, 에러 처리가 필요해보입니다.

Copy link

Choose a reason for hiding this comment

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

@efdao 네, 토큰 검증과 에러 처리를 추가해야 합니다. 다음과 같이 getLastSyncTime 메소드를 개선할 수 있습니다:

public static LocalDateTime getLastSyncTime(String token) {
    // 토큰이 null이거나 비어있는지 확인
    if (token == null || token.isBlank()) {
        throw new IllegalArgumentException("Sync token cannot be null or empty");
    }
    
    try {
        // Base64 디코딩 예외 처리
        byte[] decodedBytes = Base64.getDecoder().decode(token);
        String decoded = new String(decodedBytes, StandardCharsets.UTF_8);
        
        // LocalDateTime 파싱 예외 처리
        return LocalDateTime.parse(decoded);
    } catch (IllegalArgumentException e) {
        // Base64 디코딩 오류
        throw new IllegalArgumentException("Invalid sync token format: Cannot decode Base64", e);
    } catch (DateTimeParseException e) {
        // 날짜/시간 파싱 오류
        throw new IllegalArgumentException("Invalid sync token format: Cannot parse date time", e);
    }
}

이 구현은 다음 사항을 처리합니다:

  1. 토큰의 null 및 빈 문자열 검증
  2. Base64 디코딩 예외 처리
  3. 날짜/시간 형식 파싱 예외 처리

각 예외에 명확한 오류 메시지를 포함시켜 디버깅을 용이하게 했습니다.


✏️ Learnings added
Learnt from: efdao
PR: softeer5th/Team4-enDolphin#374
File: backend/src/main/java/endolphin/backend/global/util/SyncTokenUtil.java:15-20
Timestamp: 2025-04-01T06:00:40.922Z
Learning: In the SyncTokenUtil class, token validation and error handling should be implemented for the getLastSyncTime method to handle cases like null tokens, invalid Base64 encoded strings, and invalid date-time formats.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Comment on lines 312 to 341
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

findUserInfoWithPersonalEventsByUsers potential refactoring.

The logic is quite involved. Splitting map building, user processing, and final sorting into helper methods could improve readability.

Copy link
Collaborator

@efdao efdao left a comment

Choose a reason for hiding this comment

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

현재 구조가 클라이언트가 sync 응답을 받고 get events를 한 후 반복문으로 변경사항을 확인한 후 렌더링하게되는건가요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

p1; 최초 요청 시 syncToken을 전달받지 않는다면 required = false 설정해야 할 것 같습니다.

Comment on lines 15 to 20
Copy link
Collaborator

Choose a reason for hiding this comment

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

p1; 토큰 검증, 에러 처리가 필요해보입니다.

@kwon204
Copy link
Contributor Author

kwon204 commented Apr 2, 2025

현재 구조가 클라이언트가 sync 응답을 받고 get events를 한 후 반복문으로 변경사항을 확인한 후 렌더링하게되는건가요?

네 맞습니다. 그런데 다시 생각해보니 syncToken 없이도 할 수 있을 거 같아서 새로 수정 후 올리겠습니다

@kwon204 kwon204 force-pushed the feature/be/long-polling branch from 8d8f6dd to 09cdae8 Compare April 6, 2025 06:43
@kwon204 kwon204 requested a review from efdao April 6, 2025 06:44
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: 2

♻️ Duplicate comments (2)
backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventController.java (2)

131-144: ⚠️ Potential issue

Add missing syncToken parameter for synchronization

According to the PR objectives, there should be a way to receive a syncToken when retrieving changed events. Currently, the endpoint doesn't accept any parameters for this functionality.

Add a parameter for the syncToken that should be marked as optional since it won't be present on the first request.

    @GetMapping("/sync")
-    public DeferredResult<ResponseEntity<SyncResponse>> poll() {
+    public DeferredResult<ResponseEntity<SyncResponse>> poll(
+        @RequestParam(required = false) String syncToken) {
         User user = userService.getCurrentUser();
+        // TODO: Handle syncToken here
 
         return deferredResultManager.create(user);
     }

131-138: 🛠️ Refactor suggestion

Add missing Swagger documentation for syncToken parameter

Once you add the syncToken parameter, update the Swagger documentation to explain its purpose and usage.

    @Operation(summary = "개인 일정 동기화", description = "개인 일정을 실시간으로 동기화합니다.")
    @ApiResponses(value = {
        @ApiResponse(responseCode = "200", description = "일정 동기화 성공"),
        @ApiResponse(responseCode = "401", description = "인증 실패",
            content = @Content(schema = @Schema(implementation = ErrorResponse.class))),
        @ApiResponse(responseCode = "500", description = "서버 오류",
            content = @Content(schema = @Schema(implementation = ErrorResponse.class)))
    })
+    @io.swagger.v3.oas.annotations.Parameters({
+        @io.swagger.v3.oas.annotations.Parameter(name = "syncToken", description = "이전 동기화에서 받은 토큰. 첫 요청시 생략 가능.", required = false)
+    })
🧹 Nitpick comments (2)
backend/src/main/java/endolphin/backend/domain/personal_event/dto/SyncResponse.java (1)

5-15: Good use of factory methods for response creation

The implementation is clean and provides clear factory methods for the two response types. Using constants for the response types would make the code more maintainable:

-    public static SyncResponse timeout() {
-        return new SyncResponse(null, "timeout");
-    }
+    private static final String TYPE_TIMEOUT = "timeout";
+    private static final String TYPE_SYNC = "sync";
+
+    public static SyncResponse timeout() {
+        return new SyncResponse(null, TYPE_TIMEOUT);
+    }

-    public static SyncResponse sync(List<SyncPersonalEvent> events) {
-        return new SyncResponse(events, "sync");
-    }
+    public static SyncResponse sync(List<SyncPersonalEvent> events) {
+        return new SyncResponse(events, TYPE_SYNC);
+    }
backend/src/main/java/endolphin/backend/global/util/DeferredResultManager.java (1)

3-4: Remove unused import

The PersonalEventResponse import is not used anywhere in this class.

-import endolphin.backend.domain.personal_event.dto.PersonalEventResponse;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8d8f6dde1d072c4f26439e64d17561bed0b6b0ef and 09cdae8.

📒 Files selected for processing (11)
  • backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventController.java (6 hunks)
  • backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventService.java (5 hunks)
  • backend/src/main/java/endolphin/backend/domain/personal_event/dto/SyncPersonalEvent.java (1 hunks)
  • backend/src/main/java/endolphin/backend/domain/personal_event/dto/SyncResponse.java (1 hunks)
  • backend/src/main/java/endolphin/backend/domain/personal_event/event/handler/PersonalEventHandler.java (3 hunks)
  • backend/src/main/java/endolphin/backend/global/sse/CalendarEventController.java (0 hunks)
  • backend/src/main/java/endolphin/backend/global/sse/SseEmitters.java (0 hunks)
  • backend/src/main/java/endolphin/backend/global/util/DeferredResultManager.java (1 hunks)
  • backend/src/main/java/endolphin/backend/global/util/SyncTokenUtil.java (1 hunks)
  • backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventServiceTest.java (1 hunks)
  • backend/src/test/java/endolphin/backend/global/util/SyncTokenUtilTest.java (1 hunks)
💤 Files with no reviewable changes (2)
  • backend/src/main/java/endolphin/backend/global/sse/SseEmitters.java
  • backend/src/main/java/endolphin/backend/global/sse/CalendarEventController.java
✅ Files skipped from review due to trivial changes (1)
  • backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (4)
  • backend/src/main/java/endolphin/backend/global/util/SyncTokenUtil.java
  • backend/src/main/java/endolphin/backend/domain/personal_event/event/handler/PersonalEventHandler.java
  • backend/src/test/java/endolphin/backend/global/util/SyncTokenUtilTest.java
  • backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventService.java
🔇 Additional comments (3)
backend/src/main/java/endolphin/backend/domain/personal_event/dto/SyncPersonalEvent.java (1)

6-16: Good implementation of the DTO using Java records

The implementation of SyncPersonalEvent as a Java record is clean and follows modern Java practices. The static factory method from(GoogleEvent) provides a convenient way to convert from Google event data to your internal representation.

Two suggestions for consideration:

  1. Consider adding validation annotations if these fields have constraints
  2. You might want to add documentation explaining the possible values for the status field
backend/src/main/java/endolphin/backend/global/util/DeferredResultManager.java (1)

18-20: LGTM! Good use of ConcurrentHashMap

Using a ConcurrentHashMap is appropriate for this use case as it provides thread-safety for concurrent access from multiple user requests.

backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventController.java (1)

43-45: LGTM! Good field initialization

The new fields are properly initialized through constructor injection.

Copy link
Collaborator

@efdao efdao left a comment

Choose a reason for hiding this comment

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

코멘트 확인 부탁드립니다.

@kwon204 kwon204 requested a review from efdao April 8, 2025 09:15
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: 0

🧹 Nitpick comments (3)
backend/src/main/java/endolphin/backend/global/util/DeferredResultManager.java (1)

19-37: Add documentation about connection replacement behavior

The method correctly handles the case when a user already has an existing connection by replacing it with a "replaced" response. However, it would be helpful to add Javadoc comments to clearly document this behavior.

+    /**
+     * Creates a DeferredResult for the specified user.
+     * If a connection already exists for the user, it will be closed with a "replaced" response.
+     *
+     * @param user The user for whom to create a DeferredResult
+     * @return A new DeferredResult instance
+     */
     public DeferredResult<ResponseEntity<SyncResponse>> create(User user) {
backend/src/main/java/endolphin/backend/domain/personal_event/dto/SyncResponse.java (2)

5-8: Consider using enum for response types

Using string literals for the response types ("timeout", "sync", "replaced") works, but using an enum would provide better type safety and prevent potential typos in the codebase.

 package endolphin.backend.domain.personal_event.dto;
 
 import java.util.List;
 
+public enum SyncResponseType {
+    TIMEOUT, SYNC, REPLACED
+}
+
 public record SyncResponse(
         List<SyncPersonalEvent> events,
-        String type
+        SyncResponseType type
 ) {

Then update the factory methods to use the enum values.


17-19: Consider adding documentation for replaced responses

The factory method creates a "replaced" response correctly, but it might be helpful to add a comment explaining when this response should be used.

+    /**
+     * Creates a response indicating that an existing connection has been replaced by a new one.
+     * Used when a user creates a new connection while an existing one is still active.
+     *
+     * @return A SyncResponse with type "replaced" and null events
+     */
     public static SyncResponse replaced() {
         return new SyncResponse(null, "replaced");
     }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cf9d4fa and 04c9a8a.

📒 Files selected for processing (3)
  • backend/src/main/java/endolphin/backend/domain/personal_event/dto/SyncResponse.java (1 hunks)
  • backend/src/main/java/endolphin/backend/domain/personal_event/event/handler/PersonalEventHandler.java (3 hunks)
  • backend/src/main/java/endolphin/backend/global/util/DeferredResultManager.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • backend/src/main/java/endolphin/backend/domain/personal_event/event/handler/PersonalEventHandler.java
🔇 Additional comments (6)
backend/src/main/java/endolphin/backend/global/util/DeferredResultManager.java (4)

15-17: Good implementation of the map for storing DeferredResult instances.

Using a ConcurrentHashMap is appropriate here since multiple threads might access this map concurrently in a web application environment. The timeout is correctly set to 60000L (60 seconds), which aligns with the PR objectives for a one-minute timeout.


29-34: Well implemented cleanup for DeferredResult

Good practice to remove the DeferredResult from the map on both completion and timeout to prevent memory leaks.


39-45: Proper check before setting result

The condition correctly checks that the DeferredResult exists and is not already set or expired before setting a new result. This ensures that results are only set for active connections.


47-49: Simple and effective active connection check

The method provides a clean way to check for active connections by looking for the user's ID in the map.

backend/src/main/java/endolphin/backend/domain/personal_event/dto/SyncResponse.java (2)

9-11: Consistent handling of timeout responses

The factory method correctly returns a response with null events and a "timeout" type, matching the pattern established in the PR objectives.


13-15: Clear factory method for sync responses

This method appropriately creates a response with the provided events and sets the type to "sync", making it clear what kind of response is being created.

Copy link
Collaborator

@efdao efdao left a comment

Choose a reason for hiding this comment

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

고생하셨습니다

@kwon204 kwon204 merged commit a1a7318 into dev Apr 13, 2025
3 checks passed
@kwon204 kwon204 deleted the feature/be/long-polling branch April 13, 2025 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🛠️ BE Backend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants