Skip to content
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

Improve session support to be more friendly to persistent and expensive stores. #447

Closed
ldaley opened this issue Sep 27, 2014 · 15 comments
Closed
Assignees

Comments

@ldaley
Copy link
Member

ldaley commented Sep 27, 2014

What is currently SessionStorage should become:

public interface SessionStorage {

  // Returned object is not live, is a copy of the data at read time
  <T extends Serializable> Promise<Optional<T>> get(TypeToken<T> type);

  <T extends Serializable> Promise<Boolean> put(TypeToken<T> type, T t);

  <T extends Serializable> Promise<Integer> remove(TypeToken<T> type);

  Promise<Optional<String>> get(String key);

  Promise<Boolean> set(String key, String value);

  Promise<Integer> remove(String key);

  Promise<Integer> clear();

}
  1. Reads from the session always return value objects (i.e. changes to the objects are not automatically written back to the session)
  2. Type based storage is based on equality, not assignability
@wmacgyver
Copy link

@alkemist you want this to lead to ratpack-hazelcast in the end, any particular reason on hazelcast vs say redis? Don't get me wrong, I use and love hazelcast. I'm just curious if there was a reason. Is it because it's very easy to integrate on JVM?

@ldaley
Copy link
Member Author

ldaley commented Sep 29, 2014

@wmacgyver I do want both.

That is, the idea is that you can drop in different impls.

@davidmc24
Copy link
Contributor

Do we want to add support for other session-related capabilities as part of this? Invalidate, get/set timeout, start time, last access time?

Link to equivalent shiro interface:
http://shiro.apache.org/static/1.2.1/apidocs/org/apache/shiro/session/Session.html

@wmacgyver
Copy link

wow, feature creep already :)

@ldaley
Copy link
Member Author

ldaley commented Sep 29, 2014

@davidmc24 I'd say a lot of that makes sense to include.

@beckje01
Copy link
Member

For a first cut I landed on the following:

public interface SessionStorage  {

  <T> Promise<Optional<T>> get(String key, Class<T> type);

  Promise<Boolean> set(String key, Object value);

  Promise<Set<String>> getKeys();

  Promise<Integer> remove(String key);

  Promise<Integer> clear();
}

This will allow expensive stores but didn't get into the Type based storage.

@wmacgyver
Copy link

shouldn't invalid/timeout be part of the interface too?

@ldaley
Copy link
Member Author

ldaley commented Apr 26, 2015

@wmacgyver can you elaborate?

@beckje01
Copy link
Member

Invalidate and timeout are part of the session itself not the storage inside a session. We currently have these split. So you can invalidate and time out sessions without ever using a session storage.

@wmacgyver
Copy link

@alkemist @beckje01 my bad, I didn't realize it's split this way.

@beckje01
Copy link
Member

@wmacgyver Its confusing look for some docs soon...

@ldaley
Copy link
Member Author

ldaley commented Apr 27, 2015

It's also not set in stone. We may in fact merge these two things.

zedar added a commit to zedar/ratpack that referenced this issue Apr 27, 2015
Conflicts:
	ratpack-session/src/main/java/ratpack/session/clientside/internal/CookieBasedSessionStorageBindingHandler.java
	ratpack-session/src/main/java/ratpack/session/clientside/internal/InitialStorageContainer.java
	ratpack-session/src/test/groovy/ratpack/session/clientside/ClientSideSessionSpec.groovy
@gschrader
Copy link
Contributor

This is a bit of a breaking change and it wasn't obvious at first why. The promise needs to be acted on for the SessionStorage methods to do anything.

this code:

        get("logout") { SessionStorage sessionStorage ->
            sessionStorage.clear()
            redirect("/")
        }

now needs to be:

        get("logout") { SessionStorage sessionStorage ->
            sessionStorage.clear().then({})
            redirect("/")
        }

I'm not sure if there is a cleaner way to handle this.

@ldaley
Copy link
Member Author

ldaley commented May 3, 2015

It actually needs to be:

        get("logout") { SessionStorage sessionStorage ->
            sessionStorage.clear().then { redirect "/" }
        }

Otherwise, you are issuing the redirect and then clearing the session.

Re not subscribing to the session, we are working on some diagnostics that would have helped to point this out.

@gschrader
Copy link
Contributor

Ah cool yes that makes sense now, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants