Skip to content

Commit 2406ec8

Browse files
vpavicrwinch
vpavic
authored and
rwinch
committed
Fix SessionCreatedEvent handling in RedisIndexedSessionRepository
At present, RedisIndexedSessionRepository publishes a SessionCreatedEvent backed by an empty MapSession instance. This happens because session delta has been cleared before publishing a message to the session created channel. Fixes gh-1338
1 parent 6d74cf5 commit 2406ec8

File tree

3 files changed

+40
-59
lines changed

3 files changed

+40
-59
lines changed

‎spring-session-data-redis/src/integration-test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryITests.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,10 +111,10 @@ void saves() throws InterruptedException {
111111
this.repository.save(toSave);
112112

113113
assertThat(this.registry.receivedEvent(toSave.getId())).isTrue();
114-
assertThat(this.registry.<SessionCreatedEvent>getEvent(toSave.getId())).isInstanceOf(SessionCreatedEvent.class);
115114
assertThat(this.redis.boundSetOps(usernameSessionKey).members()).contains(toSave.getId());
116115

117-
Session session = this.repository.findById(toSave.getId());
116+
SessionCreatedEvent createdEvent = this.registry.getEvent(toSave.getId());
117+
Session session = createdEvent.getSession();
118118

119119
assertThat(session.getId()).isEqualTo(toSave.getId());
120120
assertThat(session.getAttributeNames()).isEqualTo(toSave.getAttributeNames());

‎spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisIndexedSessionRepository.java

Lines changed: 13 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -462,11 +462,6 @@ public RedisOperations<String, Object> getSessionRedisOperations() {
462462
@Override
463463
public void save(RedisSession session) {
464464
session.save();
465-
if (session.isNew) {
466-
String sessionCreatedKey = getSessionCreatedChannel(session.getId());
467-
this.sessionRedisOperations.convertAndSend(sessionCreatedKey, session.delta);
468-
session.isNew = false;
469-
}
470465
}
471466

472467
public void cleanUpExpiredSessions() {
@@ -507,7 +502,7 @@ private RedisSession getSession(String id, boolean allowExpired) {
507502
if (entries.isEmpty()) {
508503
return null;
509504
}
510-
MapSession loaded = loadSession(id, entries);
505+
MapSession loaded = new RedisSessionMapper(id).apply(entries);
511506
if (!allowExpired && loaded.isExpired()) {
512507
return null;
513508
}
@@ -516,26 +511,6 @@ private RedisSession getSession(String id, boolean allowExpired) {
516511
return result;
517512
}
518513

519-
private MapSession loadSession(String id, Map<String, Object> entries) {
520-
MapSession loaded = new MapSession(id);
521-
for (Map.Entry<String, Object> entry : entries.entrySet()) {
522-
String key = entry.getKey();
523-
if (RedisSessionMapper.CREATION_TIME_KEY.equals(key)) {
524-
loaded.setCreationTime(Instant.ofEpochMilli((long) entry.getValue()));
525-
}
526-
else if (RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY.equals(key)) {
527-
loaded.setMaxInactiveInterval(Duration.ofSeconds((int) entry.getValue()));
528-
}
529-
else if (RedisSessionMapper.LAST_ACCESSED_TIME_KEY.equals(key)) {
530-
loaded.setLastAccessedTime(Instant.ofEpochMilli((long) entry.getValue()));
531-
}
532-
else if (key.startsWith(RedisSessionMapper.ATTRIBUTE_PREFIX)) {
533-
loaded.setAttribute(key.substring(RedisSessionMapper.ATTRIBUTE_PREFIX.length()), entry.getValue());
534-
}
535-
}
536-
return loaded;
537-
}
538-
539514
@Override
540515
public void deleteById(String sessionId) {
541516
RedisSession session = getSession(sessionId, true);
@@ -568,9 +543,13 @@ public void onMessage(Message message, byte[] pattern) {
568543

569544
if (ByteUtils.startsWith(messageChannel, this.sessionCreatedChannelPrefixBytes)) {
570545
// TODO: is this thread safe?
546+
String channel = new String(messageChannel);
547+
String sessionId = channel.substring(channel.lastIndexOf(":") + 1);
571548
@SuppressWarnings("unchecked")
572-
Map<String, Object> loaded = (Map<String, Object>) this.defaultSerializer.deserialize(message.getBody());
573-
handleCreated(loaded, new String(messageChannel));
549+
Map<String, Object> entries = (Map<String, Object>) this.defaultSerializer.deserialize(message.getBody());
550+
MapSession loaded = new RedisSessionMapper(sessionId).apply(entries);
551+
RedisSession session = new RedisSession(loaded, false);
552+
handleCreated(session);
574553
return;
575554
}
576555

@@ -618,9 +597,7 @@ private void cleanupPrincipalIndex(RedisSession session) {
618597
}
619598
}
620599

621-
private void handleCreated(Map<String, Object> loaded, String channel) {
622-
String id = channel.substring(channel.lastIndexOf(":") + 1);
623-
Session session = loadSession(id, loaded);
600+
private void handleCreated(RedisSession session) {
624601
publishEvent(new SessionCreatedEvent(this, session));
625602
}
626603

@@ -874,9 +851,12 @@ private void saveDelta() {
874851
.add(sessionId);
875852
}
876853
}
877-
854+
if (this.isNew) {
855+
String sessionCreatedKey = getSessionCreatedChannel(getId());
856+
RedisIndexedSessionRepository.this.sessionRedisOperations.convertAndSend(sessionCreatedKey, this.delta);
857+
this.isNew = false;
858+
}
878859
this.delta = new HashMap<>(this.delta.size());
879-
880860
Long originalExpiration = (this.originalLastAccessTime != null)
881861
? this.originalLastAccessTime.plus(getMaxInactiveInterval()).toEpochMilli() : null;
882862
RedisIndexedSessionRepository.this.expirationPolicy.onExpirationUpdated(originalExpiration, this);

‎spring-session-data-redis/src/test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryTests.java

Lines changed: 25 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -311,18 +311,18 @@ void redisSessionGetAttributes() {
311311
}
312312

313313
@Test
314-
@SuppressWarnings("unchecked")
315314
void delete() {
316315
String attrName = "attrName";
317316
MapSession expected = new MapSession();
318317
expected.setLastAccessedTime(Instant.now().minusSeconds(60));
319318
expected.setAttribute(attrName, "attrValue");
320319
given(this.redisOperations.<String, Object>boundHashOps(anyString())).willReturn(this.boundHashOperations);
321320
given(this.redisOperations.boundSetOps(anyString())).willReturn(this.boundSetOperations);
322-
Map map = map(RedisIndexedSessionRepository.getSessionAttrNameKey(attrName), expected.getAttribute(attrName),
323-
RedisSessionMapper.CREATION_TIME_KEY, expected.getCreationTime().toEpochMilli(),
324-
RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY, (int) expected.getMaxInactiveInterval().getSeconds(),
325-
RedisSessionMapper.LAST_ACCESSED_TIME_KEY, expected.getLastAccessedTime().toEpochMilli());
321+
Map<String, Object> map = map(RedisIndexedSessionRepository.getSessionAttrNameKey(attrName),
322+
expected.getAttribute(attrName), RedisSessionMapper.CREATION_TIME_KEY,
323+
expected.getCreationTime().toEpochMilli(), RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY,
324+
(int) expected.getMaxInactiveInterval().getSeconds(), RedisSessionMapper.LAST_ACCESSED_TIME_KEY,
325+
expected.getLastAccessedTime().toEpochMilli());
326326
given(this.boundHashOperations.entries()).willReturn(map);
327327
given(this.redisOperations.boundSetOps(anyString())).willReturn(this.boundSetOperations);
328328

@@ -345,7 +345,6 @@ void deleteNullSession() {
345345
}
346346

347347
@Test
348-
@SuppressWarnings("unchecked")
349348
void getSessionNotFound() {
350349
String id = "abc";
351350
given(this.redisOperations.<String, Object>boundHashOps(getKey(id))).willReturn(this.boundHashOperations);
@@ -355,7 +354,6 @@ void getSessionNotFound() {
355354
}
356355

357356
@Test
358-
@SuppressWarnings("unchecked")
359357
void getSessionFound() {
360358
String attribute1 = "attribute1";
361359
String attribute2 = "attribute2";
@@ -365,7 +363,7 @@ void getSessionFound() {
365363
expected.setAttribute(attribute2, null);
366364
given(this.redisOperations.<String, Object>boundHashOps(getKey(expected.getId())))
367365
.willReturn(this.boundHashOperations);
368-
Map map = map(RedisIndexedSessionRepository.getSessionAttrNameKey(attribute1),
366+
Map<String, Object> map = map(RedisIndexedSessionRepository.getSessionAttrNameKey(attribute1),
369367
expected.getAttribute(attribute1), RedisIndexedSessionRepository.getSessionAttrNameKey(attribute2),
370368
expected.getAttribute(attribute2), RedisSessionMapper.CREATION_TIME_KEY,
371369
expected.getCreationTime().toEpochMilli(), RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY,
@@ -386,27 +384,27 @@ void getSessionFound() {
386384
}
387385

388386
@Test
389-
@SuppressWarnings("unchecked")
390387
void getSessionExpired() {
391388
String expiredId = "expired-id";
392389
given(this.redisOperations.<String, Object>boundHashOps(getKey(expiredId)))
393390
.willReturn(this.boundHashOperations);
394-
Map map = map(RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY, 1, RedisSessionMapper.LAST_ACCESSED_TIME_KEY,
391+
Map<String, Object> map = map(RedisSessionMapper.CREATION_TIME_KEY, Instant.EPOCH.toEpochMilli(),
392+
RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY, 1, RedisSessionMapper.LAST_ACCESSED_TIME_KEY,
395393
Instant.now().minus(5, ChronoUnit.MINUTES).toEpochMilli());
396394
given(this.boundHashOperations.entries()).willReturn(map);
397395

398396
assertThat(this.redisRepository.findById(expiredId)).isNull();
399397
}
400398

401399
@Test
402-
@SuppressWarnings("unchecked")
403400
void findByPrincipalNameExpired() {
404401
String expiredId = "expired-id";
405402
given(this.redisOperations.boundSetOps(anyString())).willReturn(this.boundSetOperations);
406403
given(this.boundSetOperations.members()).willReturn(Collections.singleton(expiredId));
407404
given(this.redisOperations.<String, Object>boundHashOps(getKey(expiredId)))
408405
.willReturn(this.boundHashOperations);
409-
Map map = map(RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY, 1, RedisSessionMapper.LAST_ACCESSED_TIME_KEY,
406+
Map<String, Object> map = map(RedisSessionMapper.CREATION_TIME_KEY, Instant.EPOCH.toEpochMilli(),
407+
RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY, 1, RedisSessionMapper.LAST_ACCESSED_TIME_KEY,
410408
Instant.now().minus(5, ChronoUnit.MINUTES).toEpochMilli());
411409
given(this.boundHashOperations.entries()).willReturn(map);
412410

@@ -416,7 +414,6 @@ void findByPrincipalNameExpired() {
416414
}
417415

418416
@Test
419-
@SuppressWarnings("unchecked")
420417
void findByPrincipalName() {
421418
Instant lastAccessed = Instant.now().minusMillis(10);
422419
Instant createdTime = lastAccessed.minusMillis(10);
@@ -426,7 +423,7 @@ void findByPrincipalName() {
426423
given(this.boundSetOperations.members()).willReturn(Collections.singleton(sessionId));
427424
given(this.redisOperations.<String, Object>boundHashOps(getKey(sessionId)))
428425
.willReturn(this.boundHashOperations);
429-
Map map = map(RedisSessionMapper.CREATION_TIME_KEY, createdTime.toEpochMilli(),
426+
Map<String, Object> map = map(RedisSessionMapper.CREATION_TIME_KEY, createdTime.toEpochMilli(),
430427
RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY, (int) maxInactive.getSeconds(),
431428
RedisSessionMapper.LAST_ACCESSED_TIME_KEY, lastAccessed.toEpochMilli());
432429
given(this.boundHashOperations.entries()).willReturn(map);
@@ -468,7 +465,10 @@ void onMessageCreated() {
468465
String channel = "spring:session:event:0:created:" + session.getId();
469466
JdkSerializationRedisSerializer defaultSerailizer = new JdkSerializationRedisSerializer();
470467
this.redisRepository.setDefaultSerializer(defaultSerailizer);
471-
byte[] body = defaultSerailizer.serialize(new HashMap());
468+
Map<String, Object> map = map(RedisSessionMapper.CREATION_TIME_KEY, Instant.EPOCH.toEpochMilli(),
469+
RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY, 0, RedisSessionMapper.LAST_ACCESSED_TIME_KEY,
470+
System.currentTimeMillis() - TimeUnit.MINUTES.toMillis(5));
471+
byte[] body = defaultSerailizer.serialize(map);
472472
DefaultMessage message = new DefaultMessage(channel.getBytes(StandardCharsets.UTF_8), body);
473473

474474
this.redisRepository.setApplicationEventPublisher(this.publisher);
@@ -485,7 +485,10 @@ void onMessageCreatedCustomSerializer() {
485485
byte[] pattern = "".getBytes(StandardCharsets.UTF_8);
486486
byte[] body = new byte[0];
487487
String channel = "spring:session:event:0:created:" + session.getId();
488-
given(this.defaultSerializer.deserialize(body)).willReturn(new HashMap<String, Object>());
488+
Map<String, Object> map = map(RedisSessionMapper.CREATION_TIME_KEY, Instant.EPOCH.toEpochMilli(),
489+
RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY, 0, RedisSessionMapper.LAST_ACCESSED_TIME_KEY,
490+
System.currentTimeMillis() - TimeUnit.MINUTES.toMillis(5));
491+
given(this.defaultSerializer.deserialize(body)).willReturn(map);
489492
DefaultMessage message = new DefaultMessage(channel.getBytes(StandardCharsets.UTF_8), body);
490493
this.redisRepository.setApplicationEventPublisher(this.publisher);
491494

@@ -497,12 +500,12 @@ void onMessageCreatedCustomSerializer() {
497500
}
498501

499502
@Test
500-
@SuppressWarnings("unchecked")
501503
void onMessageDeletedSessionFound() {
502504
String deletedId = "deleted-id";
503505
given(this.redisOperations.<String, Object>boundHashOps(getKey(deletedId)))
504506
.willReturn(this.boundHashOperations);
505-
Map map = map(RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY, 0, RedisSessionMapper.LAST_ACCESSED_TIME_KEY,
507+
Map<String, Object> map = map(RedisSessionMapper.CREATION_TIME_KEY, Instant.EPOCH.toEpochMilli(),
508+
RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY, 0, RedisSessionMapper.LAST_ACCESSED_TIME_KEY,
506509
System.currentTimeMillis() - TimeUnit.MINUTES.toMillis(5));
507510
given(this.boundHashOperations.entries()).willReturn(map);
508511

@@ -525,7 +528,6 @@ void onMessageDeletedSessionFound() {
525528
}
526529

527530
@Test
528-
@SuppressWarnings("unchecked")
529531
void onMessageDeletedSessionNotFound() {
530532
String deletedId = "deleted-id";
531533
given(this.redisOperations.<String, Object>boundHashOps(getKey(deletedId)))
@@ -549,12 +551,12 @@ void onMessageDeletedSessionNotFound() {
549551
}
550552

551553
@Test
552-
@SuppressWarnings("unchecked")
553554
void onMessageExpiredSessionFound() {
554555
String expiredId = "expired-id";
555556
given(this.redisOperations.<String, Object>boundHashOps(getKey(expiredId)))
556557
.willReturn(this.boundHashOperations);
557-
Map map = map(RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY, 1, RedisSessionMapper.LAST_ACCESSED_TIME_KEY,
558+
Map<String, Object> map = map(RedisSessionMapper.CREATION_TIME_KEY, Instant.EPOCH.toEpochMilli(),
559+
RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY, 1, RedisSessionMapper.LAST_ACCESSED_TIME_KEY,
558560
System.currentTimeMillis() - TimeUnit.MINUTES.toMillis(5));
559561
given(this.boundHashOperations.entries()).willReturn(map);
560562

@@ -577,7 +579,6 @@ void onMessageExpiredSessionFound() {
577579
}
578580

579581
@Test
580-
@SuppressWarnings("unchecked")
581582
void onMessageExpiredSessionNotFound() {
582583
String expiredId = "expired-id";
583584
given(this.redisOperations.<String, Object>boundHashOps(getKey(expiredId)))
@@ -813,7 +814,7 @@ void onMessageCreatedInOtherDatabase() {
813814

814815
MapSession session = this.cached;
815816
String channel = "spring:session:event:created:1:" + session.getId();
816-
byte[] body = serializer.serialize(new HashMap());
817+
byte[] body = serializer.serialize(new HashMap<>());
817818
DefaultMessage message = new DefaultMessage(channel.getBytes(StandardCharsets.UTF_8), body);
818819

819820
this.redisRepository.onMessage(message, "".getBytes(StandardCharsets.UTF_8));
@@ -913,7 +914,7 @@ private String getKey(String id) {
913914
return "spring:session:sessions:" + id;
914915
}
915916

916-
private Map map(Object... objects) {
917+
private Map<String, Object> map(Object... objects) {
917918
Map<String, Object> result = new HashMap<>();
918919
if (objects == null) {
919920
return result;

0 commit comments

Comments
 (0)