Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using ORM to save Multi-PK tables overwrites multiple rows #201

Closed
sigginet opened this issue Mar 3, 2018 · 3 comments

Comments

Projects
None yet
3 participants
@sigginet
Copy link

commented Mar 3, 2018

Kohana ORM always expects a single column unique primary key when updating rows in the database.

I found this out the hard way, but I'm too lazy to fix it in the ORM.

Imagine you have a table like this:

CREATE TABLE `strings` (
  `id` varchar(128) NOT NULL DEFAULT '',
  `locale_id` char(5) NOT NULL,
  `string` text,
  PRIMARY KEY (`id`,`locale_id`),
  KEY `s_locale_id` (`locale_id`) USING BTREE,
  KEY `string_lookup` (`id`,`locale_id`),
  CONSTRAINT `s_locale_id` FOREIGN KEY (`locale_id`) REFERENCES `locales` (`id`) ON UPDATE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8 ROW_FORMAT=COMPACT

Then you have the same id for multiple locale_id

So if I have some data like this (CSV)
id,locale_id,string
'message','en','Row 1 text'
'message','fr','Row 2 text'

Now, if I find row 1 with the ORM, update the string to be 'Awesome suit man' and do ORM::save(), then row 2 will contain 'Awesome suit man' also.

The offending code is here:
https://github.com/koseven/koseven/blob/devel/modules/orm/classes/Kohana/ORM.php#L1558-L1562

It would be a bit of an effort to add support for multiple primary keys, but if someone is up for the challenge, it's yours! :)

@toitzi

This comment has been minimized.

Copy link
Member

commented Jan 12, 2019

That's kinda rare.
Even Laravel (at least Version 5..don't know about new ones) ORM based on Eloquent don't supprot this.

There are not that much frameworks supporting this. There is a module (sadly not maintained anymore) which supports this (https://github.com/spadefoot/kohana-orm-leap). This module even is based on Kohana (Code from this module got used as example in other frameworks). As already discussed for Kohana:

... have decided to drop this for the time being. There are more important features we'd like to focus on.

... to be honest it will take quite an effort now so I'd rather concentrate on features that are more likley to be useful to people.

I agree to them. But i am thinking of porting the outdated module to Koseven, as this module has lots of features that are missing in kohana orm module (like this one)...but i don't think this is worth it. If somone wants to do the implementation or thinks this would be useful, please reopen and either give it a +1 or comment on it your opinion

@toitzi toitzi closed this Jan 12, 2019

@toitzi

This comment has been minimized.

Copy link
Member

commented Jan 12, 2019

@bato3

This comment has been minimized.

Copy link

commented Mar 14, 2019

For your own mental health, enter an artificial master key here, and replace the current one with a unique one.
In most cases, such a key is just an unnecessary complexity at the code level.

If you want, you can overwrite the model's constructor, Eg. my User:

   public function __construct($id = NULL){
      if(!is_int($id) && is_string($id))
         $id = Array(Valid::email($id) ? 'email' : 'name' => $id);
      parent::__construct($id);
   }

//load use by id 3
ORM::factory('User', 3)
//load user by name 3
ORM::factory('User', '3')

Although I would recommend using this:

ORM::factory('String', ['id' => 'message','locale_id'=>'en'])
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.