propelorm / Propel2

Propel2 is an open-source high-performance Object-Relational Mapping (ORM) for modern PHP
http://propelorm.org/
MIT License
1.26k stars 393 forks source link

Propel <behavior name="versionable" /> doesn't preserve foreign key relationsships #1983

Open kasparsatke opened 8 months ago

kasparsatke commented 8 months ago

I am working on a table that has multiple foreign keys. This table is versionable.

Once I tried to call foreign keys on a version of the table, I got an error like "unknown method".

I then checked the DB and found out that in the versions table foreign keys are not preserved.

I looked a bit around the Propel documentation and found out that the behavior "archivable" does have the option to preserve foreign keys - if I do understand it the right way. The behavior "archivable" has parameters like "inherit_foreign_key_relations" and "inherit_foreign_key_constraints" and it sounds like they would solve my issue. Unfortunately, I did not find any documentation if those parameters are also available for the behaviour "versionable". Quickly looking through the source code I also did not find any evidence.

Saying this, I would like to file a feature request/bug report to enable calling foreign key objects on versioned tables.

PhilinTv commented 2 months ago

Hey @kasparsatke , Could you share an example table schema and a use case for better problem confirmation and resolution?

kasparsatke commented 2 months ago

Dear Denis,

thanks for picking up on this.

Below I provide a shortened (and slightly altered) table scheme.

    <table name="invoices">
        <column name="id" type="integer" required="true" primaryKey="true" autoIncrement="true" />
        <column name="description" type="longvarchar" />
        <column name="internal_note" type="longvarchar" />
        <column name="creator" type="integer"  default="0" />
        <foreign-key foreignTable="be_user" phpName="InvoiceCreator" onDelete="none" onUpdate="none">
            <reference local="creator" foreign="id" />
        </foreign-key>
        <column name="last_change_editor" type="integer" default="0" />
        <foreign-key foreignTable="be_user" phpName="InvoiceLastChangeEditor" onDelete="none" onUpdate="none">
            <reference local="last_change_editor" foreign="id" />
        </foreign-key>
        <column name="created" type="TIMESTAMP" defaultExpr="CURRENT_TIMESTAMP" required="true" />
        <column name="modified" type="TIMESTAMP" defaultExpr="CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP" required="true" />
        <column name="supplier" type="integer" />
        <foreign-key foreignTable="suppliers" onDelete="none" onUpdate="none">
            <reference local="supplier" foreign="id" />
        </foreign-key>
        <column name="customer" type="integer" />
        <foreign-key foreignTable="customers" onDelete="none" onUpdate="none">
            <reference local="customer" foreign="id" />
        </foreign-key>
        <column name="product" type="integer" />
        <foreign-key foreignTable="products" onDelete="none" onUpdate="none">
            <reference local="product" foreign="id" />
        </foreign-key>

        [...]

        <behavior name="versionable" />
    </table>

As you can see, the table is versionable and contains multiple foreign keys. The "versionable" behaviour works like this: Every time I update an object from the original table with different values, a copy of the old object with incremented version counter will be created in the table invoices_version. The updated original object of course will be updated in the original table invoices


Now when I call an object of the main table, like e.g.

$invoice = InvoicesQuery::create()->findPk(1);

I can also directly call a related Object and get columns of the related object e.g.

$creatorname = $invoice->getInvoiceCreator()->getName();

Just normal stuff Propel allows with foreign keys.

Now once I try to retrieve on a previous version of the original object like so

$old_invoice = InvoicesVersionQuery::create()->filterByVersion(3)->findPk(1);

this works fine. However, once I try to get related objects of foreign tables like

$old_creatorname = $old_invoice->getInvoiceCreator()->getName();

it fails with an unknown method thrown as the foreign key on the be_user table is not preserved by Propel in the versioned table (invoices_version).



Instead I have to go back and call the related table separatedly:

$old_creatorname = BeUserQuery::create()->findPk($old_invoice->getCreator())->getName;


Checking the underlying DB tables, I can confirm that foreign keys are added by propel on the original table, yet not on the versions table.

It be would really great if I could call the related objects of table object versions out of the box. To my understanding, @mringler has recently realized this feature on the "archivable" behaviour with two parameters implementing foreign keys on archived table objects. Unfortunately I could not test it yet and hence don't know if this works the way I understand it, since until recently I had to use an older Propel version where the enhancements to the "archivable" behaviour had not been not included yet.

I was hoping that (if functioning the way I understand it here) the same or similar parameters as in "archivable" could be implemented for "versionable".

mringler commented 1 week ago

Hey @kasparsatke, I needed the same functionality for another behavior, so I extracted it from archiveable behavior into its own behavior ("synced table") and set it as a base for versionable and archivable. This allows to configure foreign keys on the versionable table as you requested. If this is interesting to you, have a look at https://github.com/mringler/Propel2/pull/10.

kasparsatke commented 1 week ago

Dear Moritz,

just wow. This sounds super promising. Also your new base behaviour synced_table has great configuration options.

Out of interest, what was the new behaviour that you wrote this for? Just found it at https://github.com/mringler/Propel2/pull/12. Auditing sounds also interesting. I am not sure yet I understand how this will work yet I feel this also should allow some nice possibilites.

Are your features Is your whole feature already ripe to enter the main repo?

All the best, Kaspar.

mringler commented 1 week ago

Hey Kaspar,

thank you for the kind feedback! Yes, I needed it for that auditable behavior. It records changes to the parent table, so you can figure out who did what when. It seems to work well, but there is a huge caveat: I wanted to avoid duplicating every single row, so now each change only records the overridden values (instead of the new values), and the actual audit has to be rebuilt going backwards from the current row. That's fun and interesting, but it is fragile to changes from outside Propel (i.e. direct database access). For a general release, it would probably need a default fool-proof mechanism as well.

But since nothing gets merged into Propel anymore, that does not matter, there is no point in creating PRs. Synced table and the changes to versionable are finished, but if you want to use it, you have to get it into your own fork, or copy the files and register them as external behavior. I have completely switched to my own fork, you are welcome to use that too of course (I don't plan on doing something crazy with it). None of these options are great, but it is what it is.

I'd be curious if the changes to versionable work for you though. Let me know if you try it out or if there is something else I can do.

Best, Moritz

kasparsatke commented 1 week ago

Hi Moritz,

as for the direct database access - I would say there is no bulletproof method.

One way to minimize potential impact would be to make the table WORM for a certain (audit) user. A drawback is that you would at least need root access again once the table definition changes I guess.

Or, one could prevent changes with triggers on that audit table that prevent UPDATE, DELETE or REPLACE statements to be executed. But then again someone with root access could also tamper/delete these triggers again- regardless if in Propel or directly on the DB.

Or, at least in MySQL/MariaDB, as I have just found out, there is a special ARCHIVE storage engine which could is almost WORM (besides the allowed REPLACE statement). But then again you would need to handle a second DB with this engine as you don't want have WORM on all your tables in the main DB. I don't know how easy it would be in Propel to adjust your auditable behaviour to offer the option to automatically copy/redirect changes into the special archive DB.

So in general I would say there is alway a risk yet I would still say your auditable behaviour would also be a great addition to Propel!

Which brings me to the following question as to why you say nothing gets merged anymore into Propel. I thought @dereuromark has been the previous maintainer and could not be so anymore and now @PhilinTv is the current maintainer. So I thought although progress in the Propel repo is slow, there is something happening. Or do you think otherwise?

If indeed no merge and release is happening anymore, I have to think of a way to test your additions with real data in a new test environment. I am not sure if/how fast I can do this, so please don't expect feedback soon.

Best, Kaspar.

mringler commented 1 week ago

Hey Kaspar,

oh,protecting the table on DB-level is an interesting idea. But as you say, it seems unfeasible to forbid manual updates completely. The problem with the current solution is that manual updates are not just unaccounted, they give you wrong audits (I have added some description to the PR, it felt out of place here). Not sure if it makes sense to be "published" the way it is, and figuring out how to get there does not seem to make sense at the moment.

As to the state of the repo, I had several PRs, from bugfix to feature, doesn't seem like anybody looked at them even once. #1981 is still open, it's a simple bugfix, nine month old, not even a comment. Go figure...

No worry about the testing, I just remembered your comment and thought I'd let you know.

Best, Moritz

kasparsatke commented 6 days ago

Oh I am sure people are looking, watching and even reading. Maybe even many. Propel is a great project!

It just needs people to engage to keep things rolling.

So to all you people reading this, engage! Even if you feel you can’t contribute with code reviews or whatnot, reach out to people you know who can.

And for whoever is maintaining this repo, think of how responsibility can be shared with multiple capable and trustworthy people who ideally have knowledge of Propel‘s inner workings. Increase the Truck number!

There is no such sad thing as a great project depending on a lonely maintainer that has not enough time, means or strength and gets overwhelmed with responsibilities. Recent examples have bitterly shown what might happen, if a lonely maintainer gets swamped: XZ anyone?

Saying this, come up with people you trust and that have the right knowledge to easen the burden. Keep up the great work yourself && let trustworthy people help you! Together you are strong! Together the community is strong! 💪