Skip to content

Add SafeRetrievingSessionRepository #646

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
/*
* Copyright 2014-2016 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.session;

import java.util.Collections;
import java.util.Map;
import java.util.Set;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;

import org.springframework.util.Assert;

/**
* A {@link FindByIndexNameSessionRepository} that decorates the delegating
* {@link SessionRepository}, ignoring the configured set of exceptions during retrieval
* of a {@link Session} from the underlying store. Useful with handling exceptions
* during session deserialization (for example, when serialization UID changes) in
* scenarios where {@link SessionRepository} consumer wants to treat the session that
* wasn't deserializable as non-existing.
* <p>
* By default, this implementation will delete the session whose retrieval using
* {@link #getSession(String)} has failed with the configured ignored exception. This
* behavior can be changed using {@link #setDeleteOnIgnoredException(boolean)} method.
*
* @param <S> the type of {@link Session} being managed
* @author Vedran Pavic
* @since 1.3.0
*/
public class SafeRetrievingSessionRepository<S extends Session>
implements FindByIndexNameSessionRepository<S> {

private static final Log logger = LogFactory.getLog(SafeRetrievingSessionRepository.class);

private final FindByIndexNameSessionRepository<S> delegate;

private final Set<Class<? extends RuntimeException>> ignoredExceptions;

private boolean deleteOnIgnoredException = true;

private boolean logIgnoredException;

/**
* Create a new {@link SafeRetrievingSessionRepository} instance backed by a
* {@link FindByIndexNameSessionRepository} delegate.
* @param delegate the {@link FindByIndexNameSessionRepository} delegate
* @param ignoredExceptions the set of exceptions to ignore
*/
public SafeRetrievingSessionRepository(
FindByIndexNameSessionRepository<S> delegate,
Set<Class<? extends RuntimeException>> ignoredExceptions) {
Assert.notNull(delegate, "Delegate must not be null");
Assert.notEmpty(ignoredExceptions, "Ignored exceptions must not be empty");
this.delegate = delegate;
this.ignoredExceptions = ignoredExceptions;
}

/**
* Create a new {@link SafeRetrievingSessionRepository} instance backed by a
* {@link SessionRepository} delegate.
* @param delegate the {@link SessionRepository} delegate
* @param ignoredExceptions the set of exceptions to ignore
*/
public SafeRetrievingSessionRepository(SessionRepository<S> delegate,
Set<Class<? extends RuntimeException>> ignoredExceptions) {
this(new FindByIndexNameSessionRepositoryAdapter<S>(delegate), ignoredExceptions);
}

/**
* Set whether session should be deleted after ignored exception has occurred during
* retrieval.
* @param deleteOnIgnoredException the flag to indicate whether session should be
* deleted
*/
public void setDeleteOnIgnoredException(boolean deleteOnIgnoredException) {
this.deleteOnIgnoredException = deleteOnIgnoredException;
}

/**
* Set whether ignored exception should be logged.
* @param logIgnoredException the flag to indicate whether to log ignored exceptions
*/
public void setLogIgnoredException(boolean logIgnoredException) {
this.logIgnoredException = logIgnoredException;
}

public S createSession() {
return this.delegate.createSession();
}

public void save(S session) {
this.delegate.save(session);
}

public S getSession(String id) {
try {
return this.delegate.getSession(id);
}
catch (RuntimeException e) {
if (isIgnoredException(e)) {
if (this.logIgnoredException) {
logger.warn("Error occurred while retrieving session " + id + ": "
+ e);
}
if (this.deleteOnIgnoredException) {
this.delegate.delete(id);
}
return null;
}
throw e;
}
}

public void delete(String id) {

Choose a reason for hiding this comment

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

Hi, I have just tested the above using Redis and calling delegate.delete results in a serialization issue as RedisOperationsSessionRepository goes away and tries to get the session again
RedisSession session = getSession(sessionId, true);

Not sure if this is an issue with this code or with RedisOperationsSessionRepository. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for giving this a test run @davidmelia. It's a problem with this code since it doesn't take into account that delegate's delete operation (or any other for that matter) could internally call an operation that can result in a deserialization error.

I guess this is a case in point for implementing some sort of deserialization strategy vs the session repository decorator pattern.

this.delegate.delete(id);
}

public Map<String, S> findByIndexNameAndIndexValue(String indexName, String indexValue) {
try {
return this.delegate.findByIndexNameAndIndexValue(indexName, indexValue);
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible some of the sessions are deserializable while some are not? It would seem nice to be able to return just the ones that are deserializable and delete the ones that are not. Otherwise, repeated attempts to find will always result in an empty result (until any configured timeout of the sessions that cannot be deserialized). I'm not sure how this can be accomplished in a way that supports any FindByIndexNameSessionRepository implementation, though.
At least documenting the behavior in the JavaDoc for this method would be nice, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for raising this concern @shakuzen - I've planned to comment on findByIndexNameAndIndexValue behavior but forgot. I'm not sure what would be the best way to address this due to exact reasons you've described. Delegates will fail even if only one of the results is not deserializable and I don't see a way around it.

}
catch (RuntimeException e) {
if (this.logIgnoredException) {
logger.warn("Error occurred while retrieving sessions with index name '"
+ indexName + "' and value '" + indexValue + "': " + e);
}
if (isIgnoredException(e)) {
return Collections.emptyMap();
}
throw e;
}
}

public boolean isIgnoredException(RuntimeException e) {
return this.ignoredExceptions.contains(e.getClass());
}

private static class FindByIndexNameSessionRepositoryAdapter<S extends Session>
implements FindByIndexNameSessionRepository<S> {

private final SessionRepository<S> delegate;

FindByIndexNameSessionRepositoryAdapter(SessionRepository<S> delegate) {
Assert.notNull(delegate, "Delegate must not be null");
this.delegate = delegate;
}

public S createSession() {
return this.delegate.createSession();
}

public void save(S session) {
this.delegate.save(session);
}

public S getSession(String id) {
return this.delegate.getSession(id);
}

public void delete(String id) {
this.delegate.delete(id);
}

public Map<String, S> findByIndexNameAndIndexValue(String indexName,
String indexValue) {
throw new UnsupportedOperationException();
}

}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,213 @@
/*
* Copyright 2014-2016 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.session;

import java.util.Collections;
import java.util.UUID;

import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.runners.MockitoJUnitRunner;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.BDDMockito.given;
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyString;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyZeroInteractions;

/**
* Tests for {@link SafeRetrievingSessionRepository}.
*
* @author Vedran Pavic
*/
@RunWith(MockitoJUnitRunner.class)
public class SafeRetrievingSessionRepositoryTests {

@Rule
public ExpectedException thrown = ExpectedException.none();

@Mock
private FindByIndexNameSessionRepository<ExpiringSession> delegate;

@Test
public void createWithNullDelegateFails() {
this.thrown.expect(IllegalArgumentException.class);
this.thrown.expectMessage("Delegate must not be null");
new SafeRetrievingSessionRepository<Session>(null,
Collections.<Class<? extends RuntimeException>>singleton(RuntimeException.class));
}

@Test
public void createWithNullIgnoredExceptionsFails() {
this.thrown.expect(IllegalArgumentException.class);
this.thrown.expectMessage("Ignored exceptions must not be empty");
new SafeRetrievingSessionRepository<ExpiringSession>(this.delegate, null);
}

@Test
public void createWithEmptyIgnoredExceptionsFails() {
this.thrown.expect(IllegalArgumentException.class);
this.thrown.expectMessage("Ignored exceptions must not be empty");
new SafeRetrievingSessionRepository<ExpiringSession>(this.delegate,
Collections.<Class<? extends RuntimeException>>emptySet());
}

@Test
public void createSession() {
SafeRetrievingSessionRepository<ExpiringSession> repository =
new SafeRetrievingSessionRepository<ExpiringSession>(this.delegate,
Collections.<Class<? extends RuntimeException>>singleton(RuntimeException.class));
repository.createSession();
verify(this.delegate, times(1)).createSession();
verifyZeroInteractions(this.delegate);
}

@Test
public void saveSession() {
SafeRetrievingSessionRepository<ExpiringSession> repository =
new SafeRetrievingSessionRepository<ExpiringSession>(this.delegate,
Collections.<Class<? extends RuntimeException>>singleton(RuntimeException.class));
repository.save(new MapSession());
verify(this.delegate, times(1)).save(any(ExpiringSession.class));
verifyZeroInteractions(this.delegate);
}

@Test
public void getSession() {
ExpiringSession session = mock(ExpiringSession.class);
given(this.delegate.getSession(anyString())).willReturn(session);
SafeRetrievingSessionRepository<ExpiringSession> repository =
new SafeRetrievingSessionRepository<ExpiringSession>(this.delegate,
Collections.<Class<? extends RuntimeException>>singleton(RuntimeException.class));
assertThat(repository.getSession(UUID.randomUUID().toString()))
.isEqualTo(session);
verify(this.delegate, times(1)).getSession(anyString());
verifyZeroInteractions(this.delegate);
}

@Test
@SuppressWarnings("unchecked")
public void getSessionThrowsIgnoredException() {
given(this.delegate.getSession(anyString())).willThrow(RuntimeException.class);
SafeRetrievingSessionRepository<ExpiringSession> repository =
new SafeRetrievingSessionRepository<ExpiringSession>(this.delegate,
Collections.<Class<? extends RuntimeException>>singleton(RuntimeException.class));
assertThat(repository.getSession(UUID.randomUUID().toString())).isNull();
verify(this.delegate, times(1)).getSession(anyString());
verify(this.delegate, times(1)).delete(anyString());
verifyZeroInteractions(this.delegate);
}

@Test
@SuppressWarnings("unchecked")
public void getSessionThrowsIgnoredExceptionWithDeletionDisabled() {
given(this.delegate.getSession(anyString())).willThrow(RuntimeException.class);
SafeRetrievingSessionRepository<ExpiringSession> repository =
new SafeRetrievingSessionRepository<ExpiringSession>(this.delegate,
Collections.<Class<? extends RuntimeException>>singleton(RuntimeException.class));
repository.setDeleteOnIgnoredException(false);
assertThat(repository.getSession(UUID.randomUUID().toString())).isNull();
verify(this.delegate, times(1)).getSession(anyString());
verifyZeroInteractions(this.delegate);
}

@Test
@SuppressWarnings("unchecked")
public void getSessionThrowsNotIgnoredException() {
this.thrown.expect(RuntimeException.class);
given(this.delegate.getSession(anyString())).willThrow(RuntimeException.class);
SafeRetrievingSessionRepository<ExpiringSession> repository =
new SafeRetrievingSessionRepository<ExpiringSession>(this.delegate,
Collections.<Class<? extends RuntimeException>>singleton(IllegalStateException.class));
repository.getSession(UUID.randomUUID().toString());
verify(this.delegate, times(1)).getSession(anyString());
verifyZeroInteractions(this.delegate);
}

@Test
public void deleteSession() {
SafeRetrievingSessionRepository<ExpiringSession> repository =
new SafeRetrievingSessionRepository<ExpiringSession>(this.delegate,
Collections.<Class<? extends RuntimeException>>singleton(RuntimeException.class));
repository.delete(UUID.randomUUID().toString());
verify(this.delegate, times(1)).delete(anyString());
verifyZeroInteractions(this.delegate);
}

@Test
public void findByIndexNameAndIndexValue() {
ExpiringSession session = mock(ExpiringSession.class);
given(this.delegate.findByIndexNameAndIndexValue(anyString(), anyString()))
.willReturn(Collections.singletonMap("name", session));
SafeRetrievingSessionRepository<ExpiringSession> repository =
new SafeRetrievingSessionRepository<ExpiringSession>(this.delegate,
Collections.<Class<? extends RuntimeException>>singleton(RuntimeException.class));
assertThat(repository.findByIndexNameAndIndexValue("name", "value").get("name"))
.isEqualTo(session);
verify(this.delegate, times(1)).findByIndexNameAndIndexValue(anyString(),
anyString());
verifyZeroInteractions(this.delegate);
}

@Test
@SuppressWarnings("unchecked")
public void findByIndexNameAndIndexValueThrowsIgnoredException() {
given(this.delegate.findByIndexNameAndIndexValue(anyString(), anyString()))
.willThrow(RuntimeException.class);
SafeRetrievingSessionRepository<ExpiringSession> repository =
new SafeRetrievingSessionRepository<ExpiringSession>(this.delegate,
Collections.<Class<? extends RuntimeException>>singleton(RuntimeException.class));
assertThat(repository.findByIndexNameAndIndexValue("name", "value")).isEmpty();
verify(this.delegate, times(1)).findByIndexNameAndIndexValue(anyString(),
anyString());
verifyZeroInteractions(this.delegate);
}

@Test
@SuppressWarnings("unchecked")
public void findByIndexNameAndIndexValueThrowsNotIgnoredException() {
this.thrown.expect(RuntimeException.class);
given(this.delegate.findByIndexNameAndIndexValue(anyString(), anyString()))
.willThrow(RuntimeException.class);
SafeRetrievingSessionRepository<ExpiringSession> repository =
new SafeRetrievingSessionRepository<ExpiringSession>(this.delegate,
Collections.<Class<? extends RuntimeException>>singleton(IllegalStateException.class));
repository.findByIndexNameAndIndexValue("name", "value");
verify(this.delegate, times(1)).findByIndexNameAndIndexValue(anyString(),
anyString());
verifyZeroInteractions(this.delegate);
}

@Test
@SuppressWarnings("unchecked")
public void findByIndexNameAndIndexValueOnSessionRepositoryThrowsException() {
this.thrown.expect(UnsupportedOperationException.class);
SessionRepository delegate = mock(SessionRepository.class);
SafeRetrievingSessionRepository<ExpiringSession> repository =
new SafeRetrievingSessionRepository<ExpiringSession>(delegate,
Collections.<Class<? extends RuntimeException>>singleton(IllegalStateException.class));
repository.findByIndexNameAndIndexValue("name", "value");
verifyZeroInteractions(delegate);
}

}