Something Strange with Custom Primary Key

[starfishpatkhoo]

I'm having some strange problems when using a custom PK... Not too sure where it is, but my gut feel is two bugs are related somehow.

Assume a DB with:

CREATE TABLE my_table (my_pk TEXT NOT NULL PRIMARY KEY, data INTEGER, name TEXT)

Then try something like this

$db = new \flight\ActiveRecord($pdo, "my_table", ["primaryKey" => "my_pk"]);
echo "IsHydrated: " . ($db->isHydrated() ? "Yes" : "No") . PHP_EOL;  // No

$my_pk = time();

$db->my_pk = $my_pk;
$db->data: = 12345;
echo "PK: " . $db->my_pk . PHP_EOL;

$db->save();  // Doesn't Work
echo "IsHydrated: " . ($db->isHydrated() ? "Yes" : "No") . PHP_EOL;  // No
echo "PK: " . $db->my_pk . PHP_EOL;  // PK

$db->insert();  // Now it really gets inserted
echo "IsHydrated: " . ($db->isHydrated() ? "Yes" : "No") . PHP_EOL;  // No
echo "PK: " . $db->my_pk . PHP_EOL;  // This is Row No, not PK

$db->find($my_pk);  // This works
echo "IsHydrated: " . ($db->isHydrated() ? "Yes" : "No") . PHP_EOL;  // Yes
echo "PK: " . $db->my_pk . PHP_EOL;  // Back to PK instead of Row No

$db->name = "Boo";
echo "IsDirty: " . ($db->isDirty() ? "Yes" : "No") . PHP_EOL;  // Yes
$db->save();
echo "IsDirty: " . ($db->isDirty() ? "Yes" : "No") . PHP_EOL;  // No
echo "IsHydrated: " . ($db->isHydrated() ? "Yes" : "No") . PHP_EOL;  // Yes
echo PHP_EOL;

Strange Issue 1

After insert(), the PK changes to become the table row number. I don't think this is intended behaviour right?

Strange Issue 2

$db->save() is supposed to either insert() or update() depending on the situation right? But it doesn't do anything for "inserts" in this case, so you have to specifically insert()...

Can also try:

$db = new \flight\ActiveRecord($pdo, "my_table", ["primaryKey" => "my_pk"]);
$db->my_pk = "Duplicate ID";
$db->save();  // No exception
$db->insert();  // Exception

I would have thought that an exception would have occurred on the save() ..

I have a feeling that these two problems are related somehow. Maybe there's something I missed, so I'm still investigating...

Additional Discussion

After insert(), isHydrated() continues to say no. I think it is fine - documentation states that it is true only if a find() was successful.

I'm perfectly OK with this behaviour, but just wanted to ask if it should also be true after a successful insert() ? Does a successful "insert" also imply a successful "find" in an ORM pattern like ActiveRecord because the model remains with the record data? Note that after "updating/saving" a previously "found" record, isHydrated() is still true... so maybe people might misunderstand and try to use isHydrated() as a form of hasData() when it is not really ?

Ha ha.. not trying to start a flame war here...

[n0nag0n]

So this only works if the primary key is an integer. If I'm being honest, I don't know if I've ever seen a TEXT field be a primary key ever in my career unless they are being used as UUID's (which in and of themselves are complex to properly increment). This library doesn't handle text based primary keys. What I would recommend is if you need the value of the TEXT field be unique, I would add an id column to your table and assign that as a primary key, and then I would add a UNIQUE index to your TEXT field.

[starfishpatkhoo]

So this only works if the primary key is an integer. If I'm being honest, I don't know if I've ever seen a TEXT field be a primary key ever in my career unless they are being used as UUID's (which in and of themselves are complex to properly increment). This library doesn't handle text based primary keys. What I would recommend is if you need the value of the TEXT field be unique, I would add an id column to your table and assign that as a primary key, and then I would add a UNIQUE index to your TEXT field.

hmmm, understand.. yeah, there's UUIDs all over the place. Field identifiers, job IDs, session IDs, User IDs.. duh. It's insane these days. They are not meant to be auto incremented.. Ultimately, it's nice to explicitly use these IDs as the PK because, well, we always look them up by the UUID, not the row ID ..

Do you know if this is also an issue for primary keys that are not AUTO_INCREMENT even if they are INTEGER ? Sometimes we use time() type things for event tracking etc. They're simple numbers, but they don't auto increment..

[n0nag0n]

Umm, my guess is it would be a problem. Right now the way the $ActiveRecord->id is assigned is from calling the $pdo->lastInsertId() method. I'd have to do a little experimenting to see if you did an insert/save and you supplied to PK, what that would do and if you fetched rows off of that what it would do....might be a fun experiment :) I've looked into auto figuring out the next UUID if it was a trigger/function in the database and I didn't find very many good answers on how to address that.

[starfishpatkhoo]

What do you think about splitting ->id from ->PK .. If ->internal_row_id is used for internal mechanics, and ->PK is used mostly for "dev facing" code?

And then, if the current record (via find) has a +ve ->internal_row_id then it has been retrieved, and ->save() can do an ->update() .. If not, then it is probably meant to be an ->insert() ..

So most of the existing mechanisms would not be affected, only when doing a default ->find() we need to use primaryKey instead of id.. Or so I think.. LOL...

Actually, to be honest, I'm fine to not use ->save() because it becomes explicitly clear when we are trying to update existing information vs creating new information.. So yeah.. ^_^

[n0nag0n]

So I spent a little time to see what I could do here. I actually found several bugs in the setup.

• It was only checking if there was a primaryKey field and if it had one, it would assume an update() was going to happen.
• It was always assuming that the new ID was going to be an int (but I knew that already).
• It also never marked a successfully inserted row as hydrated.

So with those things fixed, can you test the current master branch and see if it does what you want?

Additionally, just so you're aware, you can automate your uuids if you wanted if you did something like:

class MyTextRecord extends flight\ActiveRecord {
    public function __construct($myDbConnection, $config = []) {
        parent::__construct($myDbConnection, 'my_text_table', $config);
        $this->primaryKey = 'my_pk';
    }

    public function beforeInsert(self $self) {
        $self->my_pk = time(); // or however you generate your uuid
    }
}

[starfishpatkhoo]

Awesome! Looks pretty good!

$db->my_pk = "something";
echo "PK: " . $db->my_pk . PHP_EOL;

$db->save();  // Works
echo "IsHydrated: " . ($db->isHydrated() ? "Yes" : "No") . PHP_EOL;  // Yes
echo "PK: " . $db->my_pk . PHP_EOL;  // PK

->save() works just fine now. And it exceptions when the PK is duplicated. And ->isHydrated() is true after save/insert. I think the biggest thing is that PK doesn't change after the insert().. that was messing up code after the insert that depended on PK..

And yeah, thanks for the beforeInsert() reminder! I've been using afterFind() a bit for some pretty-fier magic getters. But yeah, good tip!

If there are no further issues, and no one else sees any problem, feel free to close.. ^_^

[n0nag0n]

https://github.com/flightphp/active-record/releases/tag/v0.4.2 She's released into the wild. Thanks for your help! Come hang out in the chat room for Flight sometime. And tell your friends about flight :)

[n0nag0n]

I've also added some documentation in the insert() method surrounding text based primary keys. https://docs.flightphp.com/awesome-plugins/active-record#crud-functions

[starfishpatkhoo]

https://github.com/flightphp/active-record/releases/tag/v0.4.2 She's released into the wild. Thanks for your help! Come hang out in the chat room for Flight sometime. And tell your friends about flight :)

Awesome thanks! It's working well so far..

Yep, I've been pushing flightphp as a framework of choice since almost 10 years ago now.. I think it was like version 1.2 or something.,. LOL..

Ah IRC? Ha ha ha, it has been more than a decade since I last went into IRC .. 🤣 Been on discord more nowadays, but let me go have a look..

Thanks for keeping flight and active-record alive. It's much appreciated. Wish I had more time to contribute back..

[n0nag0n]

ha, it's not IRC, that takes me way back. It's like IRC 2.0. I would use discord but there are some folks that I've worked with in Europe that have issues with some of discords privacy issues and requiring phone numbers and such. Just hop in the room. I typically use app.element.io for the web and the Element chat client for my phone. https://matrix.to/#/!cTfwPXhpkTXPXwVmxY:matrix.org?via=matrix.org&via=integrations.ems.host&via=leitstelle511.net

And thank you for your help. You may not be able to contribute much but you were very detailed in your Issue which made it so easy for me to write a unit test, replicate, and then safely fix the issue moving forward.