Dolphin Session & Client Scope #121

Merged
merged 8 commits into from Mar 17, 2016

Projects

None yet

2 participants

@hendrikebbers
Member

This pull request introduces 3 new parts:

The DolphinSession (Interface) is new part of the public API and can be injected in any controller:

@Inject
DolphinSession dolphinSession

It represents the public API for a Dolphin Platform context and provides similar methods than the session context. Currently each session will contain exactly 1 Dolphin Platform context / DolphinSession but this will change in future (1 DolphinSession for each Tab in a Browser). So instead of using the session context to store and manage data developers should use the DolphinSession.

Based on the Dolphin Platform context / the DolphinSession a new scope was added to Spring that is currently called ClientScope. This can easily be used to define injectable Spring beans that lifecycle is bound to a Dolphin Platform Context:

@ClientScoped
public class MyService() { ... }

Based on the Dolphin Platform context / the DolphinSession a new scope was added to JavaEE / CDI that is currently called ClientScope. This can easily be used to define injectable Spring beans that lifecycle is bound to a Dolphin Platform Context:

@ClientScoped
public class MyService() { ... }

Since the Dolphin Platform context / the DolphinSession is still bound to a session (but will change in future) the lifecycles of the new scopes are equal to the SessionScope.

This new feature are still missing unit tests but currently this can't be tested since the DolphinContextHandler is still a singleton. This will be refactored in my next Pull Request. Once this is done We can create unitTests for the new features.

Review on Reviewable

@hendrikebbers hendrikebbers added this to the 0.8.3 milestone Mar 7, 2016
@aalmiray aalmiray added the in progress label Mar 7, 2016
@hendrikebbers hendrikebbers license headers
1e7979c
@aalmiray
Contributor

Reviewed 16 of 16 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


java-server-javaee/src/main/java/com/canoo/dolphin/server/javaee/ClientScopeContext.java, line 79 [r1] (raw file):
synchronized map or concurrent hash map?


java-server-spring/src/main/java/com/canoo/dolphin/server/spring/DolphinPlatformSpringBootstrap.java, line 19 [r1] (raw file):
spurious import? I don't see any new code that makes use of this import statement


java-server/src/main/java/com/canoo/dolphin/server/DolphinSession.java, line 22 [r1] (raw file):
does it make sense to expose a method that returns a Set of keys? Even an attributeSet method akin to java.util.Map.entrySet?


java-server/src/main/java/com/canoo/dolphin/server/context/DolphinSessionImpl.java, line 32 [r1] (raw file):
wouldn't ConcurrentHashMap work better than a synchronized Map? If there's no contention you'll pay a heavier price by synchronizing all the time.


Comments from the review on Reviewable.io

@hendrikebbers
Member

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


java-server-spring/src/main/java/com/canoo/dolphin/server/spring/DolphinPlatformSpringBootstrap.java, line 19 [r1] (raw file):
Oh, was used in the javadoc as a link. Changed this one


java-server/src/main/java/com/canoo/dolphin/server/DolphinSession.java, line 22 [r1] (raw file):
The HttpSession interface defines "Enumeration getAttributeNames()" but I think a unmodifiable Set would be better. added it


Comments from the review on Reviewable.io

@hendrikebbers hendrikebbers Changes based on review
40b77ee
@hendrikebbers
Member

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


java-server/src/main/java/com/canoo/dolphin/server/context/DolphinSessionImpl.java, line 32 [r1] (raw file):
Yes this will make sense since normally only 1 thread (thread that handles the current http request) will use it.


Comments from the review on Reviewable.io

@hendrikebbers
Member

Did all needed changes based on the review. @netopyr / @aalmiray I will merge this after the meeting next week if we defined a general / final architecture definition.

hendrikebbers added some commits Mar 17, 2016
@hendrikebbers hendrikebbers Merge branch 'master' into client-scope
# Conflicts:
#	java-server-javaee/src/main/java/com/canoo/dolphin/server/javaee/BeanFactory.java
#	java-server-spring/src/main/java/com/canoo/dolphin/server/spring/DolphinPlatformSpringBootstrap.java
#	java-server/src/main/java/com/canoo/dolphin/server/context/DolphinContext.java
ae9c861
@hendrikebbers hendrikebbers Merge branch 'master' into client-scope
# Conflicts:
#	java-server-javaee/src/main/java/com/canoo/dolphin/server/javaee/BeanFactory.java
#	java-server-spring/src/main/java/com/canoo/dolphin/server/spring/DolphinPlatformSpringBootstrap.java
#	java-server/src/main/java/com/canoo/dolphin/server/context/DolphinContext.java
#	java-server/src/main/java/com/canoo/dolphin/server/context/DolphinContextProvider.java
499bbf1
@hendrikebbers hendrikebbers Merge of master in feature branch
65ed546
@hendrikebbers hendrikebbers merged commit 51ab26b into master Mar 17, 2016

2 of 4 checks passed

coverage/coveralls Coverage decreased (-0.2%) to 24.159%
Details
code-review/reviewable Review in progress: 1 of 19 files reviewed, 4 unresolved discussions
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@hendrikebbers hendrikebbers deleted the client-scope branch Mar 17, 2016
@aalmiray aalmiray removed the in progress label Mar 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment