SbloodyS commented on code in PR #18703:
URL:
https://github.com/apache/dolphinscheduler/pull/18703#discussion_r4267254215
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/SessionServiceImpl.java:
##########
@@ -71,7 +71,9 @@ public Session createSessionIfAbsent(User user) {
}
}
session = sessionList.get(0);
- if (isSessionExpire(session)) {
+ // An unexpired session is reused (its last-login time refreshed);
an
+ // expired one is deleted so a new session is created below
Review Comment:
```suggestion
```
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/SessionServiceImpl.java:
##########
@@ -71,7 +71,9 @@ public Session createSessionIfAbsent(User user) {
}
}
session = sessionList.get(0);
- if (isSessionExpire(session)) {
+ // An unexpired session is reused (its last-login time refreshed);
an
Review Comment:
```suggestion
```
##########
dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/SessionServiceTest.java:
##########
@@ -120,6 +120,51 @@ public void testIsSessionExpireUsesConfiguredTimeout() {
Assertions.assertTrue(sessionService.isSessionExpire(expired));
}
+ /**
+ * #18699: an unexpired session must be reused on re-login, not deleted and
+ * replaced (the expired/unexpired branches were inverted before the fix)
+ */
+ @Test
+ public void testCreateSessionIfAbsentReusesUnexpiredSession() {
+ User user = new User();
+ user.setUserType(UserType.GENERAL_USER);
+ user.setId(1);
+ // fixture session's lastLoginTime is 40 days in the future ->
unexpired
+ Mockito.when(sessionDao.queryByUserId(1)).thenReturn(getSessions());
+
+ Session session = sessionService.createSessionIfAbsent(user);
+
+ Assertions.assertNotNull(session);
+ Assertions.assertEquals(sessionId, session.getId());
+ Mockito.verify(sessionDao).updateById(session);
+ Mockito.verify(sessionDao,
Mockito.never()).deleteById(Mockito.anyString());
+ Mockito.verify(sessionDao,
Mockito.never()).insert(Mockito.any(Session.class));
+ }
+
+ /**
+ * #18699: an expired session must be replaced by a new one on re-login,
+ * not refreshed and reused (the expired/unexpired branches were inverted
+ * before the fix)
+ */
Review Comment:
```suggestion
```
##########
dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/SessionServiceTest.java:
##########
@@ -120,6 +120,51 @@ public void testIsSessionExpireUsesConfiguredTimeout() {
Assertions.assertTrue(sessionService.isSessionExpire(expired));
}
+ /**
+ * #18699: an unexpired session must be reused on re-login, not deleted and
+ * replaced (the expired/unexpired branches were inverted before the fix)
+ */
Review Comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
