Re: [gatein-dev] GateIn Api Discussion

On May 25, 2011, at 10:35 AM, Boleslaw Dawidowicz wrote: > I need more time to give more detailed feedback but some quick comments inline. > > On May 25, 2011, at 9:44 AM, Christophe Laprun wrote: >>> >>> First I want to reiterate that for the scope of next release we said we >>> stick with: >>> - application registry features >>> - page/navigation node metadata >>> So >>> - creating a page >>> - adding apps to a page >>> - managing users >>> is out of scope for the coming release. (will be for the release after) > > For users if we drop the ID part of Chris proposal it can be decoupled from the API. Actually I would strongly argue to not complicate this part and leave user and group reference with simple string ("john" and "/platform/users"). You know my point of view on this… :) Again, the Id classes don't need to be as complex as they are in my current proposal. I just wanted to flesh out some generic Id ideas that I had to see if they were actually feasible. Plus, the complexity is not that high from a user perspective. Contrast: Groups.getGroup("/platform/users"); with: Groups.getGroup(Ids.getGroupId("platform", "users"));

for example. How does that complicate things? It is a little more complicated for us to implement but it's simpler for users because they don't need to know what the format of a group name is. They don't need to concatenate and/or parse Strings anytime they need to deal with a group name, etc. In the first version, you need to let users of the API know about the internal format of a group name and basically commit to it for a long time: it becomes an implicit part of the publish API. In the second case, you have a slight overhead of creating an intermediate object but one that you won't need to parse again. Most importantly, you never expose the internals of how groups are named, which makes it easier to evolve the internal implementation without impacting existing code. Also, the validation code goes into the Id creation and you can then assume in the getGroup method that the Id you get is actually valid which makes it easier to implement and not have to duplicate validation code all over the place. This also would help against things like XSS or injection attacks for example.

I would really like to hear a compelling argument against Ids as separate objects because so far, the only arguments I've heard are: I don't like them without any solid arguments as to why we shouldn't use them. Please enlighten me as I only see advantages to them! ;)

>>> Some rough notes I took while looking into it, I mainly looked at the >>> testcases/usecases >>> >>> https://github.com/metacosm/gatein-api/blob/master/src/test/java/org/gate... >>> Looks like 2 first tests are contradicting themselves: >>> assert portal.getContentRegistry().getCategory("inexistent") == null; >>> assert portal.getContentRegistry().getCategory("inexistent") != null; >> >> Yes, I noticed that as well and this has already been addressed in the code. Note that I'm still debating wether getOrCreate methods are useful or not. They are useful for users who just want to retrieve something whether it previously existed or not. I'm not sure whether that's a predominant use case but I personally thought that it makes writing code against the API a little cleaner by avoiding a lot of boilerplate code. It also maps reasonably well to REST with the contract that GET will never fail (though I need to think this one through more) :) >> >>> ************ >>> public void assigningContentToAWindowOnAPage() >>> is out of scope of what we will provide in that version, we agreed to >>> have read-only info about pages/navigation nodes information without the >>> info about page compositions. >> >> I added this use case because this is actually something I need for WSRP (and I suspect Nick would need as well) import/export so I wanted to see how that particular scenario could work out. >> > > Commenting on both getOrCreate() and assigningContentToAWindowOnAPage()... I know you and Nick may need it and I also understand that there may be good case for performance reasons. However we should place such dedicated or specialized method in internal API where it is fine but not in the public API. This is meant to be simple. Otherwise we'll end up with API bloated with lots and lots of methods like because of different needs. We should take our use cases into account but not to this level. Also I support the view that we should keep it simple and minimal - it is always possible to add and extend. Also it is easier to deprecate if we keep it simple getOrCreate actually doesn't make any sense on an internal API (well, at the implementation level, maybe) but it makes more sense at the user level to keep the code as simple as possible. Compare: Window window; if (!page.hasChild(windowName)) { window = page.createChild(windowName, Window.class); } else { window = page.getChild(windowName, Window.class); } with: Window window = page.getOrCreateChild(windowName, Window.class); when you only want to get a window with a given name and you don't care whether it's a new window or not, you just want a window with that name. I think this use case makes sense when assembling a portal from scratch and you just want to get it done in as little code as possible. Imagine having to create lots of pages and assigning content to it and you'll get bored quickly of the check then create or get boilerplate code… :) Then it's a matter of which use cases we want to make easier for people. I agree that the API should probably be as simple as possible first so that usage patterns can develop and then we can improve things but even in my tiny use cases I got bored of the boilerplate code quickly so I can envision users doing the same :) On assigning content to a window, this is something that is needed by users as well: how else do you tell the portal that you want such and such content to be displayed on a given page? This last one as nothing to do with performance. Regardless of how you do it, users will need to be able to tell the API: I want this portlet displayed here on this page. Whether this is something we want the API to do in the first iteration or not is a different issue. So +1 to remove it from the fist iteration, but -100 to tell me to remove it because it's only a convenience method for Nick or I. :) >>> >>> I was actually expecting something more like: >>> Portlet portlet = new Portlet(...); >>> Category category = new Category(...); >>> category.add(portlet); >>> contentRegistry.save(category); >> >> The less detached the API is, the better in my opinion. I would rather also have an API that is persistent by default, i.e. you don't need to do call save or update methods to persist the changes you made to "detached" entities. I understand, though, that it might be tricky to do so if we want to have large batches of modifications. I guess it boils down to whether we want the API to be mainly read-oriented or write-oriented or a mix of both. :) >> > > IMO having such state aware entities instead of POJOs will be opening another can of worms (to use your own words ;). Think about user persisting those objects somewhere, or have them flying between cluster instances and having some variation of infamous LazyInitializationException. Obviously it can be done and properly solved but will add unnecessary complexity. I think you misunderstood what I meant. The term detached refers to the situation you describe where you create objects, you deal with them then persist them (and you have to remember to do so). Users shouldn't need to persist objects elsewhere than in the GateIn "database". Maintaining consistency across cluster instances is a lot easier if all modifications to objects are persistent (i.e. the database is always up to date, though obviously pushing updates to the different instances is always a tricky issue). The persisting service(s) needed by the objects to save themselves should be injected via something like CDI. Even without having persistent method calls, I don't think that users should create new instances of objects directly and then save them. They should ask the API services for an instance and then act on it. This way, this instance is always managed by GateIn and we don't have completely detached objects. tl;dr: +1 on adding save and update methods if needed (though I would try to avoid them as much as possible), -1 on letting users create completely detached objects. >>> ************ >>> https://github.com/metacosm/gatein-api/blob/master/src/test/java/org/gate... >>> >>> getSingleResult in Hibernate isn't the best thing created, it actually >>> didn't make it through the JPA API. >>> I would rather remove this method from the API, and potentially add it >>> in some form later if really needed. >>> - Nodes.getSingleOrFail(Query.<Dashboard>builder().where(filter).build()); >> >> Yes, this is just syntactic sugar but an API is also a lot about syntactic sugar (making your users' life easier). However, it's probably a good idea to have a minimal version of the API first and then add the methods to smooth out the desire paths (http://en.wikipedia.org/wiki/Desire_path) once we've had some feedback on the API. > > Yes, +1 on keeping it minimal approach. We can always add more sophisticated methods to internal APIs > > Bolek Cordialement / Best, Chris == Principal Software Engineer / JBoss Enterprise Middleware Red Hat, Inc. Follow GateIn: http://blog.gatein.org / http://twitter.com/gatein Follow me: http://metacosm.info/metacosm / http://twitter.com/metacosm _______________________________________________ gatein-dev mailing list gatein-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/gatein-dev