DATAJDBC-219 Implement optimistic record locking

[tkvangorder]

@schauder This is my initial work on the optimistic record locking that incorporates the work Tom did originally. I wanted to get some eyes on this to make sure I am on the right track. I still need to add some more tests and this is very much a work in progress.

[tkvangorder]

I just ran into an issue with an immutable version attribute. The initial approach is setting the version in the DefaultDataAccess strategy and this may be too late. I can't find a good way to return the new entity (with its version set) from the data access strategy.

[tkvangorder]

While this current PR does implement the optimistic record locking pattern,
I am not at all happy with the approach.

The problem is that the version is being set in the data access strategy (as you would expect). However, there is no way to pass back the updated version and set that information in the DBAction. So, in the interpreter, I am inferring the version number using the same logic as the data access strategy....ugh. This breaks encapsulation (as I am really computing the version in two places)

There is also a problem in that if the version is immutable, then setting this by reference in the DBAction's entity is not going to be reflected when the updated aggregate root is returned from the template.

So, I wrote logic similar to that of the generated Id: grab the version from the entity in the DBEntity (although, I guess I could have just added the version directly to the actions) and then set the version using the root property accessor and the path from the DBAction.

I would love some feedback, I suspect this PR will need some pretty substantial changes.

[schauder]

Thank @tkvangorder for picking this up.

I don't think we should support version attributes on nested entities within an aggregate. I don't see how version attributes make sense in nested entities. Just as we have auditing only on the aggregate root level.

For setting the version I wonder if it makes more sense to pull he logic out of the DataAccessStrategy into the interpreter or into the action.

The responsibility of the DataAccessStrategy is the direct interaction with the database.
For id generation this means it also has to extract the id.
But the version is not created or incremented by the id.
So it does make sense to have it outside the DataAccessStrategy.
I think this also works well with alternative DataAccessStrategy which would with the current approach reimplement the version increment.

@mp911de I'd appreciate it if you could also take a look.

[tkvangorder]

@schauder Makes sense. If the interpreter sets the version, the DataAccessStrategy would still need to compute the previous version (or have the previous version passed in) to generate the version condition on the update. We can leave the contract "as is" and just document the contract on the interface.

[schauder]

It gets the entity plus a version. Since the entity contains a version we have both values available and now need to (re)compute it.

I'd say the entity should contain the original version so it only gets changed after the update happened successfully.

[tkvangorder]

@schauder Getting closer.

• Moved the version generation into the interpreter.
• Created updateWithVersion, and deleteWithVersion in the DataAccessStrategy.
• Enforcing optimistic versioning on only the roots. (which is why I created the two new methods..root actions can call the new methods, nested entities continue to call the methods without considering versions)
• Refactored updateWithVersion to pass in the nextVersion into the DataAccessStrategy
• Refactored the InsertRoot and UpdateRoot to include a version (similar to how generated IDs are handled)

I went back and forth on the need for an insertWithVersion but in the end, it just seemed strange. I ended up setting the initial version in the interpreter and then back it out on an error.

I still think there is more work to be done. After passing the next version on the update, it requires the data access strategy to correctly set that value when performing the update. This ends up being clunky and you have to do some work in each data access strategy to set the new version without modifying the entity by reference. See the MybatisDataAccessStrategy as an example of how this gets strange.

I am thinking maybe it would make more sense to set the new version on the entity in the interpreter and passing the old version as an additional argument into the update and then revert that change in the interpreter when an error occurs.

[tkvangorder]

@schauder What do you think? I Added a commit where I reworked things such that the previous version is passed into the update and the delete. It feels cleaner, in that the interpreter is doing all the heavy lifting.

[vuck3o]

Any news here? #askingforafriend

[schauder]

Not yet, it is still waiting for me to find some time. This will probably happen in October.

[schauder]

I Added a commit where I reworked things such that the previous version is passed into the update and the delete. It feels cleaner, in that the interpreter is doing all the heavy lifting.

I don't like manually undoing stuff and therefore would prefer passing in the new version and setting it in the entity only after the change went through in the interpreter.
Inside the interpreter the following would happen:

1. create new version
2. call access strategy with unchanged entity and new version
3. set new version

Or am I missing something?

[schauder]

We use static imports only in tests.

[tkvangorder]

OK, I will make that change.

[schauder]

This looks pretty good.

Unfortunately AggregateChange changed significantly lately /cough/ this morning /cough/ so we need a rebase.

[schauder]

I Added a commit where I reworked things such that the previous version is passed into the update and the delete. It feels cleaner, in that the interpreter is doing all the heavy lifting.

I don't like manually undoing stuff and therefore would prefer passing in the new version and setting it in the entity only after the change went through in the interpreter.
Inside the interpreter the following would happen:

1. create new version
2. call access strategy with unchanged entity and new version
3. set new version

Actually scratch that.
Just get rid of the undoing part.

Undoing won't work anyway since we then should also undo Id setting that might have happened in other entities and also do it when an exception is thrown during saving of other entities contained in the aggregate.

[tkvangorder]

You are right,

I guess trying to prevent a mutated version "on error" is a futile exercise given that any error within the larger transaction is likely to leave any mutable entities in an indeterminate state.

I will give this another crack when I free up some time (hopefully within the week)

[tkvangorder]

This looks pretty good.

Unfortunately AggregateChange changed significantly lately /cough/ this morning /cough/ so we need a rebase.

Tyler shakes a fist at you, Haha, no worries, wouldn't be the first time this has happened to me.

[tkvangorder]

@schauder I finally had some time over the holiday to give this tracker some attention. I have:

• Rebased off master.
• Removed the static imports for the version helper.
• Removed the logic that is backing out any changes to the version on an error.

[schauder]

Thanks, this is polished and merged