Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 47 minutes and 56 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughRemoved JwtUtil's fallback secret and made it read Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
base/src/main/java/com/tinyengine/it/login/utils/JwtUtil.java (1)
48-55:⚠️ Potential issue | 🟠 MajorNPE risk and missing key validation after removing default secret.
With
DEFAULT_SECRETremoved, ifSECRET_STRINGis unsetgetSecretString()returnsnullandgetSecretKey()throws an opaqueNullPointerExceptionon.getBytes()— every token issue/validate path (includingvalidateToken) will then fail with a generic exception rather than a clear configuration error. Additionally,Keys.hmacShaKeyForrequires the key to be at least 32 bytes for HS256; a shortSECRET_STRINGwill throwWeakKeyExceptionat runtime with no early feedback.Please fail fast at startup (or on first use) with a descriptive message, e.g.:
🛡️ Suggested guard
private static String getSecretString() { - return System.getenv("SECRET_STRING"); + String secret = System.getenv("SECRET_STRING"); + if (secret == null || secret.isEmpty()) { + throw new IllegalStateException( + "Environment variable SECRET_STRING is not set; cannot initialize JWT signing key."); + } + if (secret.getBytes(java.nio.charset.StandardCharsets.UTF_8).length < 32) { + throw new IllegalStateException( + "SECRET_STRING must be at least 32 bytes for HS256."); + } + return secret; }Also note that
Optional(line 32) is now unused after this change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/src/main/java/com/tinyengine/it/login/utils/JwtUtil.java` around lines 48 - 55, getSecretString can return null and getSecretKey currently calls .getBytes() blindly and passes the result to Keys.hmacShaKeyFor which will either NPE or throw WeakKeyException later; change getSecretKey to validate the environment value from getSecretString() (non-null/non-empty after trim) and ensure its byte length is >= 32 (or the algorithm's minimum) and throw an IllegalStateException (or similar) with a clear message like "SECRET_STRING must be set and at least 32 bytes" so the application fails fast; perform this check inside getSecretKey (or during startup initialization) so validateToken paths don't get opaque NPEs, and remove the now-unused Optional import/field referenced on line 32.
🧹 Nitpick comments (1)
base/src/test/java/com/tinyengine/it/dynamic/service/DynamicModelServiceTest.java (1)
21-21: Avoid wildcard imports in test code.
import java.util.*;(and the staticorg.junit.jupiter.api.Assertions.*/org.mockito.Mockito.*wildcards above) make it harder to see which symbols the test actually depends on and can shadow names when the class grows. Most style guides (and typical project checkstyle configs) disallow them — prefer explicit imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/src/test/java/com/tinyengine/it/dynamic/service/DynamicModelServiceTest.java` at line 21, Replace the wildcard imports in DynamicModelServiceTest (e.g., import java.util.*; and any static wildcards like org.junit.jupiter.api.Assertions.* and org.mockito.Mockito.*) with explicit imports for only the symbols the test actually uses — for example import java.util.List, java.util.Arrays, java.util.Optional (or other specific types used) and static imports such as org.junit.jupiter.api.Assertions.assertEquals, assertTrue, assertThrows and org.mockito.Mockito.when, org.mockito.Mockito.verify — by opening DynamicModelServiceTest, identifying the concrete types and static methods referenced and adding those explicit imports, removing the wildcard lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@base/src/test/java/com/tinyengine/it/dynamic/service/DynamicModelServiceTest.java`:
- Around line 329-341: The testCreateData test stubs the wrong JdbcTemplate
overload (a non-existent three-arg variant) so the stub never matches the actual
call; update the stub to match the real call used by
dynamicModelService.createData by stubbing
jdbcTemplate.update(PreparedStatementCreator, KeyHolder) (e.g.
when(jdbcTemplate.update(any(PreparedStatementCreator.class),
any(KeyHolder.class))).thenReturn(1)) and adjust the verify to match that
signature (verify(jdbcTemplate).update(any(PreparedStatementCreator.class),
any(KeyHolder.class))); also ensure loginUserContext.getLoginUserId() remains
stubbed and assert the real returned Map contents (or remove the duplicate test
altogether and keep a single correct test for createData) so
assertNotNull(result) is meaningful.
- Around line 187-202: The test stubs two calls to
namedParameterJdbcTemplate.queryForList(String, Map) but the second when(...)
overwrites the first, so the data branch in DynamicModelService.queryWithPage
isn't exercised; update the test in DynamicModelServiceTest to stub the two
calls in order — use
Mockito.when(...).thenReturn(firstDataList).thenReturn(countList) or use
doReturn(firstDataList).doReturn(countList).when(namedParameterJdbcTemplate).queryForList(anyString(),
anyMap()) so the first call returns the row data
(List.of(Map.of("id",1,"name","test_name"))) and the second returns the count
(List.of(Map.of("count",1L))); alternatively, distinguish stubs by matching the
SQL string argument if you prefer, ensuring the sequence matches the actual call
order in queryWithPage.
- Around line 43-51: Remove the duplicate MockitoAnnotations.openMocks(this)
call in DynamicModelServiceTest.setUp and drop the three
ReflectUtil.setFieldValue(...) calls for jdbcTemplate, loginUserContext and
namedParameterJdbcTemplate because `@InjectMocks` on dynamicModelService already
performs constructor injection; either store the AutoCloseable returned by the
single openMocks call and close it in an `@AfterEach` method, or replace manual
initialization with `@ExtendWith`(MockitoExtension.class) on the test class to
handle mock lifecycle automatically.
- Around line 238-247: The Mockito matcher Optional.ofNullable(any()) is used
incorrectly in DynamicModelServiceTest when stubbing jdbcTemplate.queryForList
and jdbcTemplate.update; replace those wrapped matchers with the simple any()
matcher so mocks match the production calls that pass single Long/Object
parameters (update all occurrences noted: the queryForList stubs around
dynamicModelService.getDataById and the jdbcTemplate.update stubs at the other
referenced test lines); ensure you use any() for the parameter arguments in the
when(...) and verify(...) calls targeting jdbcTemplate so Mockito matchers
operate correctly.
---
Outside diff comments:
In `@base/src/main/java/com/tinyengine/it/login/utils/JwtUtil.java`:
- Around line 48-55: getSecretString can return null and getSecretKey currently
calls .getBytes() blindly and passes the result to Keys.hmacShaKeyFor which will
either NPE or throw WeakKeyException later; change getSecretKey to validate the
environment value from getSecretString() (non-null/non-empty after trim) and
ensure its byte length is >= 32 (or the algorithm's minimum) and throw an
IllegalStateException (or similar) with a clear message like "SECRET_STRING must
be set and at least 32 bytes" so the application fails fast; perform this check
inside getSecretKey (or during startup initialization) so validateToken paths
don't get opaque NPEs, and remove the now-unused Optional import/field
referenced on line 32.
---
Nitpick comments:
In
`@base/src/test/java/com/tinyengine/it/dynamic/service/DynamicModelServiceTest.java`:
- Line 21: Replace the wildcard imports in DynamicModelServiceTest (e.g., import
java.util.*; and any static wildcards like org.junit.jupiter.api.Assertions.*
and org.mockito.Mockito.*) with explicit imports for only the symbols the test
actually uses — for example import java.util.List, java.util.Arrays,
java.util.Optional (or other specific types used) and static imports such as
org.junit.jupiter.api.Assertions.assertEquals, assertTrue, assertThrows and
org.mockito.Mockito.when, org.mockito.Mockito.verify — by opening
DynamicModelServiceTest, identifying the concrete types and static methods
referenced and adding those explicit imports, removing the wildcard lines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d97a2ed7-32b6-4f7c-baf5-c56c81cbb0d9
📒 Files selected for processing (2)
base/src/main/java/com/tinyengine/it/login/utils/JwtUtil.javabase/src/test/java/com/tinyengine/it/dynamic/service/DynamicModelServiceTest.java
| @BeforeEach | ||
| void setUp() { | ||
| MockitoAnnotations.openMocks(this); | ||
| MockitoAnnotations.openMocks(this); | ||
| ReflectUtil.setFieldValue(dynamicModelService, "jdbcTemplate", jdbcTemplate); | ||
| ReflectUtil.setFieldValue(dynamicModelService, "loginUserContext", loginUserContext); | ||
| ReflectUtil.setFieldValue(dynamicModelService, "namedParameterJdbcTemplate", namedParameterJdbcTemplate); | ||
|
|
||
| } |
There was a problem hiding this comment.
Redundant openMocks call and unnecessary ReflectUtil usage.
- Line 45–46:
MockitoAnnotations.openMocks(this)is invoked twice; the second call creates a new set of mocks that are then discarded, and leaks the firstAutoCloseable. Keep one call (and ideally close it in@AfterEach, or switch to@ExtendWith(MockitoExtension.class)). - Lines 47–49:
DynamicModelServiceuses Lombok@RequiredArgsConstructoroverfinalfields (seeDynamicModelService.java:32-39), so@InjectMocksalready performs constructor injection with the three mocks. TheReflectUtil.setFieldValue(...)calls are redundant, and if the constructor injection ever silently fails (e.g. a mock is unnamed), reflection will mask that failure.
♻️ Proposed simplification
- `@BeforeEach`
- void setUp() {
- MockitoAnnotations.openMocks(this);
- MockitoAnnotations.openMocks(this);
- ReflectUtil.setFieldValue(dynamicModelService, "jdbcTemplate", jdbcTemplate);
- ReflectUtil.setFieldValue(dynamicModelService, "loginUserContext", loginUserContext);
- ReflectUtil.setFieldValue(dynamicModelService, "namedParameterJdbcTemplate", namedParameterJdbcTemplate);
-
- }
+ `@BeforeEach`
+ void setUp() {
+ MockitoAnnotations.openMocks(this);
+ }📝 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.
| @BeforeEach | |
| void setUp() { | |
| MockitoAnnotations.openMocks(this); | |
| MockitoAnnotations.openMocks(this); | |
| ReflectUtil.setFieldValue(dynamicModelService, "jdbcTemplate", jdbcTemplate); | |
| ReflectUtil.setFieldValue(dynamicModelService, "loginUserContext", loginUserContext); | |
| ReflectUtil.setFieldValue(dynamicModelService, "namedParameterJdbcTemplate", namedParameterJdbcTemplate); | |
| } | |
| `@BeforeEach` | |
| void setUp() { | |
| MockitoAnnotations.openMocks(this); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@base/src/test/java/com/tinyengine/it/dynamic/service/DynamicModelServiceTest.java`
around lines 43 - 51, Remove the duplicate MockitoAnnotations.openMocks(this)
call in DynamicModelServiceTest.setUp and drop the three
ReflectUtil.setFieldValue(...) calls for jdbcTemplate, loginUserContext and
namedParameterJdbcTemplate because `@InjectMocks` on dynamicModelService already
performs constructor injection; either store the AutoCloseable returned by the
single openMocks call and close it in an `@AfterEach` method, or replace manual
initialization with `@ExtendWith`(MockitoExtension.class) on the test class to
handle mock lifecycle automatically.
| List<Map<String, Object>> mockData = new ArrayList<>(); | ||
| mockData.add(Map.of("id", 1, "name", "test_name")); | ||
|
|
||
| when(namedParameterJdbcTemplate.queryForList(anyString(), anyMap())).thenReturn(mockData); | ||
| when(namedParameterJdbcTemplate.queryForList(anyString(), anyMap())).thenReturn(List.of(Map.of("count", 1L))); | ||
|
|
||
| // Act | ||
| Map<String, Object> result = dynamicModelService.queryWithPage(dto); | ||
|
|
||
| // Assert | ||
| assertNotNull(result); | ||
| assertTrue((Boolean) result.get("success")); | ||
| assertEquals(1L, result.get("total")); | ||
| assertEquals(1, ((List<?>) result.get("data")).size()); | ||
| verify(namedParameterJdbcTemplate, times(2)).queryForList(anyString(), anyMap()); | ||
| } |
There was a problem hiding this comment.
queryWithPage test: second stubbing overrides the first — data branch is never exercised.
Both when(...) calls on lines 190 and 191 target the exact same method signature queryForList(String, Map), so Mockito replaces the first stub with the second. Both the data query and the count query inside queryWithPage will now return [{count=1}], so result.get("data") depends on how the service interprets that single row — the assertions pass only incidentally and the test does not actually validate pagination.
Use sequential stubbing (order must match the production call order) or distinguish by SQL:
♻️ Proposed fix
- when(namedParameterJdbcTemplate.queryForList(anyString(), anyMap())).thenReturn(mockData);
- when(namedParameterJdbcTemplate.queryForList(anyString(), anyMap())).thenReturn(List.of(Map.of("count", 1L)));
+ when(namedParameterJdbcTemplate.queryForList(anyString(), anyMap()))
+ .thenReturn(mockData) // first call: data page
+ .thenReturn(List.of(Map.of("count", 1L))); // second call: total countConfirm the actual call order in queryWithPage before finalizing.
📝 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.
| List<Map<String, Object>> mockData = new ArrayList<>(); | |
| mockData.add(Map.of("id", 1, "name", "test_name")); | |
| when(namedParameterJdbcTemplate.queryForList(anyString(), anyMap())).thenReturn(mockData); | |
| when(namedParameterJdbcTemplate.queryForList(anyString(), anyMap())).thenReturn(List.of(Map.of("count", 1L))); | |
| // Act | |
| Map<String, Object> result = dynamicModelService.queryWithPage(dto); | |
| // Assert | |
| assertNotNull(result); | |
| assertTrue((Boolean) result.get("success")); | |
| assertEquals(1L, result.get("total")); | |
| assertEquals(1, ((List<?>) result.get("data")).size()); | |
| verify(namedParameterJdbcTemplate, times(2)).queryForList(anyString(), anyMap()); | |
| } | |
| List<Map<String, Object>> mockData = new ArrayList<>(); | |
| mockData.add(Map.of("id", 1, "name", "test_name")); | |
| when(namedParameterJdbcTemplate.queryForList(anyString(), anyMap())) | |
| .thenReturn(mockData) // first call: data page | |
| .thenReturn(List.of(Map.of("count", 1L))); // second call: total count | |
| // Act | |
| Map<String, Object> result = dynamicModelService.queryWithPage(dto); | |
| // Assert | |
| assertNotNull(result); | |
| assertTrue((Boolean) result.get("success")); | |
| assertEquals(1L, result.get("total")); | |
| assertEquals(1, ((List<?>) result.get("data")).size()); | |
| verify(namedParameterJdbcTemplate, times(2)).queryForList(anyString(), anyMap()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@base/src/test/java/com/tinyengine/it/dynamic/service/DynamicModelServiceTest.java`
around lines 187 - 202, The test stubs two calls to
namedParameterJdbcTemplate.queryForList(String, Map) but the second when(...)
overwrites the first, so the data branch in DynamicModelService.queryWithPage
isn't exercised; update the test in DynamicModelServiceTest to stub the two
calls in order — use
Mockito.when(...).thenReturn(firstDataList).thenReturn(countList) or use
doReturn(firstDataList).doReturn(countList).when(namedParameterJdbcTemplate).queryForList(anyString(),
anyMap()) so the first call returns the row data
(List.of(Map.of("id",1,"name","test_name"))) and the second returns the count
(List.of(Map.of("count",1L))); alternatively, distinguish stubs by matching the
SQL string argument if you prefer, ensuring the sequence matches the actual call
order in queryWithPage.
| when(jdbcTemplate.queryForList(anyString(), Optional.ofNullable(any()))).thenReturn(mockResult); | ||
|
|
||
| // Act | ||
| Map<String, Object> result = dynamicModelService.getDataById(modelId, id); | ||
|
|
||
| // Assert | ||
| assertNotNull(result); | ||
| assertEquals("test_name", result.get("name")); | ||
| verify(jdbcTemplate, times(1)).queryForList(anyString(), Optional.ofNullable(any())); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C2 'jdbcTemplate\.(queryForList|update)\s*\(' base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.javaRepository: opentiny/tiny-engine-backend-java
Length of output: 1355
🏁 Script executed:
cat -n base/src/test/java/com/tinyengine/it/dynamic/service/DynamicModelServiceTest.java | sed -n '230,350p'Repository: opentiny/tiny-engine-backend-java
Length of output: 4583
🏁 Script executed:
cat -n base/src/test/java/com/tinyengine/it/dynamic/service/DynamicModelServiceTest.java | sed -n '350,360p'Repository: opentiny/tiny-engine-backend-java
Length of output: 341
🏁 Script executed:
rg -n 'import.*jdbcTemplate|import.*JdbcTemplate' base/src/test/java/com/tinyengine/it/dynamic/service/DynamicModelServiceTest.javaRepository: opentiny/tiny-engine-backend-java
Length of output: 209
🏁 Script executed:
rg -n -A10 'public.*getDataById|public.*deleteDataById' base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.java | head -60Repository: opentiny/tiny-engine-backend-java
Length of output: 876
🏁 Script executed:
cat -n base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.java | sed -n '640,645p'Repository: opentiny/tiny-engine-backend-java
Length of output: 370
🏁 Script executed:
rg -n 'queryForList|\.update\(' base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.java | grep -E '\.toArray|Object\[\]'Repository: opentiny/tiny-engine-backend-java
Length of output: 206
🏁 Script executed:
rg -n -B2 'jdbcTemplate\.(queryForList|update)' base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.javaRepository: opentiny/tiny-engine-backend-java
Length of output: 1017
🏁 Script executed:
python3 << 'EOF'
# Let's trace what the actual parameter types are in the production code
production_calls = {
"line_586": "queryForList(sql, id)", # id is Long
"line_642": "update(sql, id)", # id is Long
"line_169": "update(sql, values.toArray())", # Object[]
"line_623": "update(sql.toString(), params.toArray())", # Object[]
}
test_stubs = {
"lines_238_246": "queryForList(anyString(), Optional.ofNullable(any()))",
"lines_275_283": "update(anyString(), Optional.ofNullable(any()))",
"lines_349_353": "update(anyString(), Optional.ofNullable(any()))",
}
print("Production code parameter types:")
print("- queryForList(String, Long) - matches test at lines 238, 246")
print("- update(String, Long) - matches test at lines 275, 283, 349, 353")
print("- update(String, Object[]) - different overload")
print()
print("Test stub matcher issues:")
print("- Optional.ofNullable(any()) is INVALID matcher syntax")
print("- Should be any() for single parameter (Long)")
print("- Should be any(Object[].class) for array parameter")
EOFRepository: opentiny/tiny-engine-backend-java
Length of output: 465
Fix invalid Mockito matcher usage: replace Optional.ofNullable(any()) with appropriate matchers.
Optional.ofNullable(any()) wraps the matcher incorrectly, evaluating it outside Mockito's context. The production code calls these methods with single Long parameters, not arrays.
Correct fixes:
- Lines 238, 246 (queryForList): Use
any()instead - Lines 275, 283, 349, 353 (update with single parameter): Use
any()instead
Example:
- when(jdbcTemplate.queryForList(anyString(), Optional.ofNullable(any()))).thenReturn(mockResult);
+ when(jdbcTemplate.queryForList(anyString(), any())).thenReturn(mockResult);
...
- verify(jdbcTemplate, times(1)).update(anyString(), Optional.ofNullable(any())).thenReturn(1);
+ verify(jdbcTemplate, times(1)).update(anyString(), any());Note: The method calls at lines 169 and 623 in production use Object[] parameters, but no test mocks those overloads with this bug.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@base/src/test/java/com/tinyengine/it/dynamic/service/DynamicModelServiceTest.java`
around lines 238 - 247, The Mockito matcher Optional.ofNullable(any()) is used
incorrectly in DynamicModelServiceTest when stubbing jdbcTemplate.queryForList
and jdbcTemplate.update; replace those wrapped matchers with the simple any()
matcher so mocks match the production calls that pass single Long/Object
parameters (update all occurrences noted: the queryForList stubs around
dynamicModelService.getDataById and the jdbcTemplate.update stubs at the other
referenced test lines); ensure you use any() for the parameter arguments in the
when(...) and verify(...) calls targeting jdbcTemplate so Mockito matchers
operate correctly.
| @Test | ||
| void testCreateData() { | ||
| DynamicInsert dataDto = new DynamicInsert(); | ||
| dataDto.setNameEn("test_table"); | ||
| dataDto.setParams(Map.of("name", "test")); | ||
|
|
||
| when(loginUserContext.getLoginUserId()).thenReturn("1"); | ||
| when(jdbcTemplate.update(any(), any(PreparedStatementCreator.class), any())).thenReturn(1); | ||
|
|
||
| Map<String, Object> result = dynamicModelService.createData(dataDto); | ||
| assertNotNull(result); | ||
| verify(jdbcTemplate, times(1)).update(any(PreparedStatementCreator.class), any()); | ||
| } |
There was a problem hiding this comment.
testCreateData stubs a non-matching overload and will not verify what it claims.
Line 336 stubs jdbcTemplate.update(any(), any(PreparedStatementCreator.class), any()) with three arguments, but JdbcTemplate has no 3-arg update overload that accepts PreparedStatementCreator in the middle position. Either this fails to compile/resolves to an unintended varargs overload, or simply never matches the call performed in createData (which uses update(PreparedStatementCreator, KeyHolder), as correctly verified on line 340). The assertNotNull(result) assertion is therefore meaningless.
This whole test* block (lines 287–354) also largely duplicates the earlier createDynamicTable / dropDynamicTable / dynamicQuery / createData / deleteDataById tests — consider removing the duplicates and keeping a single, correct version per scenario.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@base/src/test/java/com/tinyengine/it/dynamic/service/DynamicModelServiceTest.java`
around lines 329 - 341, The testCreateData test stubs the wrong JdbcTemplate
overload (a non-existent three-arg variant) so the stub never matches the actual
call; update the stub to match the real call used by
dynamicModelService.createData by stubbing
jdbcTemplate.update(PreparedStatementCreator, KeyHolder) (e.g.
when(jdbcTemplate.update(any(PreparedStatementCreator.class),
any(KeyHolder.class))).thenReturn(1)) and adjust the verify to match that
signature (verify(jdbcTemplate).update(any(PreparedStatementCreator.class),
any(KeyHolder.class))); also ensure loginUserContext.getLoginUserId() remains
stubbed and assert the real returned Map contents (or remove the duplicate test
altogether and keep a single correct test for createData) so
assertNotNull(result) is meaningful.
There was a problem hiding this comment.
Actionable comments posted: 3
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/login/config/LoginConfig.java (1)
36-48:⚠️ Potential issue | 🟠 MajorRestore the model-data SSO exclusion or confirm the breaking auth change.
addPathPatterns("/**")now appliesSSOInterceptorto/platform-center/api/model-data/**; unauthenticated requests will be rejected before reachingModelDataController. If these APIs are still intended to be public, re-add the exclusion.Proposed fix
"/app-center/api/ai/chat", "/app-center/api/chat/completions", + // 模型数据接口无需 SSO 鉴权 + "/platform-center/api/model-data/**", // 图片文件资源下载 "/material-center/api/resource/download/*"Based on learnings, in the opentiny/tiny-engine-backend-java project, the
/platform-center/api/model-data/**endpoints are intentionally designed to be unauthenticated and should bypass SSO authentication in LoginConfig.java.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/src/main/java/com/tinyengine/it/login/config/LoginConfig.java` around lines 36 - 48, The SSOInterceptor is now applied to all paths via addPathPatterns("/**") which blocks unauthenticated access to intended-public model-data endpoints; update LoginConfig to re-add an exclusion for "/platform-center/api/model-data/**" in the SSOInterceptor configuration (the same excludePathPatterns block that currently lists register/login/forgot-password, AI and resource download paths) so ModelDataController endpoints bypass SSO, or alternatively document/confirm the intentional auth change if you want them protected instead of public.base/src/main/java/com/tinyengine/it/dynamic/service/DynamicService.java (1)
37-47:⚠️ Potential issue | 🟠 MajorRestore sorting parameters in the DAO call.
DynamicSqlProvider.select()readsorderByandorderTypefrom this map, butquery()no longer passes them, so requested sorting is silently ignored.Proposed fix
params.put("fields", dto.getFields()); params.put("pageNum", dto.getCurrentPage()); params.put("pageSize", dto.getPageSize()); + params.put("orderBy", dto.getOrderBy()); + params.put("orderType", dto.getOrderType());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicService.java` around lines 37 - 47, DynamicService.query is not passing sorting parameters to the DAO so DynamicSqlProvider.select cannot apply ordering; update query(DynamicQuery dto) to add the "orderBy" and "orderType" entries into the params map (using the DynamicQuery getters, e.g., getOrderBy() and getOrderType()) before calling dynamicDao.select(params) so orderBy/orderType are available to DynamicSqlProvider.select.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@base/src/main/java/com/tinyengine/it/dynamic/dao/DynamicSqlProvider.java`:
- Around line 11-13: The calls to
SQLIdentifierValidator.isValidIdentifier(tableName) in DynamicSqlProvider
(notably in select, insert, update, and delete methods) ignore the boolean
result so invalid identifiers still proceed; change each call to check the
boolean and throw an IllegalArgumentException (or similar runtime exception)
when it returns false, and remove the duplicate isValidIdentifier call in
update(); update all occurrences referenced (the calls around lines 11-13,
56-58, 74-78, 100-102) so every method validates and fails fast on an invalid
tableName.
In `@base/src/main/java/com/tinyengine/it/dynamic/dto/DynamicQuery.java`:
- Around line 16-17: The `@Pattern` is currently applied to the List field itself
and therefore won't validate each element; update the declaration in class
DynamicQuery so the pattern is applied to the list's element type (i.e., change
the annotation placement to the container-element form so each String in fields
is validated with the regexp "^[a-zA-Z_][a-zA-Z0-9_]*$" and message
"字段名称格式不正确"). Ensure you import the correct javax.validation.constraints.Pattern
(or jakarta equivalent) and keep the field name fields and type List<String>
unchanged.
In `@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicService.java`:
- Around line 238-246: validateFields currently uses field.matches(...) which
throws NPE for null elements; replace that regex check with a call to
SQLIdentifierValidator.isValidIdentifier(field) so null/blank values are handled
consistently. In method validateFields(List<String> fields) iterate as before
but use SQLIdentifierValidator.isValidIdentifier(field) and throw the same
IllegalArgumentException("Field name format is invalid: " + field) when it
returns false; keep the outer null check for fields.
---
Outside diff comments:
In `@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicService.java`:
- Around line 37-47: DynamicService.query is not passing sorting parameters to
the DAO so DynamicSqlProvider.select cannot apply ordering; update
query(DynamicQuery dto) to add the "orderBy" and "orderType" entries into the
params map (using the DynamicQuery getters, e.g., getOrderBy() and
getOrderType()) before calling dynamicDao.select(params) so orderBy/orderType
are available to DynamicSqlProvider.select.
In `@base/src/main/java/com/tinyengine/it/login/config/LoginConfig.java`:
- Around line 36-48: The SSOInterceptor is now applied to all paths via
addPathPatterns("/**") which blocks unauthenticated access to intended-public
model-data endpoints; update LoginConfig to re-add an exclusion for
"/platform-center/api/model-data/**" in the SSOInterceptor configuration (the
same excludePathPatterns block that currently lists
register/login/forgot-password, AI and resource download paths) so
ModelDataController endpoints bypass SSO, or alternatively document/confirm the
intentional auth change if you want them protected instead of public.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e68a2aff-687c-41ab-88f7-7b138b5e5bf3
📒 Files selected for processing (5)
base/src/main/java/com/tinyengine/it/dynamic/dao/DynamicSqlProvider.javabase/src/main/java/com/tinyengine/it/dynamic/dto/DynamicQuery.javabase/src/main/java/com/tinyengine/it/dynamic/service/DynamicService.javabase/src/main/java/com/tinyengine/it/dynamic/util/SQLIdentifierValidator.javabase/src/main/java/com/tinyengine/it/login/config/LoginConfig.java
| public String select(Map<String, Object> params) { | ||
| String tableName = (String) params.get("tableName"); | ||
| SQLIdentifierValidator.isValidIdentifier(tableName); |
There was a problem hiding this comment.
Throw when the table name is invalid.
These calls ignore the boolean return value, so invalid table names still flow into FROM / INSERT_INTO / UPDATE / DELETE_FROM. The duplicate call in update() should also be removed.
Proposed fix
public class DynamicSqlProvider {
+
+ private void validateTableName(String tableName) {
+ if (!SQLIdentifierValidator.isValidIdentifier(tableName)) {
+ throw new IllegalArgumentException("表名格式不正确: " + tableName);
+ }
+ }
public String select(Map<String, Object> params) {
String tableName = (String) params.get("tableName");
- SQLIdentifierValidator.isValidIdentifier(tableName);
+ validateTableName(tableName);
@@
public String insert(Map<String, Object> params) {
String tableName = (String) params.get("tableName");
- SQLIdentifierValidator.isValidIdentifier(tableName);
+ validateTableName(tableName);
@@
public String update(Map<String, Object> params) {
String tableName = (String) params.get("tableName");
- SQLIdentifierValidator.isValidIdentifier(tableName);
-
- SQLIdentifierValidator.isValidIdentifier(tableName);
+ validateTableName(tableName);
@@
public String delete(Map<String, Object> params) {
String tableName = (String) params.get("tableName");
- SQLIdentifierValidator.isValidIdentifier(tableName);
+ validateTableName(tableName);Also applies to: 56-58, 74-78, 100-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@base/src/main/java/com/tinyengine/it/dynamic/dao/DynamicSqlProvider.java`
around lines 11 - 13, The calls to
SQLIdentifierValidator.isValidIdentifier(tableName) in DynamicSqlProvider
(notably in select, insert, update, and delete methods) ignore the boolean
result so invalid identifiers still proceed; change each call to check the
boolean and throw an IllegalArgumentException (or similar runtime exception)
when it returns false, and remove the duplicate isValidIdentifier call in
update(); update all occurrences referenced (the calls around lines 11-13,
56-58, 74-78, 100-102) so every method validates and fails fast on an invalid
tableName.
| @Pattern(regexp = "^[a-zA-Z_][a-zA-Z0-9_]*$", message = "字段名称格式不正确") | ||
| private List<String> fields; // 查询字段 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Jakarta Bean Validation @pattern supported validated types and container element validation List<@Pattern String>
💡 Result:
Jakarta Bean Validation @Pattern supports validation of CharSequence types, specifically String and other CharSequence implementations. It does not directly support other primitive or object types like Integer or List. Container element validation for List<@Pattern String> is supported in Bean Validation 2.0+ (including Jakarta Bean Validation 3.0). Constraints like @Pattern can be placed on the type parameter of generic containers such as List, Set, Map (values), arrays, and Optional. Built-in value extractors handle Iterable (including List), arrays, Map values/keys, and Optional. Example: private List<@Pattern(regexp = "[a-zA-Z0-9]*") String> myList; This validates each String element in the List against the regex. Null elements are valid per @Pattern spec. Empty collections skip element validation. This works via value extraction: the validation engine uses extractors for List/Iterable to pull each String and apply the @Pattern constraint.
Citations:
- 1: https://jakarta.ee/specifications/bean-validation/3.0/apidocs/jakarta/validation/constraints/pattern
- 2: https://www.baeldung.com/bean-validation-container-elements
- 3: https://stackoverflow.com/questions/20096155/pattern-annotation-on-list-of-strings
- 4: https://jakarta.ee/specifications/bean-validation/3.0/jakarta-bean-validation-spec-3.0.html
- 5: https://beanvalidation.org/proposals/BVAL-508-appendix/
- 6: https://github.com/hibernate/hibernate-validator/blob/d30d626b/documentation/src/main/asciidoc/ch07.asciidoc
🏁 Script executed:
cat -n base/src/main/java/com/tinyengine/it/dynamic/dto/DynamicQuery.java | head -50Repository: opentiny/tiny-engine-backend-java
Length of output: 1145
Apply @Pattern to the list elements, not the List itself.
@Pattern on List<String> does not validate each field name; use container-element validation so each requested field is checked against the pattern.
Proposed fix
- `@Pattern`(regexp = "^[a-zA-Z_][a-zA-Z0-9_]*$", message = "字段名称格式不正确")
- private List<String> fields; // 查询字段
+ private List<@Pattern(regexp = "^[a-zA-Z_][a-zA-Z0-9_]*$", message = "字段名称格式不正确") String> fields; // 查询字段📝 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.
| @Pattern(regexp = "^[a-zA-Z_][a-zA-Z0-9_]*$", message = "字段名称格式不正确") | |
| private List<String> fields; // 查询字段 | |
| private List<@Pattern(regexp = "^[a-zA-Z_][a-zA-Z0-9_]*$", message = "字段名称格式不正确") String> fields; // 查询字段 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@base/src/main/java/com/tinyengine/it/dynamic/dto/DynamicQuery.java` around
lines 16 - 17, The `@Pattern` is currently applied to the List field itself and
therefore won't validate each element; update the declaration in class
DynamicQuery so the pattern is applied to the list's element type (i.e., change
the annotation placement to the container-element form so each String in fields
is validated with the regexp "^[a-zA-Z_][a-zA-Z0-9_]*$" and message
"字段名称格式不正确"). Ensure you import the correct javax.validation.constraints.Pattern
(or jakarta equivalent) and keep the field name fields and type List<String>
unchanged.
| private void validateFields(List<String> fields) { | ||
| if (fields != null) { | ||
| for (String field : fields) { | ||
| if (!field.matches("^[a-zA-Z_][a-zA-Z0-9_]*$")) { | ||
| throw new IllegalArgumentException("Field name format is invalid: " + field); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Use the identifier validator here to handle nulls consistently.
field.matches(...) throws NullPointerException for a null element; SQLIdentifierValidator.isValidIdentifier() already handles null/blank values safely.
Proposed fix
private void validateFields(List<String> fields) {
if (fields != null) {
for (String field : fields) {
- if (!field.matches("^[a-zA-Z_][a-zA-Z0-9_]*$")) {
+ if (!SQLIdentifierValidator.isValidIdentifier(field)) {
throw new IllegalArgumentException("Field name format is invalid: " + field);
}
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicService.java`
around lines 238 - 246, validateFields currently uses field.matches(...) which
throws NPE for null elements; replace that regex check with a call to
SQLIdentifierValidator.isValidIdentifier(field) so null/blank values are handled
consistently. In method validateFields(List<String> fields) iterate as before
but use SQLIdentifierValidator.isValidIdentifier(field) and throw the same
IllegalArgumentException("Field name format is invalid: " + field) when it
returns false; keep the outer null check for fields.
1 participant
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
Tests
Refactor
Validation
Behavior