873: Migrate session to use optional by alfardil · Pull Request #872 · Patina-Network/codebloom

alfardil · 2026-03-24T04:25:35Z

873 Description of changes

Migrate SessionRepository to use optional

Checklist before review

I have done a thorough self-review of the PR
Copilot has reviewed my latest changes, and all comments have been fixed and/or closed.
If I have made database changes, I have made sure I followed all the db repo rules listed in the wiki here. (check if no db changes)
All tests have passed
I have successfully deployed this PR to staging
I have done manual QA in both dev (and staging if possible) and attached screenshots below.

Screenshots

Dev

Staging

github-actions · 2026-03-24T04:26:06Z

Available PR Commands

/ai - Triggers all AI review commands at once
/review - AI review of the PR changes
/describe - AI-powered description of the PR
/improve - AI-powered suggestions
/deploy - Deploy to staging

See: https://github.com/tahminator/codebloom/wiki/CI-Commands

github-actions · 2026-03-24T04:27:21Z

Title

873: migrate session to use optional

PR Type

Enhancement

Description

Migrate Session.id field to Optional<String>.
Update SessionRepository methods to return Optional<Session>.
Adjust session creation and retrieval logic to handle Optional.
Refactor dependent components and tests for Optional usage.

Diagram Walkthrough

flowchart LR
  A["Session.java (Model)"] --> B["SessionRepository.java (Interface)"]
  B --> C["SessionSqlRepository.java (Implementation)"]
  B --> D["AuthController.java"]
  B --> E["CustomAuthenticationSuccessHandler.java"]
  B --> F["Protector.java"]
  A --> G["SessionDto.java"]

File Walkthrough

Relevant files

Enhancement

7 files

AuthController.java `Update logout to handle Optional session ID`	+4/-3
CustomAuthenticationSuccessHandler.java `Update cookie creation to handle Optional session ID`	+1/-1
Session.java `Change session ID to Optional and initialize as empty`	+3/-2
SessionRepository.java `Update getSessionById signature to return Optional`	+2/-1
SessionSqlRepository.java `Implement Optional handling for session ID in repository operations`	+13/-12
SessionDto.java `Update fromSession to retrieve session ID using orElseThrow`	+1/-1
Protector.java `Update validateSession to retrieve session using orElseThrow`	+1/-1

Tests

4 files

AuthControllerTest.java `Update AuthController tests for Optional session ID handling`	+6/-5
CustomAuthenticationSuccessHandlerTest.java `Update authentication success handler tests for Optional session ID`	+9/-9
SessionRepositoryTest.java `Update SessionRepository tests for Optional session and ID handling`	+7/-3
TestProtector.java `Update TestProtector to handle Optional session retrieval`	+3/-1

github-actions · 2026-03-24T04:27:36Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ No major issues detected

github-actions · 2026-03-24T20:28:09Z

Title

873: migrate session to use optional

PR Type

Enhancement

Description

Migrate Session.id to Optional<String>.
Update SessionRepository to return Optional<Session>.
Adapt session handling in controllers and services.
Enhance null-safety across session-related logic.

Diagram Walkthrough

flowchart LR
  A[Old Session Model] --> B{Refactor ID to Optional};
  B --> C[New Session Model];
  C --> D{Update Session Repository};
  D --> E{Update Auth Controllers & Services};
  E --> F[Update Tests];

File Walkthrough

Relevant files

Enhancement

3 files

AuthController.java `Update logout to handle optional session ID`	+4/-3
CustomAuthenticationSuccessHandler.java `Adjust session ID handling for optional type`	+2/-2
Protector.java Adapt session validation for `Optional` return type	+5/-2

Data model refactoring

1 files

Session.java Change session ID to `Optional` with default empty	+3/-2

Interface refactoring

1 files

SessionRepository.java Update `getSessionById` to return `Optional`	+2/-1

Implementation refactoring

1 files

SessionSqlRepository.java `Implement optional handling for session ID and retrieval`	+13/-12

Data transfer object update

1 files

SessionDto.java Update `fromSession` to extract session ID using `orElseThrow`	+1/-1

Tests

4 files

AuthControllerTest.java `Update authentication controller tests for optional session ID`	+6/-5
CustomAuthenticationSuccessHandlerTest.java `Adjust authentication success handler tests for optional session ID`	+9/-9
SessionRepositoryTest.java `Update session repository tests to handle optional types`	+7/-3
TestProtector.java `Update test protector configuration for optional session retrieval`	+3/-1

github-actions · 2026-03-24T20:28:26Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ Migrate-Session-SessionRepository-to-use-Optional-3267c85563aa802397aef5741f96ce6f - Fully compliant Compliant requirements: Migrate `SessionRepository` to use `Optional` for methods that might return null. Update the `Session` model to use `Optional` for its `id` field. Adjust all consuming code (controllers, services, DTOs, tests) to correctly handle `Optional` types.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ No major issues detected

github-actions · 2026-03-25T03:57:59Z

Title

873: migrate session to use optional

PR Type

Enhancement, Tests

Description

Migrate Session model's id field to Optional<String>.
Update SessionRepository methods to return Optional<Session>.
Adjust authentication, DTOs, and security to handle Optional Session IDs.
Refactor unit and integration tests for Session type changes.

File Walkthrough

Relevant files

Enhancement

7 files

AuthController.java Update logout to handle `Optional` `Session` ID	+4/-3
CustomAuthenticationSuccessHandler.java Adjust session creation and validation for `Optional` `Session` ID	+2/-2
Session.java Migrate `id` field to `Optional` with default empty value	+3/-2
SessionRepository.java Update `getSessionById` to return `Optional`	+2/-1
SessionSqlRepository.java Implement `Optional` handling for `Session` ID in SQL operations	+13/-12
SessionDto.java Update `fromSession` method to extract `Session` ID from `Optional`	+1/-1
Protector.java Modify `validateSession` to handle `Optional` from repository	+5/-2

Tests

4 files

AuthControllerTest.java Update session creation and assertions to use `Optional` `Session` ID	+6/-5
CustomAuthenticationSuccessHandlerTest.java Adjust mock session ID setting to use `Optional`	+9/-9
SessionRepositoryTest.java Update session tests to handle `Optional` `Session` objects and IDs	+19/-8
TestProtector.java Adjust mock session retrieval to handle `Optional` return type	+3/-1

Dependencies

1 files

BaseRepositoryTest.java `Add Mockito beans for scheduled tasks in base test class`	+8/-0

github-actions · 2026-03-25T03:58:21Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 Migrate-Session-SessionRepository-to-use-Optional-3267c85563aa802397aef5741f96ce6f - Partially compliant Compliant requirements: Migrate the `Session` model to use `Optional` for fields that can be null. Update `SessionRepository` interface methods to return `Optional<Session>` where a session might not be found. Implement the changes in `SessionSqlRepository` to handle `Optional` correctly during database operations (create, read). Adjust all consuming code (e.g., `AuthController`, `CustomAuthenticationSuccessHandler`, `Protector`, DTOs) to correctly interact with `Optional<Session>` and `Optional<String>`. Update relevant tests to reflect the `Optional` changes. Non-compliant requirements: Requires further human verification:
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Unused Method The private method `updateSessionWithResultSet` is defined but not called anywhere in the provided code diff. While not directly introduced by the `Optional` migration, it's good practice to remove unused code. private void updateSessionWithResultSet(final ResultSet resultSet, final Session session) throws SQLException { session.setId(Optional.of(resultSet.getString("id"))); }

github-actions · 2026-03-25T03:59:02Z

PR Code Suggestions ✨

No code suggestions found for the PR.

github-actions · 2026-03-25T18:04:40Z

Failed to generate code suggestions for PR

github-actions · 2026-03-25T18:18:17Z

Failed to generate code suggestions for PR

github-actions · 2026-03-25T18:20:30Z

Failed to generate code suggestions for PR

github-actions · 2026-03-29T23:17:16Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Documentation Clarity The Javadoc for the `createSession` method in `SessionRepository` describes required and optional fields for the `Session` object. Given that the `id` field is now an `Optional<String>` and is generated by the repository during creation, it would be beneficial to explicitly mention that the `id` will be populated by the repository after the method call. This clarifies the behavior for consumers of the interface. * @note - The provided object's methods will be overridden with any returned data from the database. * @param session - required fields: * <ul> * <li>userId * <li>expiresAt * </ul> / void createSession(Session session); Optional Handling* In the `createSession` method, the `session.getId().orElse(null)` call for setting the prepared statement parameter is functionally correct. However, since the `id` is explicitly generated and set on the `session` object just before this line, `session.getId().get()` or `session.getId().orElseThrow()` would more accurately reflect the expectation that the ID is always present at this point. While `orElse(null)` doesn't cause an issue here, being more explicit about the expected presence of the ID could improve code clarity. stmt.setString(1, session.getId().orElse(null));

github-actions · 2026-03-29T23:20:47Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Unrelated Feature The introduction of feature flag management (`FF.java` and `FeatureFlagManager.java`) and additional `@MockitoBean` declarations in `BaseRepositoryTest.java` for scheduled tasks (`SubmissionScheduler`, `LeetcodeAuthStealer`) appear to be unrelated to the PR's stated goal of migrating session handling to use `Optional`. These changes introduce new functionality and dependencies that should ideally be part of a separate PR to maintain focus and simplify review. package org.patinanetwork.codebloom.common.security.annotation.featureflag; import java.util.Map; import org.springframework.stereotype.Component; @Component public class FeatureFlagManager { private final Map<String, Boolean> flags = Map.of( "duel", false, "school", false); public boolean exists(String flag) { return flags.containsKey(flag); } public boolean isEnabled(String flag) { return flags.getOrDefault(flag, false); } } Missing Tests The newly introduced `FeatureFlagManager` class, which is a critical component for managing application features, lacks dedicated test coverage. It's important to ensure that the feature flag logic, including `exists` and `isEnabled` methods, is thoroughly tested to prevent unexpected behavior or misconfigurations. public class FeatureFlagManager { private final Map<String, Boolean> flags = Map.of( "duel", false, "school", false); public boolean exists(String flag) { return flags.containsKey(flag); } public boolean isEnabled(String flag) { return flags.getOrDefault(flag, false); } DB Schema Mismatch The `@NotNullColumn` annotation has been removed from the `id` field in the `Session` model, allowing it to be `Optional.empty()`. While the `createSession` method in `SessionSqlRepository` ensures an ID is generated before insertion, it's crucial to verify if the underlying database schema for the `id` column still enforces a `NOT NULL` constraint. If the database column is `NOT NULL`, the model's representation might be semantically misleading, even if the current implementation prevents `null` insertion. @Builder.Default private Optional<String> id = Optional.empty();

alfardil · 2026-03-30T00:06:31Z

/deploy

github-actions · 2026-03-30T00:07:05Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Scope Creep The files `FF.java` and `FeatureFlagManager.java` introduce a new feature flag mechanism. This functionality is not mentioned in the PR description or the associated Notion task, which focuses solely on migrating session handling to use `Optional`. While feature flags are useful, their inclusion here might represent scope creep. It should be verified if this is an intended addition for this PR or if it should be moved to a separate PR. package org.patinanetwork.codebloom.common.security.annotation.featureflag; import java.lang.annotation.Documented; import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; /** / @Target(ElementType.METHOD) @Retention(RetentionPolicy.RUNTIME) @Documented public @interface FF { String value(); } Database Schema* The `@NotNullColumn` annotation was removed from the `id` field in the `Session` model. While `Optional.empty()` is not null, the underlying database column for `id` (presumably a primary key) should still be `NOT NULL`. Ensure that the database schema correctly enforces the `NOT NULL` constraint for the `id` column, and that the repository logic (e.g., `createSession`) guarantees an ID is always generated and present before insertion. @Builder.Default private Optional<String> id = Optional.empty();

github-actions · 2026-03-30T00:19:37Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Out of Scope The PR description states the task is to migrate the Session and SessionRepository to use Optional. However, this PR introduces new files for feature flag functionality (`FF.java` and `FeatureFlagManager.java`). These changes appear to be unrelated to the stated task. Please confirm if these files are intentionally part of this PR or if they should be moved to a separate PR. package org.patinanetwork.codebloom.common.security.annotation.featureflag; import java.lang.annotation.Documented; import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; /** / @Target(ElementType.METHOD) @Retention(RetentionPolicy.RUNTIME) @Documented public @interface FF { String value(); } Lombok Annotation* The `Session` model uses `@Getter`, `@Setter`, `@Builder`, `@EqualsAndHashCode`, and `@ToString`. While not strictly required for this specific model based on its current usage, the database repository best practices mention `@Jacksonized` is best to always add so a class can be de-serialized if it's ever converted from a JSON string into a Java object. Consider adding `@Jacksonized` for completeness and future-proofing, especially if `Session` objects might be serialized/deserialized from JSON. public class Session { @Builder.Default private Optional<String> id = Optional.empty();

alfardil · 2026-03-30T00:30:50Z

/deploy

alfardil · 2026-03-30T16:45:09Z

/deploy

github-actions · 2026-03-30T16:46:37Z

Failed to generate code suggestions for PR

approved

github-actions Bot reviewed Mar 24, 2026

View reviewed changes

Comment thread ...n/java/org/patinanetwork/codebloom/api/auth/security/CustomAuthenticationSuccessHandler.java Outdated

Comment thread src/main/java/org/patinanetwork/codebloom/common/security/Protector.java Outdated

alfardil force-pushed the 873 branch from ed6a6d1 to 772b594 Compare March 24, 2026 20:26

github-actions Bot reviewed Mar 24, 2026

View reviewed changes

alfardil force-pushed the 873 branch from 31956d9 to 18c9643 Compare March 25, 2026 18:02

alfardil force-pushed the 873 branch from 6b3272f to 5ff1c13 Compare March 25, 2026 18:18

alfardil changed the title ~~873: migrate session to use optional~~ 873: Migrate session to use optional Mar 25, 2026

angelayu0530 reviewed Mar 27, 2026

View reviewed changes

Comment thread src/main/java/org/patinanetwork/codebloom/api/auth/AuthController.java Outdated

alfardil force-pushed the 873 branch from 5ff1c13 to 0e8c350 Compare March 29, 2026 23:15

github-actions Bot reviewed Mar 29, 2026

View reviewed changes

Comment thread src/main/java/org/patinanetwork/codebloom/common/db/models/Session.java

github-actions Bot reviewed Mar 29, 2026

View reviewed changes

tahminator previously requested changes Mar 29, 2026

View reviewed changes

Comment thread ...n/java/org/patinanetwork/codebloom/api/auth/security/CustomAuthenticationSuccessHandler.java Outdated

alfardil force-pushed the 873 branch from 4a324a3 to 7950538 Compare March 30, 2026 00:05

github-actions Bot reviewed Mar 30, 2026

View reviewed changes

Comment thread src/main/java/org/patinanetwork/codebloom/common/dto/session/SessionDto.java

Comment thread ...a/org/patinanetwork/codebloom/common/security/annotation/featureflag/FeatureFlagManager.java Outdated

alfardil force-pushed the 873 branch from 7950538 to 68578bd Compare March 30, 2026 00:17

github-actions Bot reviewed Mar 30, 2026

View reviewed changes

Comment thread ...a/org/patinanetwork/codebloom/common/security/annotation/featureflag/FeatureFlagManager.java Outdated

Comment thread src/main/java/org/patinanetwork/codebloom/common/dto/session/SessionDto.java

alfardil added 4 commits March 30, 2026 12:44

873: Migrate Session repository to use Optional

214bac1

873: Fixed exception

aaa2623

873: added throw for missing id

52ae215

873: removed errant files

82b117e

alfardil force-pushed the 873 branch from d261991 to 82b117e Compare March 30, 2026 16:44

angelayu0530 approved these changes Mar 30, 2026

View reviewed changes

alfardil enabled auto-merge (rebase) March 30, 2026 16:49

alfardil merged commit 7aec33c into main Mar 30, 2026
45 of 46 checks passed

alfardil deleted the 873 branch March 30, 2026 17:19

github-actions Bot mentioned this pull request Apr 17, 2026

872: Migrate BackgroundTask to use Optional #908

Merged

6 tasks

Uh oh!

Conversation

alfardil commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

873

Description of changes

Checklist before review

Screenshots

Dev

Staging

Uh oh!

github-actions Bot commented Mar 24, 2026

Available PR Commands

Uh oh!

github-actions Bot commented Mar 24, 2026

Title

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

github-actions Bot commented Mar 24, 2026

PR Reviewer Guide 🔍

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Mar 24, 2026

Title

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

github-actions Bot commented Mar 24, 2026

PR Reviewer Guide 🔍

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Mar 25, 2026

Title

PR Type

Description

File Walkthrough

Uh oh!

github-actions Bot commented Mar 25, 2026

PR Reviewer Guide 🔍

Uh oh!

github-actions Bot commented Mar 25, 2026

PR Code Suggestions ✨

Uh oh!

github-actions Bot commented Mar 25, 2026

Uh oh!

github-actions Bot commented Mar 25, 2026

Uh oh!

github-actions Bot commented Mar 25, 2026

Uh oh!

Uh oh!

github-actions Bot commented Mar 29, 2026

PR Reviewer Guide 🔍

Uh oh!

Uh oh!

github-actions Bot commented Mar 29, 2026

PR Reviewer Guide 🔍

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alfardil commented Mar 30, 2026

Uh oh!

github-actions Bot commented Mar 30, 2026

PR Reviewer Guide 🔍

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Mar 30, 2026

PR Reviewer Guide 🔍

Uh oh!

Uh oh!

alfardil commented Mar 24, 2026 •

edited

Loading