hipsterjazzbo / Landlord

A simple, single database multi-tenancy solution for Laravel 5.2+
MIT License
614 stars 138 forks source link

Tenant ID column is ignored when updating a model #15

Closed dmarklund closed 8 years ago

dmarklund commented 8 years ago

It seems like the tenant id column is ignored when updating a model, it only goes by the ID of the model. Is there any way to make it update WHERE tenant_id = x as well?

This is a real issue for me when I use multi tenant (1 db) and also have "grouped" tables, example below.


system_id id
1 1
1 2
2 1

Does anyone have a work around for this problem?

Regards, Daniel

nielsiano commented 8 years ago

Interested in this as well. Did you dig into it @dmarklund ?

dmarklund commented 8 years ago

hey @nielsiano ! I am afraid that I haven't found a solution for this issue. It seems that Eloquent doesn't support composite keys, and that's the issue. Also, it doesn't look like global scopes applies for when updating/saving/creating eloquent models, only when you're querying for them.

So basically, eloquent will always try to update where id = x and not append anything to that where clause. Have you found any other solution?

I just skipped having composite keys and will use a hash function for the front end instead.

Jmrich commented 8 years ago

@dmarklund Global scope is applied on model creation/update/saves. Check your config settings and your query debugs.

dmarklund commented 8 years ago

@Jmrich Hey. I have checked my configs, and it works great whenever I query for models.

Model::get()

The above works fine and adds the system_id = x clause. But this doesn't work:

$model = Model::get();
$model->key = "value";
$model->save();

The above only results in UPDATE WHERE id = x, and does not add the global scope of the system_id. I find it very strange. @Jmrich , do you have any idea why this is not working?

I know that, one could write:

Model::where('key', 'val')->update([ ... ])

And this would add the global scope, but I'd like to be able to do this the other way as well.

Jmrich commented 8 years ago

@dmarklund When I run

startQueryDebug();
        Message::get();
        endQueryDebug();

Start and end query debug are helper methods I created. This is the output of the query, which is the same query that you stated above.

"query" => "select * from `messages` where `messages`.`hospital_id` = ?"
    "bindings" => array:1 [▼
      0 => 1

When I test your code out on my end I get

Method save does not exist.

because ->get() returns a collection. Not sure if its your intention to do a mass update on all of those records.

$model = Message::get();
        $model->body = "fancy";
        dd($model);
        $model->save();
//Output
Collection {#481 ▼
  #items: array:1 [▶]
  +"body": "fancy"
}

If you are trying to do a mass update you may want to check the docs and use the .each collection method. Let me know if you need any other help.

dmarklund commented 8 years ago

@Jmrich Hey man. I'm not trying to perform mass updates, it was just an example.

I've installed a awesome debug-tool and here's what my queries result in. You can clearly see that the global scope tenant does not apply when saving a model.

// fetch a model with a specific ID
$model = License::where('id', 1)->first();

The above query gets updated correctly with the system id by the tenant scope, here's the generated query:

select * from `licenses` where `id` = '1' and `licenses`.`system_id` = '4' limit 1

When I then try to update this model and save it, it doesn't go by the system ID, but only the ID of the model, so I can't use composite keys.

// update
$model->expires = 0;

// save
$model->save();

The above produces the following:

update `licenses` set `updated_at` = '2016-04-14 09:03:19', `expires` = '0' where `id` = '1'
Jmrich commented 8 years ago

@dmarklund ah, I see what you mean now. Not sure why it isn't appended to the update/save. My thought is because of the redundancy. If your working with a model that is filtered when you do your initial search, select * fromlicenseswhereid= '1' andlicenses.system_id= '4' limit 1, why does it need to add that additional constraint again. Thats just my thought on it, but I'm not sure if that is the intended way for it to work.

Jmrich commented 8 years ago

@dmarklund Here is create query so you can see it automatically inserts your tenant column and id. hospital_id is my tenant column.

"query" => "insert into `messages` (`sid`, `sender`, `receiver`, `body`, `status`, `hospital_id`, `updated_at`, `created_at`) values (?, ?, ?, ?, ?, ?, ?, ?)"
    "bindings" => array:8 [▼
      0 => "sid"
      1 => "sender"
      2 => "receiver"
      3 => "body"
      4 => "delivered"
      5 => 1
      6 => "2016-04-14 13:41:08"
      7 => "2016-04-14 13:41:08"
Message::create([
            'sid' => 'sid',
            'sender' => 'sender',
            'receiver' => 'receiver',
            'body' => 'body',
            'status' => 'delivered'
        ]);
dmarklund commented 8 years ago

@Jmrich Yes it works on create, but not on save. I think it's because eloquent does not support composite keys. My tenant ID col is being set automatically when I create something as well.

I was primarily going with composite keys, and therefore wanted the save method to update WHERE id = x and system_id = x. I have dropped this now though and skipped using composite keys.

Jmrich commented 8 years ago

But you understood my thoughts on why it probably isn't there since you are working with a filtered record when you update it, right?

dmarklund commented 8 years ago

@Jmrich Uhm, yeah, I think so. Could you explain a little bit thurder?

Jmrich commented 8 years ago

When you search for a record(by id or where query), landlord adds the tenant column (in your case system_id) to the query. So for example if you are looking for all systems that are blue and the system id of the current user is 1 it will find: select * from systems where color is blue and system_id = 1. So for each record that is returned from the query, it should have the corresponding ID for the row of the column. Ownership of that record has already been determined by the initial search where system_id=1. So when you update/save the record there is no need to add and system_id=1, because whatever records you are updating/saving have already been determined to have a system_id=1. I hope that explains it a little better. Let me know if any of it is unclear so I can explain it a little better.

filpineda commented 8 years ago

I'm experiencing a somewhat similar problem, my case scenario is I can't validate/check for the uniqueness of a column value during create/update even though it should be scoped.

id name company_id
1 name1 1
2 name2 2
3 name1 2
4 name3 1

Say I'd like to create a new entry where: company_id : 1 name : name2

This will fail on Laravel unique validation even though it should not because create should be scoped already right?

Another case is say I'd like to update the row with id = 1 and change its "name" to name2, unique validation will fail again even though i don't have a row with "company_id" : 1 and "name" : name2 present in the records.

Any solution for this? Thanks in advance :smiley:

Jmrich commented 8 years ago

@filpineda You need to read through the docs again. When you run an update and your checking for a unique value you need to exclude the current id. Its hard to say exactly what is wrong without seeing the code for it.

hipsterjazzbo commented 8 years ago

Hey guys, let's move this convo to #21.