Supported Projection identifier names

albe commented 7 years ago

Currently, if a Projection Acme\Foo\Bar\Domain\Projection\Baz\BazProjector is referenced (for replaying in CLI), either of these writings is possible:

acme.foo.bar:baz foo.bar:baz bar:baz baz

Supporting all those different writings adds a lot of very complicated and hard to follow code (see https://github.com/neos/Neos.Cqrs/pull/54/files#diff-f97f8582f39d113750d81d5962d41460R158 ff.), while the benefit of being able to write any of those is debatable. If your Projection name is unique, being able to use only baz is the optimal shorthand. If however you have an ambiguous Projection name, then you can simply type the fully qualified name, which still takes less time than possibly attempting three different writings. Also, it makes documentation more complicated, because all those cases need to be explained.

I therefore suggest that we only support the fully qualified projection name, and the shortest name version. This would highly reduce code complexity in the above mentioned class.

robertlemke commented 7 years ago

Actually, I'm undecided regarding this feature.

What would be completely fine with me as well would be using the fully qualified class name. Why not … I can copy that from the output of projection:list equally easy as the identifier (which I also need to look up).

But I'm also not against keeping this. Sure, why not. The code complexity … well, it's there, it works and we won't need to really touch it again.

bwaidelich commented 7 years ago

It's exactly the same logic we use to find the shortest unambiguous CLI command identifier: While it makes sense to use the full identifier for cron jobs & deployment tasks it's very convenient to be able to use the short command on the CLI and I would be extremely unhappy if I had to type the full command every time. Same with

flow projection:replay some.long.packagekey:projection

If you think it's a problem that this internal code is in the ProjectionManager we could extract the logic to the CLI command controller but I'll fight for that UX ;)

robertlemke commented 7 years ago

yes, all fine, it's very convenient during development. But I would argue that it's too risky to use the short identifier in cron-jobs or automated tasks, and that's what some people will do …

bwaidelich commented 7 years ago

But I would argue that it's too risky to use the short identifier in cron-jobs or automated tasks, and that's what some people will do

But that's exactly the same for the command identifiers!? So should we get rid of that feature altogether?

albe commented 7 years ago

I would be extremely unhappy if I had to type the full command every time.

Just to restate it: You don't have to every time - as long as your Projection short names are unique, all is fine. flow projection:replay projection

But if you have multiple projection, why do you want to attempt to write flow projection:replay packagekey:projection if that again could fail? You lose more time if it does, and we carry more unmaintainable code with us to support this border-case.

I see two use cases:

during development: you should know exactly if your projection name is ambiguous or not, and hence can optimize typing to choose shortest or fully qualified name (if I know that a projection name is duplicated I mostly don't know where exactly that other projection lives, so I couldn't be sure any of the semi-qualified names is safe)
during deployment: you should always use fully qualified names, anything else is madness

robertlemke commented 7 years ago

As @albe wrote, we have these two use cases and I think both are important. I would just leave the feature implemented like it is, but people automating tasks should be (made) aware of the fact that they should rather use full identifiers for automation (like for commands).

bwaidelich commented 7 years ago

OK, let me add one more scenario anyways: Imagine you work on a large Flow application that covers multiple bounded contexts (with at least some of which are event-sourced). Because you're a well-behaved developer you split it into multiple packages, for example: YourCompany.SuprApp (<-- Application logic) YourCompany.SuprApp.Crm YourCompany.SuprApp.Billing YourCompany.SuprApp.Community YourCompany.SuprApp.Support ... If the app grows the likelihood that you have overlapping aggregate/read-model names (and thus projection names) increases (e.g. in the above you most probably have multiple "user" or "customer" models). During development I want to be able to type

flow projection:replay crm:users

or

flow projection:replay community:users

albe commented 7 years ago

Ok, but you don't want to be able to type

flow projection:replay SuprApp.Crm:users

too, do you? So still the solution is over-engineered (in a bad way).

But of course, if you both say that the code is good enough as is, I won't break a war on this.

we could extract the logic to the CLI command controller

This could be a compromise to at least keep the infastructural code clean from this usability-induced mess.

bwaidelich commented 7 years ago

if you both say that the code is good enough as is

I don't think it's pretty but it works and has a small footprint. But I'm more than happy if someone comes up with a cleaner version that achieves the same!

we could extract the logic to the CLI command controller

This could be a compromise to at least keep the infastructural code clean from this usability-induced mess.

Yes, I agree. We decided against this in the Flow CommandManager but I can't think of another place where this would be needed. I'll take care.

bwaidelich commented 7 years ago

I moved the logic into the command controller as discussed

bwaidelich commented 7 years ago

With #100 btw

neos / Neos.EventSourcing

Supported Projection identifier names #79