Merge pull request #6 from jducoeur/enhance-state-changes

Smarter session-state evolution

querki/build.sbt

@@ -5,7 +5,7 @@ import ByteConversions._
 lazy val clients = Seq(querkiClient)
 
 lazy val scalaV = "2.11.12"
-lazy val akkaV = "2.4.10"
+lazy val akkaV = "2.4.18"
 lazy val enumeratumV = "1.5.3"
 lazy val appV = "2.8.6"
 
@@ -47,11 +47,13 @@ lazy val querkiServer = (project in file("scalajvm")).settings(
       "com.lihaoyi" %% "utest" % "0.3.1",
       "org.querki" %% "requester" % "2.6",
       "com.github.mauricio" %% "mysql-async" % "0.2.16",
-      "org.scalatestplus.play" %% "scalatestplus-play" % "1.5.1" % "test",
+//      "org.scalatestplus.play" %% "scalatestplus-play" % "1.5.1" % "test",
+      "org.scalatestplus.play" %% "scalatestplus-play" % "2.0.1" % "test",
       "com.github.romix.akka" %% "akka-kryo-serialization" % "0.4.2",
 //      "com.typesafe.conductr" %% "play25-conductr-bundle-lib" % "1.4.4",
       "com.typesafe.akka" %% "akka-distributed-data-experimental" % akkaV,
-      "org.scalatest" %% "scalatest" % "2.2.6" % "test",
+//      "org.scalatest" %% "scalatest" % "2.2.6" % "test",
+      "org.scalatest" %% "scalatest" % "3.0.3" % "test",
       // Pretty-printer: http://www.lihaoyi.com/upickle-pprint/pprint/
       "com.lihaoyi" %% "pprint" % "0.4.1",
       "com.lihaoyi" %% "sourcecode" % "0.1.4",

querki/project/plugins.sbt

@@ -14,7 +14,7 @@ resolvers += "Sonatype OSS Snapshots" at "https://oss.sonatype.org/content/repos
 resolvers += "bintray/non" at "http://dl.bintray.com/non/maven"
 
 // Use the Play sbt plugin for Play projects
-addSbtPlugin("com.typesafe.play" % "sbt-plugin" % "2.5.3")
+addSbtPlugin("com.typesafe.play" % "sbt-plugin" % "2.5.16")
 
 addSbtPlugin("org.scala-js" % "sbt-scalajs" % "0.6.19")
 addSbtPlugin("com.vmunier" % "sbt-play-scalajs" % "0.3.0")

querki/scala/shared/src/main/scala/querki/api/ThingFunctions.scala

@@ -104,4 +104,7 @@ object ThingFunctions {
   case class TextV(vs: List[String]) extends PV {
     type TContent = String
   }
+  case class LinkV(vs: List[TID]) extends PV {
+    type TContent = TID
+  }
 }

querki/scalajvm/app/querki/conversations/SpaceConversationNotifier.scala

@@ -43,7 +43,7 @@ private [conversations] class SpaceConversationNotifier(e:Ecology, initState:Spa
   }
 
   def doReceive = {
-    case CurrentState(current) => {
+    case CurrentState(current, _) => {
       state = current
     }
 

querki/scalajvm/app/querki/conversations/SpaceConversationsActor.scala

@@ -72,7 +72,7 @@ private [conversations] class SpaceConversationsActor(ecology:Ecology, persisten
     /**
      * This Actor can't become properly active until we receive the current state to work with:
      */
-    case CurrentState(current) => {
+    case CurrentState(current, _) => {
       // Only go through boot if this is the first time we get the state.
       val boot = (state == null)
 
@@ -204,7 +204,7 @@ private [conversations] class SpaceConversationsActor(ecology:Ecology, persisten
     /**
      * Update from the Space Actor that the state has been changed.
      */
-    case CurrentState(current) => {
+    case CurrentState(current, _) => {
       state = current
     }
 

querki/scalajvm/app/querki/conversations/SpaceConversationsManager.scala

@@ -31,14 +31,14 @@ private [conversations] class SpaceConversationsManager(e:Ecology, router:ActorR
   def createChild(thingId:OID):ActorRef = context.actorOf(ThingConversationsActor.actorProps(state, thingId, notifier, ecology))
 
   def bootReceive:Receive = {
-    case CurrentState(current) => {
+    case CurrentState(current, _) => {
       _state = Some(current)
       doneBooting()
     }
   }
 
   def doReceive:Receive = {
-    case msg @ CurrentState(current) => {
+    case msg @ CurrentState(current, _) => {
       _state = Some(current)
       routeToAll(msg)
       notifier ! msg

querki/scalajvm/app/querki/conversations/ThingConversationsCore.scala

@@ -214,7 +214,7 @@ abstract class ThingConversationsCore(initState:SpaceState, val thingId:OID)(imp
     /**
      * Update from the Space Actor that the state has been changed.
      */
-    case CurrentState(current) => {
+    case CurrentState(current, _) => {
       state = current
     }
 

querki/scalajvm/app/querki/session/OldUserSpaceSession.scala

@@ -1,34 +1,30 @@
 package querki.session
 
 import scala.concurrent.Future
-import scala.util.{Failure, Success}
-
+import scala.util.{Success, Failure}
 import akka.actor._
 import akka.contrib.pattern.ReceivePipeline
 import akka.event.LoggingReceive
-
 import upickle.default._
 import autowire._
-
 import org.querki.requester._
-
 import models._
-
 import querki.globals._
-
 import querki.admin.SpaceTimingActor.MonitorMsg
 import querki.api._
 import querki.identity.{Identity, User}
 import querki.history.SpaceHistory._
 import querki.publication.CurrentPublicationState
 import querki.session.messages._
-import querki.spaces.messages.{ChangeProps, CurrentState, SpaceSubsystemRequest, SpacePluginMsg, ThingError, ThingFound}
+import querki.spaces.SpaceEvolution
+import querki.spaces.SpaceMessagePersistence.SpaceEvent
+import querki.spaces.messages.{SpaceSubsystemRequest, ThingError, CurrentState, SpacePluginMsg, ThingFound, ChangeProps}
 import querki.spaces.messages.SpaceError._
 import querki.time.DateTime
 import querki.uservalues.SummarizeChange
 import querki.uservalues.PersistMessages._
-import querki.util.{PublicException, QLog, TimeoutChild, UnexpectedPublicException}
-import querki.values.{QValue, RequestContext, SpaceState}
+import querki.util.{PublicException, UnexpectedPublicException, TimeoutChild, QLog}
+import querki.values.{SpaceState, RequestContext, QValue}
 
 /**
  * The Actor that controls an individual's relationship with a Space.
@@ -39,10 +35,15 @@ import querki.values.{QValue, RequestContext, SpaceState}
  * concerns.
  */
 private [session] class OldUserSpaceSession(e:Ecology, val spaceId:OID, val user:User, val spaceRouter:ActorRef, val persister:ActorRef, timeSpaceOps:Boolean)
-  extends Actor with Stash with Requester with EcologyMember with ReceivePipeline with TimeoutChild
+  extends Actor with Stash with Requester with EcologyMember with ReceivePipeline with TimeoutChild with SpaceEvolution
   with autowire.Server[String, Reader, Writer]
 {
   implicit val ecology = e
+
+  // Needed for SpacePure:
+  // TODO: gah. A fine example of the problems of the inheritance-based approach. Can/should we refactor SpacePure and
+  // SpaceEvolution to *actually* be pure?
+  val id: OID = spaceId
 
   lazy val AccessControl = interface[querki.security.AccessControl]
   lazy val ApiInvocation = interface[querki.api.ApiInvocation]
@@ -60,15 +61,15 @@ private [session] class OldUserSpaceSession(e:Ecology, val spaceId:OID, val user
     }
   }
 
-  /**
-   * IMPORTANT: this must be set before we begin any serious work! This is why we start
-   * in a rudimentary state, and don't become useful until it is received.
-   */
-  var _rawState:Option[SpaceState] = None
-  def setRawState(s:SpaceState) = {
-    clearEnhancedState()
-    _rawState = Some(s)
-  }
+//  /**
+//   * IMPORTANT: this must be set before we begin any serious work! This is why we start
+//   * in a rudimentary state, and don't become useful until it is received.
+//   */
+//  var _rawState:Option[SpaceState] = None
+//  def setRawState(s:SpaceState) = {
+//    clearEnhancedState()
+//    _rawState = Some(s)
+//  }
   var _publicationState:Option[CurrentPublicationState] = None
   def setPublicationState(s:CurrentPublicationState) = {
     clearEnhancedState()
@@ -90,101 +91,70 @@ private [session] class OldUserSpaceSession(e:Ecology, val spaceId:OID, val user
   /**
    * This is the dumping ground for exceptions to the rule that your Space only contains Things you can
    * read. There should *not* be many of these.
+    *
+    * TODO: can be deleted once we lift it out to SpaceEvolution.
    */
   def isReadException(thingId:OID)(implicit state:SpaceState):Boolean = {
     // I always have access to my own Person record, so that _me always works:
     Person.hasPerson(user, thingId)
   }
 
-  var _enhancedState:Option[SpaceState] = None
+  /**
+    * The state as originally received from the Space Actor, with no filtering.
+    *
+    * USE THIS WITH EXTREME CARE! In general, stuff should be using the enhanced state instead.
+    */
+  var _rawState:Option[SpaceState] = None
+  /**
+    * The List of events received since we last recomputed, in *reverse* order for efficiency.
+    */
+  var _unprocessedEvents: List[SpaceEvent] = List.empty
+
+  /**
+    * The raw State, minus stuff that this user isn't allowed to see. This mostly gets evolved with events as they come in.
+    */
+  var _filteredState: Option[SpaceState] = None
+
+  /**
+    * The filtered state, plus publication and userValues if appropriate. Recomputed lazily when needed.
+    */
+  var _enhancedState: Option[SpaceState] = None
   def clearEnhancedState() = _enhancedState = None
-  def makeEnhancedState():SpaceState = {
-    _rawState match {
-      case Some(rs) => {
-        // Managers act as Owners for purposes of being able to read everything:
-        val isManager = AccessControl.isManager(user, rs)
-        val safeState =
-          if (isManager) {
-            monitor(s"makeEnhancedState() skipped -- it is a manager")
-            rs
-          } else {
-            monitor(s"makeEnhancedState() starting read filtering for User ${user.id}...")
-            // TODO: MAKE THIS MUCH FASTER! This is probably O(n**2), maybe worse. We need to do heavy
-            // caching, and do much more sensitive updating as things change.
-            val readFiltered = (rs /: rs.things) { (curState, thingPair) =>
-              val (thingId, thing) = thingPair
-              // Note that we need to pass rs into canRead(), not curState. That is because curState can
-              // be in an inconsistent state while we're in the middle of all this. For example, we may
-              // have already exised a Model from curState (because we don't have permission) when we get
-              // to an Instance of that Model. Things can then get horribly confused when we try to look
-              // at the Instance, try to examine its Model, and *boom*.
-              if (AccessControl.canRead(rs, user, thingId) || isReadException(thingId)(rs)) {
-                // Remove any SystemHidden Properties from this Thing, if there are any:
-                if (hiddenOIDs.exists(thing.props.contains(_))) {
-                  val newThing = thing.copy(pf = { (thing.props -- hiddenOIDs) })
-                  curState.copy(things = (curState.things + (newThing.id -> newThing)))
-                } else
-                  curState
-              } else
-                curState.copy(things = (curState.things - thingId))
-            }
-            monitor(s"... finished read filtering")
-
-            if (AccessControl.canRead(readFiltered, user, rs.id))
-              readFiltered
-            else {
-              // This user isn't allowed to read the Root Page, so give them a stub instead.
-              // TODO: this is a fairly stupid hack, but we have to be careful -- filtering out
-              // bits of the Space while not breaking it entirely is tricky. Think about how to
-              // do this better.
-              readFiltered.copy(pf = readFiltered.props + 
-                  (Basic.DisplayTextProp("**You aren't allowed to read this Page; sorry.**")))
-            }
-          }
-
-        val stateWithUV = (safeState /: userValues) { (curState, uv) =>
-          if (uv.thingId == curState.id) {
-            // We're enhancing the Space itself:
-            curState.copy(pf = (curState.props + (uv.propId -> uv.v)))
-          }
-          else curState.anything(uv.thingId) match {
-            case Some(thing:ThingState) => {
-              val newThing = thing.copy(pf = thing.props + (uv.propId -> uv.v))
-              curState.copy(things = curState.things + (newThing.id -> newThing))
-            }
-            // Yes, this looks like an error, but it isn't: it means that there was a UserValue
-            // for a deleted Thing.
-            case _ => curState
-          }
-        }
-
-        // If there is a PublicationState, overlay that on top of the rest:
-        // TODO (QI.7w4g8n8): there is a known bug here, that if something is Publishable *and* has User Values, the
-        // Publishers currently won't see their User Values if there are changes to the Instance.
-        // TODO (QI.7w4g8nd): this will need rationalization, especially once we get to Experiments. But by and
-        // large, I expect to be bringing more stuff into here.
-        _publicationState.map { ps =>
-          Publication.enhanceState(stateWithUV, ps)
-        }.getOrElse(stateWithUV)
-      }
-      case None => throw new Exception("UserSpaceSession trying to enhance state before there is a rawState!")
-    }
+
+  def handleStateChange(stateChange: CurrentState) = {
+    clearEnhancedState()
+    _rawState = Some(stateChange.state)
+    _unprocessedEvents = stateChange.events.getOrElse(List.empty).reverse ++ _unprocessedEvents
   }
+
   /**
    * The underlying SpaceState, plus all of the UserValues for this Identity.
    * 
    * This is effectively a lazy val that gets recalculated after we get a new rawState.
    */
   def state = {
-    if (_enhancedState.isEmpty) {
-      _enhancedState = Some(makeEnhancedState())
-      whenStateReady(_enhancedState.get)
+    _rawState match {
+      case Some(rawState) => {
+        if (_enhancedState.isEmpty) {
+          _filteredState = Some(updateForUser(_filteredState)(user)(rawState, _unprocessedEvents))
+          _unprocessedEvents = List.empty
+          val withUV = enhanceWithUserValues(_filteredState.get, userValues)
+          _enhancedState = Some(enhanceWithPublication(withUV, _publicationState))
+        }
+        _enhancedState.get
+      }
+      case None => throw new Exception("UserSpaceSession trying to enhance state before there is a rawState!")
     }
-    _enhancedState.get
   }
 
   /**
    * This is executed each time we receive a new SpaceState.
+    *
+    * TODO: we're not currently calling this! Deal with this idiotic edge condition in some smarter way. At the
+    * least, deal with it at load time, not every time we get a new SpaceState as it previously had been doing,
+    * and maybe have an additional handler when I change my name that deals with all current Spaces of mine?
+    *
+    * *And* this seems to be redundant with checkDisplayName() below!!!
    */
   def whenStateReady(implicit state:SpaceState) = {
     // Do I have the right Display Name? If I've changed my Identity's Display Name (which is very common
@@ -268,6 +238,9 @@ private [session] class OldUserSpaceSession(e:Ecology, val spaceId:OID, val user
    * TODO: we're currently checking this on every SpaceSubsystemRequest, which is certainly overkill. Once we have a
    * real UserSession object, and it knows about the UserSpaceSessions, that should instead pro-actively notify
    * all of them about the change, so we don't have to hack around it.
+    *
+    * TODO: is this actually redundant with whenStateReady()?!? Do we have *two* different pathways doing this idiotic
+    * misfeature? It does strongly show just how bad the design is.
    * 
    * TBD: in general, the way we have denormalized the Display Name between Identity and Person is kind of suspicious.
    * There are good efficiency arguments for it, but I am suspicious.
@@ -297,8 +270,8 @@ private [session] class OldUserSpaceSession(e:Ecology, val spaceId:OID, val user
    * stay there for the rest of this actor's life.
    */
   def receive = LoggingReceive {
-    case CurrentState(s) => {
-      setRawState(s)
+    case change @ CurrentState(s, _) => {
+      handleStateChange(change)
       // Now, fetch the UserValues
       // In principle, we should probably parallelize waiting for the SpaceState and UserValues:
       // the current behaviour hits first-load latency slightly. OTOH, in the interest of not hammering
@@ -372,7 +345,7 @@ private [session] class OldUserSpaceSession(e:Ecology, val spaceId:OID, val user
   def mkParams(rc:RequestContext) = AutowireParams(user, Some(SpacePayload(state, spaceRouter)), rc, this, sender)
 
   def normalReceive:Receive = LoggingReceive {
-    case CurrentState(s) => setRawState(s)
+    case change @ CurrentState(s, _) => handleStateChange(change)
 
     case ps:CurrentPublicationState => setPublicationState(ps)
 
@@ -409,7 +382,7 @@ private [session] class OldUserSpaceSession(e:Ecology, val spaceId:OID, val user
 
             // Fetch the state as of that point:
             for {
-              CurrentState(state) <- spaceRouter.request(GetHistoryVersion(v))
+              CurrentState(state, _) <- spaceRouter.request(GetHistoryVersion(v))
             }
             {
               val params = AutowireParams(user, Some(SpacePayload(state, spaceRouter)), rc, this, sender)

querki/scalajvm/app/querki/session/ThingFunctionsImpl.scala

@@ -122,7 +122,8 @@ class ThingFunctionsImpl(info:AutowireParams)(implicit e:Ecology) extends SpaceA
 
           tryType(Core.YesNoType)(BoolV) orElse
           tryType(Core.TextType) { vs => TextV(vs.map(_.text)) } orElse
-          tryType(Core.LargeTextType) { vs => TextV(vs.map(_.text)) } getOrElse {
+          tryType(Core.LargeTextType) { vs => TextV(vs.map(_.text)) } orElse
+          tryType(Core.LinkType) { vs => LinkV(vs.map(_.toTID)) } getOrElse {
             QLog.error(s"getPropertyValues() request for not-yet-implemented type ${pv.prop.pType.displayName}")
             m
           }

querki/scalajvm/app/querki/session/UserSpaceSessions.scala

@@ -92,7 +92,7 @@ private [session] class UserSpaceSessions(e:Ecology, val spaceId:OID, val spaceR
    * We stay in Boot mode until we receive *both* the SpaceState and the CurrentPublicationState.
    */
   def bootReceive = {
-    case msg @ CurrentState(s) => {
+    case msg @ CurrentState(s, _) => {
       state = Some(s)
       bootIfReady()
     }
@@ -110,7 +110,7 @@ private [session] class UserSpaceSessions(e:Ecology, val spaceId:OID, val spaceR
     /**
      * The Space has sent an updated State, so tell everyone about it.
      */
-    case msg @ CurrentState(s) => {
+    case msg @ CurrentState(s, _) => {
       state = Some(s)
       children.foreach(session => session.forward(msg))
     }