3:35 a.m.
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").
> 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
> 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.
> ************
>
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