Skip to content

Conversation

@lu-yg
Copy link
Collaborator

@lu-yg lu-yg commented Dec 30, 2025

English | 简体中文

PR

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our Commit Message Guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Built its own designer, fully self-validated

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

Background and solution

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Summary by CodeRabbit

  • Bug Fixes

    • Improved error handling and messaging when accessing non-existent applications
    • Enhanced user session persistence with proper preservation of usage status
  • Chores

    • Made tenant English name field optional during database setup

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 30, 2025

Walkthrough

Database schema updated to make tenant English name nullable, JWT utility enhanced to serialize and deserialize the isInUse property for round-trip token support, and null validation added to app service method with error handling for missing apps.

Changes

Cohort / File(s) Summary
Database Schema
app/src/main/resources/sql/mysql/01_create_all_tables_ddl_v1.0.0.mysql.sql
Column t_tenant.name_en changed from varchar(255) not null to nullable varchar(255), allowing NULL values for tenant English names.
JWT Token Serialization
base/src/main/java/com/tinyengine/it/login/utils/JwtUtil.java
Added bidirectional serialization support for isInUse field: now included when converting Tenant to map for token storage, and restored on deserialization from token payload.
App Service Validation
base/src/main/java/com/tinyengine/it/service/app/impl/v1/AppV1ServiceImpl.java
Added null check in getMetaDto(Integer id) that throws ServiceException with error code CM009 when app lookup returns null; introduced import for ExceptionEnum.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A schema bends to let names breathe free,
While tokens now recall what used to be—
Null checks stand guard at the service gate,
Each change precise, each detail ornate! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'fix: modify login set tenant' is vague and does not clearly convey the specific changes made in the changeset, which include database schema modifications, JWT serialization updates, and null-checking logic. Revise the title to be more specific about the primary change, such as 'fix: add tenant isInUse field to JWT serialization and null check in app retrieval' or focus on the most impactful change with concrete details.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e5f9c20 and 8304325.

📒 Files selected for processing (1)
  • app/src/main/resources/sql/mysql/01_create_all_tables_ddl_v1.0.0.mysql.sql
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/src/main/resources/sql/mysql/01_create_all_tables_ddl_v1.0.0.mysql.sql

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
base/src/main/java/com/tinyengine/it/service/platform/impl/TenantServiceImpl.java (1)

116-121: Potential bug: Uniqueness check on nullable column may allow duplicates.

The uniqueness check now uses name_cn, but according to the DDL changes, name_cn is now nullable. This creates two issues:

  1. If tenant.getNameCn() is null, this query won't find existing tenants with null name_cn (since NULL != NULL in SQL).
  2. The unique index on name_cn will allow multiple rows with NULL values in MySQL.

This could allow duplicate tenants to be created when name_cn is not provided.

🔎 Suggested fix: Add null check or use name_en for uniqueness
 public Integer createTenant(Tenant tenant) {
     QueryWrapper<Tenant> queryWrapper = new QueryWrapper<>();
-    queryWrapper.eq("name_cn", tenant.getNameCn());
+    // Use name_en for uniqueness check since it's NOT NULL
+    queryWrapper.eq("name_en", tenant.getNameEn());
     Tenant tenantResult = this.baseMapper.selectOne(queryWrapper);

Or if name_cn is intentionally the uniqueness constraint, ensure it's not nullable:

+    if (tenant.getNameCn() == null || tenant.getNameCn().trim().isEmpty()) {
+        throw new ServiceException(ExceptionEnum.CM001.getResultCode(), "name_cn is required");
+    }
     QueryWrapper<Tenant> queryWrapper = new QueryWrapper<>();
     queryWrapper.eq("name_cn", tenant.getNameCn());
app/src/main/resources/sql/mysql/01_create_all_tables_ddl_v1.0.0.mysql.sql (1)

583-591: Schema design concern: Unique index on nullable column.

With name_cn being nullable and having a unique index, MySQL will allow multiple rows with NULL values for name_cn. This may not align with the intended uniqueness constraint in TenantServiceImpl.createTenant().

Consider one of these approaches:

  1. Keep name_cn as NOT NULL if it's the primary identifier
  2. Change the unique index to name_en which is NOT NULL
  3. Use a composite unique index that includes a non-nullable column
🧹 Nitpick comments (2)
base/src/main/java/com/tinyengine/it/login/utils/JwtUtil.java (1)

186-186: Consider defensive handling for tokens generated before this change.

If a token was generated before this change (without isInUse), map.get("isInUse") returns null, which is safely cast to Boolean. However, you may want to explicitly document this backward compatibility or set a sensible default for older tokens.

🔎 Optional: Add explicit default handling
-            tenant.setIsInUse((Boolean) map.get("isInUse"));
+            Object isInUseValue = map.get("isInUse");
+            tenant.setIsInUse(isInUseValue != null ? (Boolean) isInUseValue : false);
base/src/test/java/com/tinyengine/it/service/app/impl/UserServiceImplTest.java (1)

18-18: Unused imports detected.

MockUserContext (line 18) and ArrayList (line 30) are imported but not used in this test file. The test uses Arrays.asList() instead of ArrayList, and MockUserContext is not referenced anywhere.

🔎 Remove unused imports
 import cn.hutool.core.util.ReflectUtil;
-import com.tinyengine.it.common.handler.MockUserContext;
 import com.tinyengine.it.login.service.impl.LoginServiceImpl;
 import com.tinyengine.it.mapper.UserMapper;
 import com.tinyengine.it.model.entity.User;

-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;

Also applies to: 30-30

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0f2ef7b and e5f9c20.

📒 Files selected for processing (6)
  • app/src/main/resources/sql/mysql/01_create_all_tables_ddl_v1.0.0.mysql.sql
  • base/src/main/java/com/tinyengine/it/login/utils/JwtUtil.java
  • base/src/main/java/com/tinyengine/it/service/app/impl/v1/AppV1ServiceImpl.java
  • base/src/main/java/com/tinyengine/it/service/platform/impl/TenantServiceImpl.java
  • base/src/test/java/com/tinyengine/it/service/app/impl/AppServiceImplTest.java
  • base/src/test/java/com/tinyengine/it/service/app/impl/UserServiceImplTest.java
🧰 Additional context used
🧬 Code graph analysis (2)
base/src/test/java/com/tinyengine/it/service/app/impl/UserServiceImplTest.java (1)
base/src/test/java/com/tinyengine/it/common/handler/MockUserContext.java (1)
  • MockUserContext (25-71)
base/src/test/java/com/tinyengine/it/service/app/impl/AppServiceImplTest.java (1)
base/src/test/java/com/tinyengine/it/common/handler/MockUserContext.java (1)
  • MockUserContext (25-71)
🔇 Additional comments (2)
base/src/main/java/com/tinyengine/it/login/utils/JwtUtil.java (1)

119-119: LGTM! isInUse serialization added consistently.

The addition of isInUse to the JWT token payload is symmetric with the deserialization at line 186. This ensures tenant state is preserved during token round-trips.

base/src/main/java/com/tinyengine/it/service/app/impl/v1/AppV1ServiceImpl.java (1)

284-286: Good addition of null-check error handling.

This defensive check prevents a NullPointerException that would occur downstream (e.g., at line 287 when accessing app.getPlatformId()). Using ExceptionEnum.CM009 provides consistent error messaging.

Copy link
Contributor

@msslulu msslulu left a comment

Choose a reason for hiding this comment

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

ok

@lu-yg lu-yg merged commit 4191663 into opentiny:develop Dec 30, 2025
1 check passed
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.

2 participants