fix: modify tenant API #280
Conversation
WalkthroughMapper now extends MyBatis‑Plus BaseMapper; Tenant adds non‑persistent isInUse and fixes Schema name; new CM339 enum constant added; SSOInterceptor now throws ServiceException instead of performing redirects; tenant selection and setTenant updated; createTenant adds duplicate check; tests and Docker example updated. Changes
Attention areas: new imports ( Sequence Diagram(s)sequenceDiagram
participant Client
participant SSOInterceptor
participant TokenService
participant UserContext
rect rgba(220,235,255,0.6)
Note left of Client: HTTP request with Authorization header
end
Client->>SSOInterceptor: Incoming HTTP request (Authorization)
SSOInterceptor->>SSOInterceptor: extract token\n(if missing -> throw ServiceException CM336)
SSOInterceptor->>TokenService: validate token / parse claims
alt token invalid
TokenService-->>SSOInterceptor: validation fails
SSOInterceptor-->>Client: throw ServiceException (CM339)
else token valid
TokenService-->>SSOInterceptor: user claims
SSOInterceptor->>UserContext: populate login user and tenant list
UserContext-->>SSOInterceptor: tenant list (some items may have isInUse=true)
SSOInterceptor->>Client: proceed to controller (request continues)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
base/src/main/java/com/tinyengine/it/mapper/AuthUsersUnitsRolesMapper.java (1)
11-11: Clean up unused custom CRUD methods to reduce API confusion.The mapper extends
BaseMapper<AuthUsersUnitsRoles>providing inherited CRUD operations (insert,deleteById,updateById,selectById), while also defining custom CRUD methods:
createAuthUsersUnitsRoles(actively used; implements custom ON DUPLICATE KEY upsert logic)deleteAuthUsersUnitsRolesById(unused)updateAuthUsersUnitsRolesById(unused)queryAuthUsersUnitsRolesById(unused)The unused custom methods add no value and create confusion about which API to use. Consider removing
deleteAuthUsersUnitsRolesById,updateAuthUsersUnitsRolesById, andqueryAuthUsersUnitsRolesByIdsince they're never called. KeepcreateAuthUsersUnitsRolesas it provides custom upsert logic.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
base/src/main/java/com/tinyengine/it/mapper/AuthUsersUnitsRolesMapper.java(1 hunks)
🔇 Additional comments (1)
base/src/main/java/com/tinyengine/it/mapper/AuthUsersUnitsRolesMapper.java (1)
3-3: LGTM!The import is necessary for extending BaseMapper and integrates MyBatis-Plus CRUD functionality.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
base/src/main/java/com/tinyengine/it/common/exception/ExceptionEnum.javabase/src/main/java/com/tinyengine/it/login/config/SSOInterceptor.javabase/src/main/java/com/tinyengine/it/login/config/context/DefaultLoginUserContext.javabase/src/main/java/com/tinyengine/it/login/controller/LoginController.javabase/src/main/java/com/tinyengine/it/model/entity/Tenant.javabase/src/main/java/com/tinyengine/it/service/platform/impl/TenantServiceImpl.javaexample-docker.yml
💤 Files with no reviewable changes (1)
- example-docker.yml
🔇 Additional comments (6)
base/src/main/java/com/tinyengine/it/model/entity/Tenant.java (1)
45-45: LGTM: Schema name correction.The Schema name has been correctly updated from "nameCn" to "orgCode" to match the actual field name, improving API documentation consistency.
base/src/main/java/com/tinyengine/it/service/platform/impl/TenantServiceImpl.java (1)
116-121: LGTM: Duplicate tenant prevention.The pre-insert duplicate check correctly validates that no tenant with the same
name_cnexists before creation, preventing data integrity issues. The use ofServiceExceptionwithCM003provides clear error messaging to the caller.base/src/main/java/com/tinyengine/it/login/controller/LoginController.java (1)
242-244: LGTM: Null check for tenantId.The addition of a null check for
tenantIdwith appropriate error response improves input validation and prevents potential downstream issues.base/src/main/java/com/tinyengine/it/login/config/SSOInterceptor.java (2)
46-49: LGTM: Exception-based authentication flow.The shift from HTTP redirects to throwing
ServiceExceptionprovides better separation of concerns and allows centralized error handling. The use ofCM336for missing tokens is appropriate.
57-60: LGTM: Consistent error handling for token validation.Token validation failures now throw
ServiceExceptionwithCM339, maintaining consistency with the exception-based flow. Note: There is a bug inExceptionEnum.CM339definition (see review of ExceptionEnum.java) where the error code is incorrectly set to "CM338" instead of "CM339".base/src/main/java/com/tinyengine/it/common/exception/ExceptionEnum.java (1)
319-319: LGTM: Enum continuation.Changing the terminator from semicolon to comma correctly prepares the enum for additional constants.
base/src/main/java/com/tinyengine/it/common/exception/ExceptionEnum.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/tinyengine/it/login/config/context/DefaultLoginUserContext.java
Show resolved
Hide resolved
base/src/main/java/com/tinyengine/it/login/controller/LoginController.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
base/src/main/java/com/tinyengine/it/login/controller/LoginController.java (1)
248-259: The isInUse logic is now correct.The if-else structure properly ensures only the selected tenant has
isInUse = truewhile all others are set tofalse. This addresses the previous bug.Consider defensive copying to avoid mutating shared state.
The code mutates
Tenantobjects retrieved fromloginUserContext.getTenants(). If the underlying list is cached or shared, these mutations (settingisInUse) could have unintended side effects elsewhere. Consider creating copies of the tenant objects before modifying them.🔎 Suggested defensive copy approach
List<Tenant> tenantList = new ArrayList<>(); boolean found = false; for (Tenant tenant : tenants) { + Tenant copy = new Tenant(tenant); // or use a copy constructor/clone method if (tenant.getId().equals(tenantId.toString())) { - tenant.setIsInUse(true); + copy.setIsInUse(true); found = true; } else { - tenant.setIsInUse(false); + copy.setIsInUse(false); } - tenantList.add(tenant); + tenantList.add(copy); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
base/src/main/java/com/tinyengine/it/common/exception/ExceptionEnum.javabase/src/main/java/com/tinyengine/it/login/controller/LoginController.java
🔇 Additional comments (3)
base/src/main/java/com/tinyengine/it/common/exception/ExceptionEnum.java (1)
315-324: LGTM!The new
CM339exception enum is correctly defined with the proper error code"CM339"and an appropriate message for token validation failures. The previous issue with the duplicate error code has been addressed.base/src/main/java/com/tinyengine/it/login/controller/LoginController.java (2)
242-244: LGTM on the null check.Adding explicit null validation for
tenantIdprovides good defense in depth, even though@RequestParamwould typically require the parameter.
260-273: LGTM!The validation using the
foundflag correctly ensures the requested tenant exists in the user's tenant list before proceeding, and the token generation now uses the properly preparedtenantListwith correctisInUseflags.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
base/src/test/java/com/tinyengine/it/service/app/impl/PageServiceImplTest.java (1)
114-114: Consider usinganyString()matcher for tenant parameter.The hardcoded tenant ID
"1"in these mock statements couples the tests to a specific tenant value. UsinganyString()(as done inAppV1ServiceImplTest.javaat lines 122 and 158) would make these tests more flexible and consistent with the other test file.🔎 Proposed refactor for consistency
-when(appMapper.queryAppById(333, "1")).thenReturn(app); +when(appMapper.queryAppById(anyInt(), anyString())).thenReturn(app);Apply similar changes to lines 154, 189, 225, and 248.
Also applies to: 154-154, 189-189, 225-225, 248-248
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
base/src/test/java/com/tinyengine/it/service/app/impl/PageServiceImplTest.javabase/src/test/java/com/tinyengine/it/service/app/impl/v1/AppV1ServiceImplTest.java
🔇 Additional comments (1)
base/src/test/java/com/tinyengine/it/service/app/impl/v1/AppV1ServiceImplTest.java (1)
122-122: LGTM! Good use of flexible matchers.Using
anyString()for the tenant parameter is appropriate for unit tests, as it keeps the tests decoupled from specific tenant values and focuses on the logic being tested.Also applies to: 158-158
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
base/src/test/java/com/tinyengine/it/service/material/impl/BlockServiceImplTest.java (1)
293-293: LGTM with note on matcher choice.Using
anyString()for the tenant parameter is more flexible than hardcoding, but it doesn't validate that the correct tenant ID is passed from the context. This approach differs fromAppServiceImplTest.java(lines 77, 86, 104-105, 119), which hardcodes"1".For consistency across the test suite, consider standardizing on one approach—either
anyString()throughout or hardcoded values that matchMockUserContext.getTenantId().Also applies to: 310-310
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
base/src/test/java/com/tinyengine/it/common/handler/MyMetaObjectHandlerTest.javabase/src/test/java/com/tinyengine/it/service/app/impl/AppServiceImplTest.javabase/src/test/java/com/tinyengine/it/service/material/impl/BlockServiceImplTest.javabase/src/test/java/com/tinyengine/it/service/platform/impl/TenantServiceImplTest.java
💤 Files with no reviewable changes (1)
- base/src/test/java/com/tinyengine/it/service/platform/impl/TenantServiceImplTest.java
🔇 Additional comments (1)
base/src/test/java/com/tinyengine/it/common/handler/MyMetaObjectHandlerTest.java (1)
39-39: The test expectation oftimes(5)is correct and accurately reflects the production code behavior. TheinsertFillmethod callssetFieldValByNamefive times unconditionally (forcreatedTime,lastUpdatedTime,createdBy,lastUpdatedBy, andplatformId). The conditional block fortenantIddoes not execute becausegetTenants().isEmpty()returns false (MockUserContext returns a non-empty list).
base/src/test/java/com/tinyengine/it/service/app/impl/AppServiceImplTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
base/src/test/java/com/tinyengine/it/service/app/impl/UserServiceImplTest.java (2)
19-19: Remove unused imports.The imports for
MockUserContext(line 19) andArrayList(line 31) are not used anywhere in the test class. These were likely left behind whentestCreateUserwas removed.🔎 Proposed fix
-import com.tinyengine.it.common.handler.MockUserContext;-import java.util.ArrayList;Also applies to: 31-31
48-48: Remove unused loginServiceImpl field and setup.The
loginServiceImplfield (line 48) and its baseMapper setup (line 54) are not used in any of the remaining test methods. This appears to be leftover code from the removedtestCreateUsertest.🔎 Proposed fix
Remove the unused field:
- @InjectMocks - private LoginServiceImpl loginServiceImpl; -Remove the unused setup:
- ReflectUtil.setFieldValue(loginServiceImpl, "baseMapper", userMapper);Also applies to: 54-54
base/src/test/java/com/tinyengine/it/service/app/impl/AppServiceImplTest.java (2)
24-24: Remove unused import.The
TestUtilimport is not used anywhere in this test file.🔎 Proposed fix
-import com.tinyengine.it.common.utils.TestUtil;
141-141: Consider extracting tenant ID to a constant.While the hardcoded
"1"is consistent withMockUserContext.getTenantId(), extracting it to a constant (e.g.,private static final String TEST_TENANT_ID = "1") would improve maintainability, especially since this value appears multiple times (lines 80, 89, 107, 108, 122, 141).🔎 Example refactor
Add a constant at class level:
+ private static final String TEST_TENANT_ID = "1"; + @Mock private AppMapper appMapper;Then use it throughout:
- param.setTenantId("1"); + param.setTenantId(TEST_TENANT_ID);base/src/test/java/com/tinyengine/it/service/app/impl/v1/AppV1ServiceImplTest.java (1)
126-126: Consider verifying the tenant ID parameter.The signature change from
queryAppById(anyInt())toqueryAppById(anyInt(), anyString())correctly reflects the tenant API modifications. However, usinganyString()doesn't verify that the correct tenant ID ("1" fromMockUserContext) is passed to the mapper.💡 Optional: Strengthen assertion by matching the expected tenant ID
-when(appMapper.queryAppById(anyInt(), anyString())).thenReturn(app); +when(appMapper.queryAppById(anyInt(), eq("1"))).thenReturn(app);Note: This change would require adding
import static org.mockito.Mockito.eq;at the top of the file.Also applies to: 164-164
base/src/test/java/com/tinyengine/it/controller/UserControllerTest.java (2)
62-64: Set tenant properties for consistency with MockUserContext.The
Tenantobject is created without any properties. For consistency withMockUserContext(which sets tenant id to "1") and to ensure the test better reflects real usage, consider initializing tenant properties such asid.🔎 Proposed enhancement
Tenant tenant = new Tenant(); +tenant.setId("1"); List<Tenant> tenants = new ArrayList<>(); tenants.add(tenant);
66-68: Consider verifying tenant-related behavior in the test assertion.The test currently only asserts that
result.getData()equalsmockData, but doesn't verify that the tenant list was properly processed or attached to the user. Consider adding assertions to verify the tenant-related changes introduced in this PR.For example, you could verify:
- That the tenant list was queried with the correct user ID
- That the returned user object contains the expected tenant information
- That
authUsersUnitsRolesMapper.queryAllTenantByUserIdwas invoked exactly once🔎 Example enhancement
Result<User> result = userController.me(); Assertions.assertEquals(mockData, result.getData()); +verify(authUsersUnitsRolesMapper, times(1)).queryAllTenantByUserId(anyInt());Note: This would require importing
verifyandtimesfrom Mockito.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
base/src/test/java/com/tinyengine/it/controller/UserControllerTest.javabase/src/test/java/com/tinyengine/it/service/app/impl/AppServiceImplTest.javabase/src/test/java/com/tinyengine/it/service/app/impl/PageServiceImplTest.javabase/src/test/java/com/tinyengine/it/service/app/impl/UserServiceImplTest.javabase/src/test/java/com/tinyengine/it/service/app/impl/v1/AppV1ServiceImplTest.javabase/src/test/java/com/tinyengine/it/service/material/impl/BlockServiceImplTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- base/src/test/java/com/tinyengine/it/service/material/impl/BlockServiceImplTest.java
🧰 Additional context used
🧬 Code graph analysis (5)
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)
base/src/test/java/com/tinyengine/it/service/app/impl/PageServiceImplTest.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/v1/AppV1ServiceImplTest.java (1)
base/src/test/java/com/tinyengine/it/common/handler/MockUserContext.java (1)
MockUserContext(25-71)
base/src/test/java/com/tinyengine/it/controller/UserControllerTest.java (2)
base/src/test/java/com/tinyengine/it/common/handler/MockUserContext.java (1)
MockUserContext(25-71)base/src/test/java/com/tinyengine/it/common/utils/TestUtil.java (1)
TestUtil(22-30)
🔇 Additional comments (10)
base/src/test/java/com/tinyengine/it/service/app/impl/AppServiceImplTest.java (2)
74-74: Good addition of MockUserContext.Injecting
MockUserContextprovides a stable tenant context (tenant ID "1") for all tests, improving test consistency and predictability.
80-80: Tenant ID standardization looks good.The changes correctly standardize on the concrete tenant ID
"1", which matchesMockUserContext.getTenantId()and addresses the inconsistency flagged in previous reviews. This provides better test specificity compared to using generic matchers likeanyString().Based on past review comments recommending concrete values over generic matchers.
Also applies to: 89-89, 107-108, 122-122
base/src/test/java/com/tinyengine/it/service/app/impl/v1/AppV1ServiceImplTest.java (4)
20-20: LGTM! Imports support tenant context testing.The addition of
ReflectUtilandMockUserContextimports appropriately supports the new reflection-based setup pattern for injecting tenant context into the service under test, aligning with the PR's tenant API modifications.Also applies to: 22-22
101-102: LGTM! Mock addition aligns with new i18n dependency.The
I18nEntryServicemock is correctly added to support the new i18n formatting behavior in the schema generation tests.
110-110: LGTM! Tenant context injection supports API changes.Using
ReflectUtilto injectMockUserContextappropriately sets up the tenant context required by the modified tenant API. This pattern is consistent across the test suite.
147-149: LGTM! I18n setup correctly supports schema generation.The
SchemaI18ninstance creation andi18nEntryService.formatEntriesList(any())stub appropriately support the i18n formatting behavior in the schema generation test.base/src/test/java/com/tinyengine/it/controller/UserControllerTest.java (2)
15-15: LGTM! Import additions support the tenant mock setup.The new imports are appropriate for the updated test that mocks tenant-related functionality.
Also applies to: 23-24, 35-36
51-51: TheauthUsersUnitsRolesMapperfield exists inUserController(line 63) and is properly annotated with@Autowired, making the@Mockand@InjectMockssetup in the test correct. No action required.Likely an incorrect or invalid review comment.
base/src/test/java/com/tinyengine/it/service/app/impl/PageServiceImplTest.java (2)
27-27: LGTM! Multi-tenant test context properly configured.The MockUserContext injection provides the necessary tenant context for testing. The tenant ID "1" returned by MockUserContext.getTenantId() aligns with the hardcoded tenant parameter used in the updated appMapper.queryAppById() calls throughout the test methods.
Also applies to: 99-99
116-116: LGTM! All appMapper.queryAppById() calls consistently updated for multi-tenancy.All five occurrences of
appMapper.queryAppById()have been updated to include the tenant ID parameter "1", which correctly aligns with the new AppMapper signature and the MockUserContext configuration.Also applies to: 156-156, 191-191, 227-227, 250-250
2 participants
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Chores
Documentation
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.