On Nov 13, 2012, at 2:15 PM, Vincent Massol <[email protected]> wrote:
> Hi Caleb, > > Thanks for taking the time to provide this feedback! > > See below. > > On Sep 27, 2012, at 9:52 AM, Caleb James DeLisle <[email protected]> > wrote: > >> Hi, >> I took a look at the new model proposal and there are a few questions and >> suggested changes. >> >> >> >> https://github.com/xwiki/xwiki-platform/blob/feature-newmodel/xwiki-platform-core/xwiki-platform-model/xwiki-platform-model-api/src/main/java/org/xwiki/model/Entity.java#L32 >> Should this be a String rather than a Java UUID type? > > Right now it's a String. I don't recall why I proposed String rather than > UUID, maybe to allow for changing the implementation in the future. > >> https://github.com/xwiki/xwiki-platform/blob/feature-newmodel/xwiki-platform-core/xwiki-platform-model/xwiki-platform-model-api/src/main/java/org/xwiki/model/Entity.java#L42 >> If we use getChildren() rather than getChildReferences(), we will be >> mangling the model with the database code which violates the MVC principle. > > I don't agree with this. MVC has nothing to do here. In MVC, the M stands for > Model and this is the only thing we're talking about here. V (for View) and C > (for Controller) are outside of the scope. > > What you hint at (I think) is a Layered Architecture and this we're going to > do, by having the persistence layer in a different layer. However business > objects are completely ok to call the persistence layer. > > My idea is to use a Rich data model (as the current model BTW), i.e. business > operations are *in* the model object interfaces. This is where we need to > agree. I personally don't want to use an Anemic model where you have business > objects be just data holders (ie. only getters/setters) and move all business > operations into a service layer. > > For more on this topic: > * http://en.wikipedia.org/wiki/Anemic_domain_model (lists pros and cons) > * http://www.martinfowler.com/bliki/AnemicDomainModel.html > >> Also we run the risk of encouraging users to walk the tree looking for the >> entity it wants rather than creating a reference to it and using that. >> Walking the tree would obviously incur enormous unnecessary database load. > > That's the reason for the Iterator: > EntityIterator<Entity> getChildren(EntityType type) > > With an Iterator the implementation can do things like: > * get a single item at a time from the DB > * get a bunch of N items at once (in one request) from the DB > > Now you have a valid point that we may also want to have the ability to get > children references. It's a good question. Now if we imagine that parents are > stored as RDBMS column in a document table. Getting all children documents > will mean doing a select in the document table where parent = a given parent > ref ("select parent from document doc where doc.parent = ?"). Now getting > getChildren() would do: select * from document doc where doc.parent = ?. The > extra cost would just be the loading of marshalling/unmarshalling of data > (which you would need thereafter in your logic). But I agree and it's > possible that on other stores, it could cost less to get references. > > So I'd propose to have 2 methods: one for getting only children references > and one for getting all children entities. I forgot to mention that if all you need is to search for some children you should definitely not use this method. In this case you should use the Query API. This method is for when you need the children data. Thanks -Vincent >> https://github.com/xwiki/xwiki-platform/blob/feature-newmodel/xwiki-platform-core/xwiki-platform-model/xwiki-platform-model-api/src/main/java/org/xwiki/model/ModelContext.java >> Do we want to continue with the concept of the global variable representing >> the "current" document? > > There's no global variable. It's a thread local variable (i.e. visible only > for the current thread). > > Yes I'd like to continue this. We didn't have it before in oldcore and > XWikiContext was passed around everywhere which is why we moved to > ThreadLocal. > >> I'm currently not brimming with alternative ideas but the heavy reliance on >> the XWikiContext has lead to >> a lot of inter-modular communication which makes oldcore difficult to >> maintain, perhaps we can prevent >> this in the new model. > > oldcore doesn't use this since it uses XWikiContext and not the Execution > Context. > > Maybe you could explain in more details what you mean by "lot of > inter-modular communication"? > >> https://github.com/xwiki/xwiki-platform/blob/feature-newmodel/xwiki-platform-core/xwiki-platform-model/xwiki-platform-model-api/src/main/java/org/xwiki/model/Object.java >> I would rather this have a name which doesn't collide with a java.lang >> class. When I see Object in a .java >> file I would like it to always mean java.lang.Object. >> >> >> https://github.com/xwiki/xwiki-platform/blob/feature-newmodel/xwiki-platform-core/xwiki-platform-model/xwiki-platform-model-api/src/main/java/org/xwiki/model/ObjectDefinition.java >> I like XClass better for this purpose, it is essentially a class and >> creating a new name which nobody >> has ever heard of will just steepen the learning curve and make things hard >> on newcomers. > > This has been discussed and voted on another thread so these names will be > modified to match the result of that vote. > >> https://github.com/xwiki/xwiki-platform/blob/feature-newmodel/xwiki-platform-core/xwiki-platform-model/xwiki-platform-model-api/src/main/java/org/xwiki/model/ObjectProperty.java >> Why not give the user direct access to the property without the intervening >> wrapper? > > Yes that's possible. However there could other methods such as a method to > get the property definition. > >> https://github.com/xwiki/xwiki-platform/blob/feature-newmodel/xwiki-platform-core/xwiki-platform-model/xwiki-platform-model-api/src/main/java/org/xwiki/model/Persistable.java#L34 >> I don't like this. >> A Persistable is saved in a store by a user. Who is the user and where are >> they saving it? > > The user who started the session. > >> What if I want to have multiple stores? > > Then set the store to use when you start the session. > >> Must there be a global variable representing the store which is implied by >> this function? >> The comment and minorEdit fields are also a bit dubious, perhaps in the >> future they will make no sense, perhaps >> they should really be attributes of the Document, also the "attributes" >> parameter reminds me of this: >> http://geek-and-poke.com/2012/06/how-to-create-a-stable-api.html > > Yep, that was exactly the reason for the attributes parameter ;) > > We can tune this API and remove comment/minorEdit if we want. I'm not sure > about putting them in Document; it's strange from an API POV IMO. I wouldn't > want to use such as API where you have to remember to set the comment in the > Document before calling save... > >> Finally, giving the document awareness of where and how it is stored >> violates the MVC boundries. > > Don't agree, see above. Nothing to do with MVC IMO. > >> I think we should not worry about the API which stores the Persistable when >> we are creating the model, the >> store API should be separate so that it can evolve while the model stays >> stable. > > See above, anemic vs rich model. > >> https://github.com/xwiki/xwiki-platform/blob/feature-newmodel/xwiki-platform-core/xwiki-platform-model/xwiki-platform-model-api/src/main/java/org/xwiki/model/Session.java >> A session is essentially a transaction? If so why not call it Transaction? > > A Session is form a user POV. Technically it will use a Transaction but > that's a technical implementation and it's more than that. It contains a > user; it can contain the store to use, etc. > >> Also why does it extend Persistable? >> How do you persist a session? > > Saving a Session means saving all entities that have been modified and have > not been saved so far. > This is the same as JCR (a lot of stuff come from JCR btw): > http://www.day.com/maven/javax.jcr/javadocs/jcr-2.0/javax/jcr/Session.html > > Note that JCR 2.0 has deprecated Item.save() and I've researched this a bit > but couldn't find the reason beyond "they wanted to simplify the API" and > it's causing problems to users who want to just persist a given Item and > what's below and the not the whole Session. Which is why I've kept having > Entities implement Persistable so far. > >> https://github.com/xwiki/xwiki-platform/blob/feature-newmodel/xwiki-platform-core/xwiki-platform-model/xwiki-platform-model-api/src/main/java/org/xwiki/model/Space.java#L41 >> Why does addSpace() not take a Space parameter? it makes sense to me that >> you would do: >> parent = storeApi.get(wikiRef, "SomeSpace", user); >> child = new DefaultSpace(); >> child.addDocument("ADocument", new DefaultDocument("This is content.")); >> parent.addSpace("ChildSpace", child); >> storeApi.save(parent, user); > > I prefer the current API: > > Entity parent = …; > > Space newSpace = parent.addSpace("child space"); > Document newDocument = newSpace.addDocument("a document"); > newDocument.setContent("This is content"); > > // Save the new entities created below parent > parent.save(…); > > // Note: if we wanted to save all entities modified(not just the one in this > tree) we would write: > session.save(…); > >> I'm just brainstorming here but this seems like a reasonable approach.. > > Your solution doesn't hide the implementation (DefaultSpace() and > DefaultDocument() in this case) and makes it a lot harder to unit test. We > don't want to repeat the issue we have today with unit testing > XWiki/XWikiDocument... > >> I noticed the lack of a User data type, is that just something which is >> planned for the future or is it >> intentionally missing? > > Good question. I don't know yet. A possibility is to pass the user in the > save() call. Another is to use the Entity's author. > > Note that we also need a new User module anyway. > > Thanks > -Vincent > _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
