Let me first say that I really appreciate the depth of thought that you have put into this, and I'm still mulling over much of it. You are right that there are some awkward evolved patterns here; transactionless() is a kludge and the behavior of Ref could use some additional thought.

I'm afraid I'm probably still going to disappoint you.

The behavior of ofy() is intentional. Persistence context is an aspect that follows a thread of execution, not object state that you pass around. The designers of JPA/EJB modeled this as a proxy (EntityManager) which always represents the appropriate context; sure you can inject it or pass it as a parameter but it's just a proxy for the current thread-local. For various reasons I decided to cut out the proxy and use a static method instead but the idea is the same; users do not manage persistence context, it's just always available.

This API you wish to create looks a lot like the API from Objectify v3. That API created some very difficult patterns in several applications that I worked on (both mine and other people's). The problem is when you have bits of code like this:

public void buyGame(User user, Game game) {
    ofy().transact(...);
}

public void signupForTournament(User user, Game game) throws AlreadyFullException {
    ofy().transact(...);
}

public void buyAndSignup(User user, Game game) throws AlreadyFullException {
   ofy().transact(() -> {
        buyGame(user, game);
        enrollInTournament(user, game);
    });
}

You want to be able to aggregate bits of independent useful behavior, each of which interact with a persistence context but have no knowledge of each other. In your model, you would have to pass a specific Objectify instance as a parameter to every method that needs to work in the same persistence context. I have been trying to draw as much of the "good parts" of EJB into Objectify, and persistence-as-aspect (accessed via the ofy() pattern) has worked out pretty well in practice. Or at least, it works much better than the v3 approach did.

That's not to say that a lot of improvements couldn't be made; I'm not particularly pleased by the way the stack is being maintained right now (it probably belongs in ObjectifyFactory). I'm about to embark on a significant rewrite of Objectify for v6 and I have a laundry list of issues I'd like to address. Fixing transactionless() is already one of them, and I'm certainly open to discussion of more.

If you would like to continue discussion, we can either continue here or move it to the objectify google group where other people may like to participate.

Another approach that might work is that if you wish to make significant changes to the Objectify API that I dislike (eg, options builders instead of functional command objects), create a wrapper as a separate project. I would be totally happy to link to it! Not everyone shares my aesthetic sensibilities, that's fine.

Jeff, thanks for the detailed response! I’ll try to address some points here and will look to kick off separate discussions on the google group.

Reading through https://github.com/objectify/objectify/wiki/UpgradeVersion3ToVersion4 I see how some of the stuff on this pull request looks like going back to v3. A few thoughts though:

The ofy() persistence-as-aspect pattern can be the recommended to use pattern with objectify. However, it need not preclude one’s ability to use a proxy object pattern, if that’s what the user wants/needs. Ignoring what the title of the pull request says, my changes enable both patterns and I think it would be reasonable for v6 to support both patterns as well.

My issue with per-command configuration (as opposed to per instance with ObjectifyOpt) has perhaps more to do with the current implementation rather than the concept. With the way ObjectifyImpl is cloned and state is shared, it’s easy to have bugs (e.g. see testChangeOfyParameterInsideTransactionHasNoEffect()). I’m particularly bothered by the fact that transactionless(), cache() and the likes return a fork/clone of ObjectifyImpl. I’d think that the intention of the ofy() pattern is to never save off the result of e.g. ofy().cache(false) for later use, however, the current implementation allows such use. In the spirit of transact(work), I think it makes perfect sense to scope the configuration to a unit of work. In other words:

ofy().cache(false, () -> ofy().load().key(k).now());
ofy().transactionless(() -> ofy().load().key(k).now());

This transact() style api also eliminates the need to create ObjectifyImpl clones, which makes it much easier to write Objectify wrappers (composition over inheritance). I think it also does make sense for options to be part of the transaction context, so each time options are changed, a new context (Transactor) is pushed on to the stack.

To take a step back, within my project I don’t use ObjectifyService and ofy(). I had created an ObjectifyStack wrapper that maintains a stack of ObjectifyImpl instances (so the proxy pattern). I didn’t implement the full Objectify api, only what I needed (transact and transactionless), with the other methods throwing. I mostly didn’t want to deal with methods that return ObjectifyImpl clones. (I imagine these are the same kind of issue that prompted you to drop the Wrapper classes in v4.) It was all working great until I tried using Ref and @Load. Because I don’t use ofy()/ObjectifyService, my refs cannot be loaded in all contexts. To get it working, I had to start digging into Objectify. With my changes to Objectify I can get rid of my ObjectifyStack wrapper and my Ref & @Load uses work. I still don’t use ofy() but it’s available and would return the (now) proxy ObjectifyImpl.

A problem I still haven’t solved is permissions aware datastore access for Ref loading. I currently have a permissions-aware datastore abstraction (similar api to objectify actually, but no forks) that wraps objectify. Most datastore operations go through the permissions layer but few bypass it. When I read a Ref via @Load or via .get(), I’d want to make sure that these reads are subject to the same permission model as the original read. The only way I can see that working at the moment is by extending ObjectifyImpl and implementing permissions checking there… but I’d much rather implement is as a wrapper (again composition over inheritance). Which reminds me of another observation. The fluent loader/saver/etc. interfaces are indeed very easy to use but the way they trigger the raw datastore operations makes it difficult to intercept (code is scattered).

In terms of practical next steps, I can be using this project fork for my current needs and I’d like to be involved in the v6 discussions, maybe even contribute some code. I’d want to ultimately be running on stock Objectify v6. Potential topics I could kick off in the google group in the coming days/weeks:

• Support both persistence-as-aspect and proxy patterns.
• API changes: ofy().transactionless(Work work) and ofy().option(Work work), i.e. don’t clone ObjectifyImpl, allow composition over inheritance
• How to “permanently” associate Ref to Objectify instance responsible for loading/session-caching it.
• Should it be allowed for transaction context to be different between initialization and execution of Loader, Deferer, Deleter, Saver be bound?
• Datastore permissions model wrapper or plugin for objectify.

Lastly, do you have any timeline for v6 development and release? Is your laundry list of things posted publicly?

I've been keeping this stuff in the back of my mind as I work on v6. I've committed quite a bit of refactoring to the "v6" branch, some of which you may like and some of which you probably will not.

The problem with ofy().cache(false, () -> ofy().load().key(k).now()); is that it produces a pretty clunky API :-( Tweaking the cache or consistency settings is a quick escape, and often you'll want to do more than one (ie cache(false).consistency(Consistency.EVENTUAL). Closures would be pretty painful.

However, I've added transactionless(Work) and deprecated transactionless(). Using a closure here is much more natural, since we already use closures to enter transaction scopes. And the behavior of transactionless() in a world of Ref<>s is a mess.

From a bigger picture, I believe there are two decent ways of trying to "weave" behavior like authorization into the persistence layer: 1) implementing a DAO layer on top, hiding most direct objectify calls or 2) building an interceptor layer deeper inside Objectify. Trying to implement #2 at the API layer is pretty painful because the API is broad. Simpler would be to be able to register some sort of arbitrary processor that examines entities as they are loaded (or saved). There are fewer places this happens (eg, WriteEngine).

BTW permanently associating a Ref with an Objectify instance is not really possible because Refs are serializable. LiveRef already tries to do this and it sucks. I've been thinking of just adding a get() method to Key (which simply calls ObjectifyService.ofy().load().key()) and deprecating Ref entirely. Or maybe strip down Ref to just this get() method.

Jeff, thanks for following up and sorry I haven't yet followed-up with posts on the google group. We are gradually addressing the issues on this thread though.

I checked out what you're doing on the v6 branch and love what I'm seeing there! No more cloning, transactionless(Work) and stack refactoring are all great!

I actually don't use cache() and the likes and had already transitioned to transactionless(Work), so I could easily implement my own proxy object like this:

public class MyObjectifyFactory extends ObjectifyFactory {
	private class ObjectifyStack implements Objectify {
		...
		@Override
		public Objectify transactionless() {
			throw new IllegalStateException("Not supported!");
		}

		@Override
		public <R> R transactionless(Work<R> work) {
			return ofy().transactionless(work);
		}
		...
	}

	private ObjectifyStack ofyStack = new ObjectifyStack();
	public Objectify ofyStack() {
  		return ofyStack;
	}

And would then save off the return value of myFactory.ofyStack() in my DataStore abstraction... and I could almost use the v6 branch without patches on top.

As for authorization layer, I ended up doing both 1) and 2). For 1) I have my own DataStore and DataStoreLoader interfaces, the implementations of which wrap access to Objectify. I don't wrap Saver, Deleter and instead have several overloaded methods save() and delete(). That used to work just fine until I started using Refs. For Refs I need to implement 2) - when a Ref is loaded, I hacked the Objectify implementation to call a callback that I configure via ObjectifyOptions (I'd have to port this to the v6 branch). It's very quick and dirty at the moment, I just ensure that that the kind of the key is allowed and I only allow kinds that are readable by everyone. It does the job now given my current simplistic use of Refs. It would be great if Objectify could be configured (e.g. via ObjectifyOptions) with Read/Save/Delete Listener interfaces that get called before a batch or operations is executed. It has to be before as we don't want a write to go through before the permission check runs. It has to be a batch to allow for a more efficient execution of the permission checks (e.g. in some cases I need to do additional datafetches). I'd very much like to see a good implementation of 2) and would move my permission checking logic there.

Regarding Refs, here's how I patched them to get them working reasonably well for me (aside from permission checking). I removed the association with a Objectify instance, so they cannot ever be implicitly loaded anymore. I didn't like the performance implications of that implicit loading. If in one context a Ref field is loaded via @Load and in another context it is not (e.g. inside out outside of a transaction), processing a list of things with non-loaded Refs will result in serial datafetches. And the only way to notice that behavior is to look at traces on prod. If a Ref is not loaded, it'd just throw on get(). If that happens, I need to make a conscious decision how best to load the refs - one by one ot in a multi-fetch. When the LiveRef is serialized to GWT, the value will only be serialized to the client if loaded. When serializing from GWT, the Ref won't be loaded on the server side, so an explicit load is required ofy().load().ref(ref), which sets the value of the LiveRef. This behavior is less auto-magical and more predictable/easy to reason about. So in summary, I'd like to keep Refs as @Load is extremely useful. Also, Refs are especially useful with GWT, making it very easy to make dependencies available in client code without much boilerplate and DTOs. I'd suggest you to just drop the automagical loading, make it explicit via @Load or via ofy().load().ref(ref). Lastly, offer a clean and efficient way to implement permission checks on Ref loading, or all datastore operations for that matter.

The other change I'd need to make on top of the v6 branch is to make ObjectifyFactory.open() public. Your intention is to keep these methods part of the implementation, however, I'm not using ObjectifyService and have my own "service" and Filter that initializes it. The reason is that I want to ensure that all datastore access goes throgh my Objectify wrapper where I implements the permission checks. I don't want stray ofy() code to automagically work, so I cannot ever initialize the standard ObjectifyFactory... not for the time being anyway.

Lastly, I agree that using closures for cache() and the likes, when chaining the calls is rather clunky. How about this:

try (ScopedObjectifyOptions opt = ofy().cache(false).consistency(Consistency.EVENTUAL)) {
  ofy().load().key(k).now();
  // etc.
}

ScopedObjectifyOptions would implement Closeable but not Objectify, the old syntax won't compile, which would make porting easier. Someone can shoot themselves in the foot if they don't use try-with-resources but then the assertion in ObjectifyFactory.pop(ObjectifyImpl) will catch it (btw, make it throw new IllegalStateException instead of assert). Still more verbose what what you have now but I think the semantics would be cleaner. Furthermore, if transaction context and options are saved on the stack, then ObjectifyFactory.ofy() could just return the proxy ObectifyStack object. If permission checks are implemented as listeners configured on ObjectifyOptions, I'd like to very well scope of what reads/writes happen with and without permission checks, so either a closure or try-with-resources notation would be needed (closure probably being preferable).

In case you'd like to have a look, here is how I patched the v6 branch to get it to work with my project:
https://github.com/PeterDimov/objectify/commits/v6-patched

The Ref changes are quite hacky but if you agree with the proposed explicit load semantics, I'm sure you'll find a cleaner way to work it in.