ecotoneframework / ecotone-dev

Ecotone Framework Development - This is Monorepo which contains all official public modules
https://docs.ecotone.tech
Other
37 stars 16 forks source link

Document Store / updateDocument / throws DocumentNotFound if the new document is identical to the existing document #37

Closed aheissenberger closed 2 years ago

aheissenberger commented 2 years ago

Ecotone/dbal version(s) affected: 1.32.2

Description

Updateing an existing document with updateDocument will throw Fatal error: Uncaught Ecotone\Messaging\Store\Document\DocumentNotFound: There is no document with id 1 in collection course to update. in... when content of new document is identical to existing document.

The error is thrown as the update return zero modified records in https://github.com/ecotoneframework/dbal/blob/938b8650cb1b1b4c6f0f0cdd36ce6072f128248a/src/DocumentStore/DbalDocumentStore.php#L82-L91

The problem is based on the fact, that SQL does not distinguish between finding no record for update and updating an existing record with identical content. The current error handling tries to fetch the case when a record which does not exist is tried to update but also covers the case that the record exists and the content ist identical.

How to reproduce

INSERT  INTO ecotone_document_store (collection,document_id,document_type,document) VALUES ('wallet','1','array','["balance":1]')
UPDATE ecotone_document_store SET document_type = 'array', document = '["balance":1]' WHERE document_id = '1' AND collection = 'wallet';
SELECT row_count();

The result of row_count() is 0 which lead to throw the document not found error.

Possible Solution

I suggest to extend the database structure with an additional field updated_at with a high precision date field.

ALTER TABLE `ecotone_document_store` ADD `updated_at` DATETIME(6)  NULL  AFTER `document`;

extend the INSERT and UPDATE to include the current timestamp NOW(6) - (6) includes microseconds!

example text with fix:

INSERT  INTO ecotone_document_store (collection,document_id,document_type,document,updated_at) VALUES ('wallet','1','array','["balance":1]',NOW(6));
UPDATE ecotone_document_store SET document_type = 'array', document = '["balance":1]',updated_at=NOW(6) WHERE document_id = '1' AND collection = 'wallet';
SELECT row_count();
UPDATE ecotone_document_store SET document_type = 'array', document = '["balance":1]',updated_at=NOW(6) WHERE document_id = '1' AND collection = 'wallet';
SELECT row_count() 

Context

The problem happened when I use the document store in a projection.

aheissenberger commented 2 years ago

I am not a friend of DBAL ;-) the api blocks many SQL commands - e.g.:

I suggest to use hrtime(true) and FLOAT(53).

aheissenberger commented 2 years ago

This is a breaking change - existing document store will fail to update :-( @dgafka can you add this with an config option or is there any other migration path for existing document stores?

dgafka commented 2 years ago

Hey @aheissenberger Please reopen the PR in Ecotone-dev :)

I like your solution, let's go for it. I think we may have no breaking change in here, as dbal will ignore field that have no corresponding columns. Can you confirm that?

aheissenberger commented 2 years ago

No I cannot confirm, in my test I broke my application as the inserts and updates fail if the database table exists without the new column and the insert tries to create a row with the timestamp :-(

Possible solutions:

  1. upgrade the package to a new major version and communicate in the package changes
  2. ad a config ->withDocumentStore(tableFormatVersion: 2); for the document store which needs to be set to create & use the new table format but will be switched to 2 by default on the next new major package version
  3. ->withDocumentStore(initializeDatabaseTable: true);: extend the existing code which checks on every start which table format exists and disables writing to the new columns

I prefer 1. or 2. as they do not add extra checking code to a running system

dgafka commented 2 years ago

@aheissenberger all good can we close?