[Refactor/signup token version] - Signup Token Version사용#108
Conversation
WalkthroughThis update introduces Flyway database migration support, adds optimistic locking and a usage flag to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AuthController
participant SignUpUsecaseImpl
participant SignupTokenService
participant SignupTokenRepository
participant SignupToken
User->>AuthController: POST /signup (with token)
AuthController->>SignUpUsecaseImpl: registerUser(token, ...)
SignUpUsecaseImpl->>SignupTokenService: extractPayload(token)
SignUpUsecaseImpl->>SignupTokenService: findUnExpiredToken(token)
SignupTokenService->>SignupTokenRepository: findByIdAndCreatedAtAfter(token, cutoff)
SignupTokenRepository-->>SignupTokenService: SignupToken (unused)
SignupTokenService-->>SignUpUsecaseImpl: SignupToken
SignUpUsecaseImpl->>...: (proceed with user registration)
SignUpUsecaseImpl->>SignupTokenService: invalidateSignupToken(SignupToken)
SignupTokenService->>SignupToken: markAsUsed()
SignupTokenService->>SignupTokenRepository: save(SignupToken)
SignupTokenService-->>SignUpUsecaseImpl: (confirmation)
AuthController-->>User: 200 OK or error response
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Graph Analysis (1)src/test/java/org/runimo/runimo/auth/domain/SignupTokenTest.java (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (8)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
build.gradle (2)
7-8: Check whether the Flyway Gradle plugin already brings inflyway-core.The plugin
org.flywaydb.flywayusually declaresflyway-coreon its class-path.
Keeping an explicitimplementation 'org.flywaydb:flyway-core'(line 49) can lead to a duplicate dependency with potentially conflicting versions if the plugin is bumped independently from the library coordinates.If the build still works after removing the explicit dependency, prefer relying on the one that the plugin supplies and keep only the database-specific artefact:
- implementation 'org.flywaydb:flyway-core' implementation 'org.flywaydb:flyway-mysql'
49-51: UseruntimeOnlyfor JDBC-specific Flyway extensions.
flyway-mysqlis needed only at runtime when migrations run against MySQL, not at compile time.
Mark itruntimeOnlyto slim the compile class-path:- implementation 'org.flywaydb:flyway-mysql' + runtimeOnly 'org.flywaydb:flyway-mysql'src/main/resources/db/migration/V20250702__add_version_column_signup_token.sql (1)
7-10:UPDATEis redundant after adding default values.Since both new columns are
NOT NULLwith defaults (0/FALSE), existing rows are auto-populated duringALTER TABLE.
The subsequentUPDATEdoes a full-table write for no gain and may lock the table unnecessarily.Safe to drop:
--- 기존 토큰들은 사용되지 않은 것으로 처리 -UPDATE `signup_token` -SET `used` = FALSE, - `version` = 0;src/test/resources/application.yml (2)
29-34:clean-disabled: falseon tests might drop objects needed by future parallel tests.Allowing
flywayCleanin the test profile is convenient but risky when multiple test classes use the same in-memory DB concurrently (e.g., with spring-test’s@DirtiesContext).
If not strictly required, set it totrueand invokeFlyway.clean()explicitly inside tests that need a fresh schema.
40-48:defer-datasource-initialization: falsecombined withsql.init.mode: neveris redundant.With
sql.init.mode: neverSpring will not runschema.sql/data.sqlanyway; thedefer-datasource-initializationflag only affects those initializers.
You can remove the property to keep the config minimal.src/main/resources/db/migration/V1__Create_baseline.sql (1)
26-28: Single-rowSELECTstatements in baseline scripts are harmless but noisy.Flyway records migrations based on checksum; returning result sets is unnecessary and can surprise new contributors.
Consider deleting the finalSELECT—the baseline marker comment alone suffices.src/test/java/org/runimo/runimo/auth/controller/AuthAcceptanceTest.java (1)
172-180: Consider cleaning up redundant token creation.The token creation logic here is redundant since a valid token already exists in the setUp method. The test could reuse the existing token or clarify why a new token instance is needed.
- SignupToken signupToken = new SignupToken( - "valid-token", - "provider-id", - "refresh-token", - SocialProvider.KAKAO - ); - token = jwtTokenFactory.generateSignupTemporalToken("provider-id", SocialProvider.KAKAO, - "valid-token"); request = new AuthSignupRequest(token, "username", Gender.UNKNOWN);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
build.gradle(2 hunks)src/main/java/org/runimo/runimo/auth/domain/SignupToken.java(3 hunks)src/main/java/org/runimo/runimo/auth/repository/SignupTokenRepository.java(1 hunks)src/main/java/org/runimo/runimo/auth/service/SignUpUsecaseImpl.java(1 hunks)src/main/resources/db/migration/V1__Create_baseline.sql(1 hunks)src/main/resources/db/migration/V20250702__add_version_column_signup_token.sql(1 hunks)src/test/java/org/runimo/runimo/auth/controller/AuthAcceptanceTest.java(2 hunks)src/test/resources/application.yml(1 hunks)src/test/resources/db/migration/h2/V1__Create_baseline.sql(1 hunks)src/test/resources/db/migration/h2/V20250702__add_version_column_signup_token.sql(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (9)
src/main/resources/db/migration/V20250702__add_version_column_signup_token.sql (1)
1-6: Column nameversioncan collide with JPA’s implicit column mapping.
@Versionfields default to the column nameversion.
Because it is also a common SQL keyword across vendors (and is reserved in some), always quoting it may become necessary when Flyway executes on different RDBMSes.Consider renaming or adding an explicit
@Column(name = "token_version")in the entity and reflecting that here to avoid any cross-dialect surprises.src/test/resources/db/migration/h2/V20250702__add_version_column_signup_token.sql (1)
1-6: Keep H2 and MySQL scripts identical only when behaviour truly matches.H2 in
MODE=MYSQLaccepts back-ticks, but subtle type differences (e.g.,BOOLEANvsBIT) can surface later. Validate that the generated schema for tests matches production (SHOW COLUMNS signup_token) to avoid green-test / red-prod scenarios.src/main/java/org/runimo/runimo/auth/repository/SignupTokenRepository.java (1)
12-14: LGTM! Query correctly filters unused tokens.The addition of
AND st.used = falseproperly aligns with the new token usage semantics where tokens are marked as used instead of deleted.src/main/java/org/runimo/runimo/auth/service/SignUpUsecaseImpl.java (1)
60-60: LGTM! Proper token lifecycle management.Replacing token deletion with
markAsUsed()correctly implements the new token usage semantics and enables optimistic locking to prevent concurrent token usage.src/main/java/org/runimo/runimo/auth/domain/SignupToken.java (1)
38-43: LGTM! Proper optimistic locking and usage tracking implementation.The
@Versionannotation for optimistic locking and theusedboolean field with defaultfalseare correctly implemented for tracking token usage state.src/test/java/org/runimo/runimo/auth/controller/AuthAcceptanceTest.java (2)
156-191: LGTM! Test correctly validates token reuse scenario.The test properly verifies that attempting to reuse the same signup token results in a 401 Unauthorized response, which aligns with the new token usage semantics.
193-230: LGTM! Test correctly validates duplicate user with different token.The test properly verifies that attempting to create a duplicate user with a different token results in a 409 Conflict response, maintaining the existing duplicate user validation logic.
src/test/resources/db/migration/h2/V1__Create_baseline.sql (2)
88-95: LGTM! Baseline migration correctly represents original schema.The
signup_tokentable definition in this baseline migration correctly shows the original structure without theversionandusedcolumns, which will be added in the subsequent migration. This properly establishes the schema baseline.
265-266: LGTM! Proper baseline migration documentation.The message clearly indicates this is a baseline migration marking the existing schema as V1, which allows subsequent versioned migrations to start from V2.
| public void markAsUsed() { | ||
| if (this.used) { | ||
| throw new SignUpException(UserHttpResponseCode.SIGNIN_FAIL_ALREADY_EXIST); | ||
| } | ||
| this.used = true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the error code for token reuse scenario.
The markAsUsed() method throws SIGNIN_FAIL_ALREADY_EXIST which seems inappropriate for a token reuse scenario. Consider using a more specific error code like TOKEN_ALREADY_USED or TOKEN_INVALID.
public void markAsUsed() {
if (this.used) {
- throw new SignUpException(UserHttpResponseCode.SIGNIN_FAIL_ALREADY_EXIST);
+ throw new SignUpException(UserHttpResponseCode.TOKEN_INVALID);
}
this.used = true;
}📝 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.
| public void markAsUsed() { | |
| if (this.used) { | |
| throw new SignUpException(UserHttpResponseCode.SIGNIN_FAIL_ALREADY_EXIST); | |
| } | |
| this.used = true; | |
| } | |
| public void markAsUsed() { | |
| if (this.used) { | |
| throw new SignUpException(UserHttpResponseCode.TOKEN_INVALID); | |
| } | |
| this.used = true; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/runimo/runimo/auth/domain/SignupToken.java lines 54 to 59,
the markAsUsed() method throws an incorrect error code SIGNIN_FAIL_ALREADY_EXIST
when the token is already used. Replace this error code with a more appropriate
one such as TOKEN_ALREADY_USED or TOKEN_INVALID to accurately reflect the token
reuse scenario.
|
1 participant
작업내역
@Version컬럼 추가, 상태 컬럼 추가Summary by CodeRabbit
New Features
Database Migrations
Bug Fixes
Tests
Chores