Concurrency check does not seem present

[niwrA]

I looked at this project for the first time during the Eindhoven Hackathon yesterday (which was great), and I like this project a lot.

I did notice one thing that often slips through in projects though but can be tricky, and that is concurrency checking. Perhaps I overlooked it, because one day isn't that much time to look through how a project is setup, between also talking with a lot of peers about this project and various tech, but there does not seem to be any.

The scenario to replicate this problem, should it exist, is simple. Two people look at the same task. One makes a change to, say, the name, and the other adds a required skill. Since the current setup updates the entire task, whomever presses save the last will overwrite the other's changes, and nobody will know.

EntityFramework has a simple quick concurrency check that can be added. Simply adding a Timestamp property will tell it by convention that a concurrency check should take place. It will then validate whether the Timestamp of the update that comes in mismatches the timestamp in the database, and hence know it has changed. I'm not sure if anything changed in that respect for EF7, but I presume it will work the same.

That would be the quick fix that I would recommend implementing before go-live.

A perhaps longer term fix that I'm personally fond of in CQRS scenarios is making the commands more fine-grained. If you add a skill to task, then generate a command that only does that. If you change the name, only generate a command that changes that. This avoids most concurrency issues directly and allows people to work together more concurrently as a result, which can be good when things need to move fast especially, but also makes for better quality data. And with relational data in particular, it saves you from having to retrieve say all the skills to check if something should be added or removed. Someone command to add skill x, and remove skill y, and that's all you need to do.

Of course, you can still also pass the old value in the command for maximum concurrency checking. If the old value does not match the existing value, someone has changed it in the meantime.

As an additional bonus, if you event source these fine-grained commands, you can also have a detailed change history of any entity.

[dpaquette]

You are correct that we do not do any concurrency checking. For reference, here is the documentation for concurrency tokens in EF Core: https://docs.microsoft.com/en-us/ef/core/modeling/concurrency

[mgmccarthy]

@niwrA, some high-level comments on this:

concurrency concerns

concurrency concerns out of context are not the best way to address concurrency: For example, you used an example above saying two people are editing a task at the same time. The real question we should be asking is "what is the likelihood of two people editing a Task at the same time?" Is this an admin page? How many admins do we expect? What will the true roles of these "admins" be?

Looking at concurrency in the context of a specific Use Case/Issue/UI can start to help us identify where concurrency COULD be an issue, and figure out how we want to address it. Addressing it might not always fall to a technical solution like using a Timestamp property on our entities. It could be addressed by re-designing the use case/UI where the issue is likely and/or coming up with compensating actions to handle the concurrency problem that is a functional solution using a workflow.

CQRS

I agree with you in everything you said/mentioned about CQRS, and aiming to make our commands more fine-grained. I am a daily user of NServiceBus and I do see how AllReady has problems with the scope of its commands/handlers especially when looking at our db design and some of our larger super-aggregates.

Keep in mind that we want to keep the bar to contributing accessible to most .NET devs who are willing the contribute their time. All devs might not understand the intricacies of bounded context, sub-contexts, and breaking supper aggregates apart into smaller pieces that are maintained using events across boundaries. We want to allow everyone to contribute, so using MediatR and our approach to CQRS is a way to keep it accessible to most devs so we can build a strong network on contributors (which has been happening since I've joined the project).

Also, since MediatR is not really a true enterprise service bus like NServiceBus, it has its short-comings. It's asynchronous, it is not durable, it does not contain functionality/policies for things like built-in retries and a framework/UI for handling failed messages. So even if we did move to boundary isolation and fine grained commands (which could help the code base), we're only going to get "so far" with a framework like MediatR.

Event Sourcing

event sourcing is even more of a departure from "mainstream" .NET development and would requires vigorous code reviews and a very well put together framework/back-end that would need to be vetted/tested. Again, looking to keep the contirbution bar accesssible to most .NET devs, and keeping in mind we're all volunteers here, event sourcing is something I don't think would really pay off for this project. I would rather move to true boundary isolation/bounded context, etc... first before introducing event sourcing.

Thanks so much for YOUR contribution over the codeathon to AllReady and for writing up your thoughts in this issue.

[EmilyLuijbregts]

Hey @niwrA Thanks again for taking the time to write up your thoughts. Have you had a chance to look at the reply from Michael? Do you have any further questions or comments?

[mgmccarthy]

@tonysurma / @EmilyLuijbregts can we please close this Issue?

[niwrA]

Hi,

I would say that regardless of context, there is currently nothing done to prevent concurrency issues from taking place. There is also not a functional analysis showing that concurrency issues are not likely to happen. In that context, and until a better solution has been provisioned, I would suggest implementing the basic timestamp solution as a low-cost security measure, until something better comes along.

The rest is far less important, and could be dealt with in a different issue. Eventsourcing by the way doesn't have to mean that the whole logic is changed, just that the messages are stored and can at some point be replayed or reviewed. Everything can build from there.

Kind regards, Arwin