search

prooph / pdo-event-store

PDO implementation of ProophEventStore http://getprooph.org
BSD 3-Clause "New" or "Revised" License
111 stars 56 forks source link

Prevents using incompatible persistence strategy #217

Closed gquemener closed 4 years ago

gquemener commented 4 years ago

Currently, configuring a MariaDB PersistenceStrategy within a PostgresEventStore will not trigger PHP type check. The result will most likely be the db server complaining about a wrong syntax (in the best case).

This PR prevents mixing persistence strategies and event store implementations.

Note: Not sure if this is compatible with php <7.4

prolic commented 4 years ago

Nice, that's a good improvement, a lot of people made exactly that mistake.

On Mon, Dec 9, 2019, 15:16 Gildas Quéméner notifications@github.com wrote:

Not sure if this is compatible with php <7.4

You can view, comment on, or merge this pull request online at:

https://github.com/prooph/pdo-event-store/pull/217 Commit Summary

  • Prevents using incompatible persistence strategy

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/prooph/pdo-event-store/pull/217?email_source=notifications&email_token=AADAJPERWHVVB2N56YU5A7DQX2DOXA5CNFSM4JYOPUT2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4H7E67WQ, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADAJPCQJC53CYT4TDDBRQLQX2DOXANCNFSM4JYOPUTQ .

fritz-gerneth commented 4 years ago

Note: Not sure if this is compatible with php <7.4

From a quick glance I have not seen anything incompatible. Any specific concerns?

gquemener commented 4 years ago

Note: Not sure if this is compatible with php <7.4

From a quick glance I have not seen anything incompatible. Any specific concerns?

I was thinking about that https://www.php.net/manual/en/migration74.new-features.php#migration74.new-features.core.type-variance, but it's not related to the content of this PR.

coveralls commented 4 years ago

Coverage Status

Coverage remained the same at 83.984% when pulling 2b2f943b717487a2a045a168186246102c787b86 on gquemener:prevent-persistence-strategy-mix-up into 9c8b5b7cf5194d3e616f38cb58497735925b353b on prooph:master.

fritz-gerneth commented 4 years ago

Since this has not been released yet, I only noticed this now while testing #221 : The constructor signature change from the generic PersistenceStrategy to a vendor specific interface itself is a BC break for those using custom persistence strategies (like us :)). Not a big deal and easy to be fixed, but something that should be announced respectively.

prolic commented 4 years ago

Constructor is not part of the interface

On Wed, Feb 26, 2020, 13:12 Fritz Gerneth notifications@github.com wrote:

Since this has not been released yet, I only noticed this now while testing #221 https://github.com/prooph/pdo-event-store/pull/221 : The constructor signature change from the generic PersistenceStrategy to a vendor specific interface itself is a BC break for those using custom persistence strategies (like us :)). Not a big deal and easy to be fixed, but something that should be announced respectively.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/prooph/pdo-event-store/pull/217?email_source=notifications&email_token=AADAJPCB3WMKWGL2JUTW74DRE2IGNA5CNFSM4JYOPUT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENA3KCY#issuecomment-591508747, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADAJPA24CQQ2OHY4RIBHOTRE2IGNANCNFSM4JYOPUTQ .

gquemener commented 4 years ago

Constructor is not part of the interface

Agreed. However could you be more specific and point to some code (or add a snippet to your comment), please @fritz-gerneth ? I'd like to fully understand what's the issue here.

In case you missed it, all the 3 new interfaces extends the existing PersistenceStrategy interface, which will allow you to keep the typehint of your dependenc(ies) with this one, thus preventing a BC break.

EDIT: Ok, I see what's the issue here. Sorry, I went to fast in writing.

As the event store implementation constructors have changed, we indeed have introduced a BC break. As a matter of fact, as stated by @fritz-gerneth, user will need to modify their custom persistence strategy implementation (by implementing the correct interface) in order to successfully inject them in the PDO event store implementations:

- class MyCustomMySqlPersistenceStrategy implements PersistenceStrategy
+ class MyCustomMySqlPersistenceStrategy implements MySqlPersistenceStrategy
{
}

A backward compatible manner would have been to keep the PersistenceStrategy typehint and check type in the ctor or every time the dependency is required:

final class MySqlEventStore implements PdoEventStore
{
    public function create(Stream $stream): void
    {
        $streamName = $stream->streamName();

        $this->addStreamToStreamsTable($stream);

        try {
+           if (
+               !$this->persistenceStrategy instanceof MySqlPersistenceStrategy
+               ||
+               (
+                   !$this->persistenceStrategy instanceof MySqlSingleStreamStrategy
+                   && !$this->persistenceStrategy instanceof MySqlSimpleStreamStrategy
+                   && !$this->persistenceStrategy instanceof MySqlAggregateStreamStrategy
+               )
+           ) {
+               throw new \InvalidArgumentException(...);
+           }
            $tableName = $this->persistenceStrategy->generateTableName($streamName);
            $this->createSchemaFor($tableName);
        } catch (RuntimeException $exception) {
...

Quite tedious... I'm not sure what's the way to go here :thinking: Semver would enforce releasing a new major version if we keep the current implementation, wouldn't it?

fritz-gerneth commented 4 years ago

Yes, that's the issue I found myself in. Not exactly sure how many people use a custom strategy and the change is very small. Strictly folllowing semver would require a new major version indeed. Personally both approaches (keeping, doing the check in the constructor) are fine. My initial concern only was that this should not go unmentioned and at the least be mentioned in the release notes somewhere :)

codeliner commented 4 years ago

Puh, I missed the discussion (hadn't much time for OSS end of last year). I was pointed to the BC break introduced in this PR today. event-store v7 is downloaded 400 times / day.

I really don't like to publish any BC breaks even not small ones, when a library is used heavily in production environments (which is the case for prooph event store v7).

We should rollback the changes. They are a good thing to prevent stupid use of persistence strategies. That's fine. But only a major release would allow us to make life better for new users. At the moment we should focus more on existing users, because we have a lot!

codeliner commented 4 years ago

What we could do is, we start working on 2.0, but gap detection needs to be included in 1.x

My suggestion:

gquemener commented 4 years ago

PR to rollback changes

I can take care of it. WDYT of introducing the interfaces in a BC manner (aka reverting only the event stores ctor signature and adding extra type check)?

gquemener commented 4 years ago

Maybe even trigger a deprecate error when a non vendor specific persistence strategy is injected :thinking:

codeliner commented 4 years ago

Yes, deprecation error please, but not an exception, otherwise you still have the problem that your custom strategy will no longer work without implementing the new interface

codeliner commented 4 years ago

and thank you @gquemener for the PR and taking care of it :+1:

prolic commented 4 years ago

Looks all good to me, only one thing: I'm a little hesitant starting a 2.0 branch and release, when all we have as changes is this persistence strategy thing. For a new major release I would expect a little more than only "cosmetic" changes.

prolic commented 4 years ago

Constructor is not part of the interface

I was writing from phone and didn't realize this in indeed a BC. Sorry for the confusion.

fritz-gerneth commented 4 years ago

Deprecation error is a good idea 👍

codeliner commented 4 years ago

@prolic release fast, release often :D

but yeah, we don't need to release 2.0 soon. The added deprecation warning should already be a good hint for those who use persistence strategy wrong.

But maybe we already have some more topics to tackle in a 2.0 like gap detection enabled by default along with a rewrite of the projections to decrease constructor arguments and better share functionality between query, projection and read model projection.

I'd also like to work on Postgres Notify Support. Not sure if this can be added without BC break to 1.x

prolic commented 4 years ago

Whoever wants to work on those topics, feel free to do so. I can do code reviews but won't have much time coding this.

I'll prepare 1.12 release this weekend.

@codeliner anything else you need to do before this can get merged?

On Fri, Feb 28, 2020, 14:44 Alexander Miertsch notifications@github.com wrote:

@prolic https://github.com/prolic release fast, release often :D

but yeah, we don't need to release 2.0 soon. The added deprecation warning should already be a good hint for those who use persistence strategy wrong.

But maybe we already have some more topics to tackle in a 2.0 like gap detection enabled by default along with a rewrite of the projections to decrease constructor arguments and better share functionality between query, projection and read model projection.

I'd also like to work on Postgres Notify Support. Not sure if this can be added without BC break to 1.x

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/prooph/pdo-event-store/pull/217?email_source=notifications&email_token=AADAJPGQ5XYVONKBXYJJ74TRFFERTA5CNFSM4JYOPUT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENJQ7WY#issuecomment-592646107, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADAJPGJMD64ZBXPGTB2GWLRFFERTANCNFSM4JYOPUTQ .