Mariadb eventstore returning "null" aggregate on new mariadb server 10.2.11
hvanoch opened this issue · 16 comments
We upgrade our mariadb server from 10.2.10 to 10.2.11.
This causes aggregates to not be loaded anymore.
I figured out that is has something to do with "json_value" previously not being escaped, but now in the newer version, it is.
See:
https://mariadb.com/kb/en/library/mariadb-10211-changelog/ (JSON_VALUE doesn't escape quote.)
https://jira.mariadb.org/browse/MDEV-14402
Removing the "addslashes" here fixes it. But then it is not backward compatible with older mariadb versions.
Maybe you can write a migration for you data in event stores and add escaping for existing events.
@adapik That will not work, because then new aggregates will still not be loaded anymore.
This has to be tackled in prooph event store some how.
What do you think of bumping the mariadb requirements? @codeliner
Another possibility is to add just another flag to the constructor / factory, where the user can notify which specific mariadb version he's using. I really hate this kind of stuff, when a db vendor just changes it's behaviour so tools can't rely on the feature-set.
same here, we now face the same problems like the Doctrine team :( Something that is really time consuming. Well, the only way without breaking others installation would be a new major version of pdo-event-store with a fix and minimum mariaDB version set to 10.2.11
If they break again, we drop mariaDB support!
If we drop mariaDB support, how about to add a doctrine-event-store? For most users pdo-event-store is the better implementation. But there are some users out there which uses doctrine for her projections anyway. So we can answer the question "how can I use the event store with Maria db" by simply recommend the doctrine-event-store.
I really don't like to use doctrine for such a simple task. But I also don't like to use symphony .. and hey, we have a bundle. ;)
Also, I don't like our implementation in the current way. Instead of a single pdo-event-store, we have a mostly same implementation for each database engine and furthermore strategy implementations for each of them. Maybe we can refactor the whole thing to have a more generic implementation which uses pdo and only use the strategies for special engine cases.
@oqq There will be NO doctrine-event-store any more, with reasons. Different vendors have different implementations for json support (and doctrine doesn't support them). Furthermore batch inserts are not possible with doctrine either. I would love to have doctrine handle all this complexity for us, but it doesn't.
The Doctrine dbal has of course a json type since 2.6.0 and uses the engine type were possible. If not, than it would use some fallback. So that should not be a reason anymore.
Also PDO has not something like "batch inserts". We build it around it, what is possible with doctrine too.
Doctrine would not be such performant as a pdo implementation and we should never prefer the doctrine-event-store for the pdo-event-store. But there are reasons why users would use it (e.g. we don't provide an own pdo implementation but doctrine does).
@oqq Doctrine doesn't support json functions. This is only possible with another extension, like: https://github.com/SyslogicNL/DoctrineJsonFunctions - but even there is no mariadb support.
Batch inserts are not support by doctrine dbal, and I don't want to put efforts into doctrine to provide this feature atm. With PDO you can do stuff like this: INSERT INTO table (column1, column2) VALUES (1,2), (2, 3), (4,5); so three entries are inserted at once. This is NOT possible with doctrine atm (see open issue here: doctrine/dbal#682).
We had an doctrine-event-store-adapter in event-store v6 and dropped it for 7.0 in favour of pdo-event-store. I will NOT go back. This decision is made. We support DB vendors, not dbal implementation. There will also be no zend-db-event-store, eloquent-event-store, whatever. It's not worth it. You can create an oqq/doctrine-event-store and maintain it yourself though.
@hvanoch sorry I was quite busy these days. Will try to fix this problem coming weekend.
I think the best way is to add a flag to the constructor indicating whether are not we need to add those slashes.
The only other option we have is bump mariadb requirement which will BC break apps running a fixed mariadb version and unable to upgrade right away.
Wanna provide a PR?
ok fine..
How about my second proposal? We should add some more capabilities to the strategies, make a generic pdo event store and remove all the other classes. Of course in a next major version. Or maybe there is a non bc break way. Our implementations mostly differ on how to escape values or more how to build queries.
And please don't use something like "add a flag to the constructor". That should never be a solution, since this would add a whole test branch for that class by simply switching between true and false. I would neither add such "flag" for a method. In a class constructor that is .. really bad. ;)
@oqq we can consider adding capabilities to strategies for next major release. About adding flag to constructor... what's your opinion on this issue? Bump mariadb requirements instead?
This was a bug from mariaDB and we have tried to circumvent this bug. I would simply remove our "fix" and inform in readme about the bug in mariadb. If someone has a fixed reason to use that buggy Maria version, than he has to use also the fixed pdo-event-store version or clone the class and patch it again.
How @codeliner already say it, that would be a never ending story if we always try to fix such type of database engine "failures".
👍 for a hint in README and changelog and just removing our workaround. the hint can include the reasons and mention the pdo-event-store version that you need to pin to continue using the buggy mariaDB version.