Fix/477 #478

josephmancuso · 2021-07-24T15:31:50Z

Closes #477

TODO:

Refactor very similiar methods. There are 2 methods in the BelongsToMany relationship that are nearly identical that we should refactor into 1 method
Need to test the timestamps. I have a feeling the timestamps are being overidden
Fix foreign key overriding relationship key

yubarajshrestha · 2021-07-25T03:42:37Z

Closes #477

TODO:

Refactor very similiar methods. There are 2 methods in the BelongsToMany relationship that are nearly identical that we should refactor into 1 method

Need to test the timestamps. I have a feeling the timestamps are being overidden

from .BaseRelationship import BaseRelationship
from ..collection import Collection
from inflection import singularize
from ..models.Pivot import Pivot


class BelongsToMany(BaseRelationship):
    """Has Many Relationship Class."""

    def __init__(
        self,
        fn=None,
        local_foreign_key=None,
        other_foreign_key=None,
        local_owner_key=None,
        other_owner_key=None,
        table=None,
        with_timestamps=False,
        pivot_id="id",
        attribute="pivot",
        with_fields=[],
    ):
        if isinstance(fn, str):
            self.fn = None
            self.local_foreign_key = fn
            self.other_foreign_key = local_foreign_key
            self.local_owner_key = other_foreign_key or "id"
            self.other_owner_key = local_owner_key or "id"
        else:
            self.fn = fn
            self.local_foreign_key = local_foreign_key
            self.other_foreign_key = other_foreign_key
            self.local_owner_key = local_owner_key or "id"
            self.other_owner_key = other_owner_key or "id"

        self._table = table
        self.with_timestamps = with_timestamps
        self._as = attribute
        self.pivot_id = pivot_id
        self.with_fields = with_fields

    def set_keys(self, owner, attribute):
        self.local_foreign_key = self.local_foreign_key or "id"
        self.other_foreign_key = self.other_foreign_key or f"{attribute}_id"
        print(self.local_foreign_key, self.other_foreign_key)
        return self

    def table(self, table):
        self._table = table
        return self

    def get_related(self, query, relation, eagers=None):
        eagers = eagers or []
        builder = self.get_builder().with_(eagers)

        if not self._table:
            pivot_tables = [
                singularize(builder.get_table_name()),
                singularize(query.get_table_name()),
            ]
            pivot_tables.sort()
            pivot_table_1, pivot_table_2 = pivot_tables
            self._table = "_".join(pivot_tables)
            self.other_foreign_key = self.other_foreign_key or f"{pivot_table_1}_id"
            self.local_foreign_key = self.local_foreign_key or f"{pivot_table_2}_id"
        else:
            pivot_table_1, pivot_table_2 = self._table.split("_", 1)
            self.other_foreign_key = self.other_foreign_key or f"{pivot_table_1}_id"
            self.local_foreign_key = self.local_foreign_key or f"{pivot_table_2}_id"

        table2 = builder.get_table_name()
        table1 = query.get_table_name()
        result = builder.select(
            f"{table2}.*",
            f"{self._table}.{self.local_foreign_key}",
            f"{self._table}.{self.other_foreign_key}",
        ).table(f"{table1}")

        if self.with_fields:
            for field in self.with_fields:
                result.select(f"{self._table}.{field}")

        result.join(
            f"{self._table}",
            f"{self._table}.{self.local_foreign_key}",
            "=",
            f"{table1}.{self.local_owner_key}",
        )

        result.join(
            f"{table2}",
            f"{self._table}.{self.other_foreign_key}",
            "=",
            f"{table2}.{self.other_owner_key}",
        )

        if self.with_timestamps:
            result.select(
                f"{self._table}.updated_at as updated_at",
                f"{self._table}.created_at as created_at",
            )

        if self.pivot_id:
            # result.select(f"{self._table}.{self.pivot_id} as {self.pivot_id}")
            result.select(f"{self._table}.{self.pivot_id} as pivot_id")

        if isinstance(relation, Collection):
            final_result = result.where_in(
                self.local_owner_key,
                relation.pluck(self.local_owner_key, keep_nulls=False),
            ).get()
        else:
            final_result = result.where(
                self.local_owner_key, getattr(relation, self.local_owner_key)
            ).get()

        for model in final_result:
            pivot_data = {
                self.local_foreign_key: getattr(model, self.local_foreign_key),
                self.other_foreign_key: getattr(model, self.other_foreign_key),
            }

            model.delete_attribute(self.local_foreign_key)
            model.delete_attribute(self.local_foreign_key)
            model.delete_attribute("pivot_id")

            if self.with_timestamps:
                pivot_data.update(
                    {
                        "updated_at": getattr(model, "updated_at"),
                        "created_at": getattr(model, "created_at"),
                    }
                )

            if self.pivot_id:
                # pivot_data.update({self.pivot_id: getattr(model, self.pivot_id)})
                pivot_data.update({self.pivot_id: getattr(model, "pivot_id")})

            if self.with_fields:
                for field in self.with_fields:
                    pivot_data.update({field: getattr(model, field)})
                    model.delete_attribute(field)

            setattr(
                model,
                self._as,
                Pivot.on(builder.connection)
                .table(self._table)
                .hydrate(pivot_data)
                .activate_timestamps(self.with_timestamps),
            )

        return final_result

    def register_related(self, key, model, collection):
        model.add_relation(
            {
                key: collection.where(
                    self.local_foreign_key, getattr(model, self.local_owner_key)
                )
            }
        )

Above code does solve the issue with ids. I also removed apply_query method, but we need a way to delete unwanted fields from there i.e. permission_id, role_id, pivot_id from permissions model. If you look closely inside for model in final_results, model.delete_attribute that doesn't work that gives keyerror. on your code also p.delete_attribute("m_reserved1") this part is giving error.

josephmancuso · 2021-07-25T14:48:50Z

@yubarajshrestha doesn't give a keyerror for me 🤔 . How would it give a key error? It is adding m_reserved1 as a select statement.

yubarajshrestha · 2021-07-25T15:00:07Z

@yubarajshrestha doesn't give a keyerror for me 🤔 . How would it give a key error? It is adding m_reserved1 as a select statement.

If you checked my code above everything is working and also don't have to add extra variables except that pivot_id. And there also if I try to remove the attribute that gives error.

josephmancuso · 2021-07-25T15:06:46Z

@yubarajshrestha Oh i see. we do need that apply_query method though. the apply_query method is ran when we access the relationship directly. like this:

User.find(1).roles

josephmancuso · 2021-07-25T15:08:43Z

Looks like your code is working when we are eager loading. Did you try my original class though? I had this working already for me with the tables i had setup.

josephmancuso · 2021-07-25T15:34:25Z

Its hard sometimes to write tests for these queries but i'll try

This reverts commit 3337a59.

josephmancuso · 2021-07-25T16:03:35Z

@yubarajshrestha I tried refactoring but it didn't work.. spent too much time on it but this code should work now. DId you try it the way it is here?

yubarajshrestha · 2021-07-25T16:21:39Z

@yubarajshrestha I tried refactoring but it didn't work.. spent too much time on it but this code should work now. DId you try it the way it is here?

Ok i will try this and let you know, and if you got 5 minutes then will you please review test case PR in masonite?

josephmancuso · 2021-07-25T17:02:19Z

@yubarajshrestha pushed some additional changes just now. Everything works on my end. The tests pass and my manual testing looks good. If you could just double check this fix, i refactored a lot of this class

yubarajshrestha · 2021-07-25T17:05:42Z

@yubarajshrestha pushed some additional changes just now. Everything works on my end. The tests pass and my manual testing looks good. If you could just double check this fix, i refactored a lot of this class

Which python version are you using?

josephmancuso · 2021-07-25T17:07:18Z

3.9.1. I don't see why a python version difference would matter here though. What are you seeing?

yubarajshrestha · 2021-07-25T17:09:00Z

3.9.1. I don't see why a python version difference would matter here though. What are you seeing?

builtins.AttributeError
class model 'Permission' has no attribute role_id

josephmancuso · 2021-07-25T17:12:24Z

@yubarajshrestha Can i see your relationship signature? inside your model?

Also can you pull the latest changes because if anything it should be doing something like

class model 'Permission' has no attribute m_reserved3

i think ..

Also what is role_id. Is that the pivot table primary key or is that the distant table primary key?

yubarajshrestha · 2021-07-25T17:14:59Z

@yubarajshrestha Can i see your relationship signature? inside your model?

Also can you pull the latest changes because if anything it should be doing something like
class model 'Permission' has no attribute m_reserved3
i think ..

Also what is role_id. Is that the pivot table primary key or is that the distant table primary key?

class Role:
     __fillable__ = ['name']

    @belongs_to_many('role_id', 'permission_id', 'id', 'id')
    def permissions(self):
        from app.models.Permission import Permission
        return Permission

class Permission:
    __fillable__ = ['name']

class PermissionRole:
    __fillable__ = ['permission_id', 'role_id']

roles = Role.with_('permissions').get()

josephmancuso · 2021-07-25T18:03:16Z

@yubarajshrestha Ok I was able to replicate this exact issue by setting up those tables exactly as you have yours.

Can you pull these changes and try again?

All of this is a little confusing to be honest so i setup all the primary keys as different names on the tables.

The way i have my tables set up is like this:

permissions
------
permission_id PRIMARY
name: VARCHAR

roles
------
role_id: PRIMARY
name: VARCHAR

permission_role
------
permission_role_id: PRIMARY
role_id: int
permission_id: int

So then my models are setup like this:

class Role(Model):
    __fillable__ = ['name']

    @belongs_to_many('role_id', 'permission_id', 'role_id', 'permission_id', pivot_id="permission_role_id")
    def permissions(self):
        return Permission

class Permission(Model):
    __fillable__ = ['name']

    @belongs_to_many('permission_id', 'role_id', 'permission_id', 'role_id', pivot_id="permission_role_id")
    def roles(self):
        return Role

yubarajshrestha · 2021-07-25T18:09:42Z

@yubarajshrestha Ok I was able to replicate this exact issue by setting up those tables exactly as you have yours.

Can you pull these changes and try again?

All of this is a little confusing to be honest so i setup all the primary keys as different names on the tables.

The way i have my tables set up is like this:
permissions
------
permission_id PRIMARY
name: VARCHAR

roles
------
role_id: PRIMARY
name: VARCHAR

permission_role
------
permission_role_id: PRIMARY
role_id: int
permission_id: int
So then my models are setup like this:
class Role(Model):
    __fillable__ = ['name']

    @belongs_to_many('role_id', 'permission_id', 'role_id', 'permission_id', pivot_id="permission_role_id")
    def permissions(self):
        return Permission

class Permission(Model):
    __fillable__ = ['name']

    @belongs_to_many('permission_id', 'role_id', 'permission_id', 'role_id', pivot_id="permission_role_id")
    def roles(self):
        return Role

Looks like now we have different bug. If you check carefully, you will see all the items inside permission is same except the pivot value.

{
    "id": 1,
    "name": "Role Name",
    "slug": "role-name",
    "status": "active",
    "created_at": "2021-07-19T14:28:11.173605+00:00",
    "updated_at": "2021-07-19T14:28:11.173605+00:00",
    "permissions": [
        {
            "id": 1,
            "name": "Add Role",
            "slug": "add-role",
            "created_at": "2021-07-19T14:28:11.219347+00:00",
            "updated_at": "2021-07-19T14:28:11.219347+00:00",
            "pivot": {
                "role_id": 3,
                "permission_id": 1,
                "id": 3
            }
        },
        {
            "id": 1,
            "name": "Add Role",
            "slug": "add-role",
            "created_at": "2021-07-19T14:28:11.219347+00:00",
            "updated_at": "2021-07-19T14:28:11.219347+00:00",
            "pivot": {
                "role_id": 2,
                "permission_id": 1,
                "id": 2
            }
        },
        {
            "id": 1,
            "name": "Add Role",
            "slug": "add-role",
            "created_at": "2021-07-19T14:28:11.219347+00:00",
            "updated_at": "2021-07-19T14:28:11.219347+00:00",
            "pivot": {
                "role_id": 1,
                "permission_id": 1,
                "id": 1
            }
        }
    ]
},

josephmancuso · 2021-07-25T22:47:00Z

ugh ... this feature is going to be the death of me ... i'll take a look

josephmancuso · 2021-07-25T23:41:14Z

@yubarajshrestha OK 😞 . Can you please try hopefully one last time? I think the query was good but the register_related was wrong.

josephmancuso · 2021-07-25T23:43:04Z

You will notice that the distant table will have the local foreign key added. This is because we need this to register the relation later

yubarajshrestha · 2021-07-26T03:25:28Z

You will notice that the distant table will have the local foreign key added. This is because we need this to register the relation later

{
    "id": 1,
    "name": "Role Name",
    "slug": "role-name",
    "created_at": "2021-07-19T14:28:11.173605+00:00",
    "updated_at": "2021-07-19T14:28:11.173605+00:00",
    "permissions": [
        {
            "id": 1,
            "name": "Add Role",
            "slug": "add-role",
            "created_at": "2021-07-19T14:28:11.219347+00:00",
            "updated_at": "2021-07-19T14:28:11.219347+00:00",
            "pivot": {
                "role_id": 1,
                "permission_id": 1,
                "id": 1
            }
        },
        {
            "id": 1,
            "name": "Edit Role",
            "slug": "edit-role",
            "created_at": "2021-07-19T14:28:11.219347+00:00",
            "updated_at": "2021-07-19T14:28:11.219347+00:00",
            "pivot": {
                "role_id": 1,
                "permission_id": 2,
                "id": 4
            }
        },
        {
            "id": 1,
            "name": "Delete Role",
            "slug": "delete-role",
            "created_at": "2021-07-19T14:28:11.219347+00:00",
            "updated_at": "2021-07-19T14:28:11.219347+00:00",
            "pivot": {
                "role_id": 1,
                "permission_id": 3,
                "id": 7
            }
        },
        {
            "id": 1,
            "name": "Role Report",
            "slug": "role-report",
            "created_at": "2021-07-19T14:28:11.219347+00:00",
            "updated_at": "2021-07-19T14:28:11.219347+00:00",
            "pivot": {
                "role_id": 1,
                "permission_id": 4,
                "id": 10
            }
        },
    ],
}

Yea looks like id of permissions are inside pivot rather than in permissions itself, actually response in pivot is perfect, id inside permission must be permissions id. If we ever need the local foreign key then you can just add "role_id" inside permissions.

josephmancuso · 2021-07-26T12:12:34Z

yeah the way the register_related method works is it takes the keys and does a .where on the collection.

So on our example we have a Role model and we are trying to get a collection of Permissions to register to them but if we have a permission like this:

{
            "id": 1,
            "name": "Role Report",
            "slug": "role-report",
            "created_at": "2021-07-19T14:28:11.219347+00:00",
            "updated_at": "2021-07-19T14:28:11.219347+00:00",
            "pivot": {
                "role_id": 1,
                "permission_id": 4,
                "id": 10
            }
        },

We don't know which role this permission is assigned to unless we access the pivot record. So now what I did was I put the role id on the permission but if the role_id is id then I think its going to overwrite the permissions id.. It worked for me because all my primary keys were different names ..

Only way now is to name it something else like {table_name}_id like roles_id and put it on the permission.

Honestly I think automatically putting a role_id on permission I technically fine because for this relationship, this permission does technically have a role_id

yubarajshrestha · 2021-07-26T12:17:15Z

yeah the way the register_related method works is it takes the keys and does a .where on the collection.

So on our example we have a Role model and we are trying to get a collection of Permissions to register to them but if we have a permission like this:
{
            "id": 1,
            "name": "Role Report",
            "slug": "role-report",
            "created_at": "2021-07-19T14:28:11.219347+00:00",
            "updated_at": "2021-07-19T14:28:11.219347+00:00",
            "pivot": {
                "role_id": 1,
                "permission_id": 4,
                "id": 10
            }
        },
We don't know which role this permission is assigned to unless we access the pivot record. So now what I did was I put the role id on the permission but if the role_id is id then I think its going to overwrite the permissions id.. It worked for me because all my primary keys were different names ..

Only way now is to name it something else like {table_name}_id like roles_id and put it on the permission.

Honestly I think automatically putting a role_id on permission I technically fine because for this relationship, this permission does technically have a role_id

In current result, if you just hide pivot table from model then you will never get the id of permission from related data, and I think that's an issue.

This reverts commit efab87a.

This reverts commit 3ecef0e.

josephmancuso · 2021-08-05T02:04:32Z

@yubarajshrestha added the fix to this. Does this work for you now?

josephmancuso added 3 commits July 24, 2021 11:21

fixed query for has many

359388e

fixed query for belongs to many

286a7d9

formatted

7b79397

josephmancuso mentioned this pull request Jul 24, 2021

with_ relation gives un-wanted data. #477

Closed

refactored variable names

7a6ed72

format and refactor

3337a59

Revert "format and refactor"

8444c06

This reverts commit 3337a59.

josephmancuso added 3 commits July 25, 2021 12:03

formatted

a481203

fixed deprecation

5cad072

fixed lint

561e96e

josephmancuso added 5 commits July 25, 2021 12:40

fixed timestamps

263dade

removed print

e23868d

fixed pivot id

f111d4d

fixed timestamps

cba23dd

fixed pivot id

c885705

fixed belongs to many

7080dca

josephmancuso added 2 commits July 25, 2021 19:27

fixed bad register relationship

d722eea

fixed belongs to many

204968e

fixed m_reserved1

db226a5

josephmancuso added 5 commits August 4, 2021 19:35

added locking

efab87a

added lock commands to Model passthrough

3ecef0e

Revert "added locking"

7fd11db

This reverts commit efab87a.

Revert "added lock commands to Model passthrough"

18958ff

This reverts commit 3ecef0e.

added belongs to many fix

df0e6e3

josephmancuso added 3 commits August 4, 2021 22:08

removed print

08ba6d5

fixed keys

91bd339

added database options

2731dc9

josephmancuso merged commit 9b4aa4b into 1.0 Aug 10, 2021

josephmancuso deleted the fix/477 branch October 17, 2021 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/477 #478

Fix/477 #478

josephmancuso commented Jul 24, 2021 •

edited

yubarajshrestha commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

yubarajshrestha commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

yubarajshrestha commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

yubarajshrestha commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

yubarajshrestha commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

yubarajshrestha commented Jul 25, 2021 •

edited

josephmancuso commented Jul 25, 2021 •

edited

yubarajshrestha commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

yubarajshrestha commented Jul 26, 2021 •

edited

josephmancuso commented Jul 26, 2021

yubarajshrestha commented Jul 26, 2021 •

edited

josephmancuso commented Aug 5, 2021

Fix/477 #478

Fix/477 #478

Conversation

josephmancuso commented Jul 24, 2021 • edited

yubarajshrestha commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

yubarajshrestha commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

yubarajshrestha commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

yubarajshrestha commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

yubarajshrestha commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

yubarajshrestha commented Jul 25, 2021 • edited

josephmancuso commented Jul 25, 2021 • edited

yubarajshrestha commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

josephmancuso commented Jul 25, 2021

yubarajshrestha commented Jul 26, 2021 • edited

josephmancuso commented Jul 26, 2021

yubarajshrestha commented Jul 26, 2021 • edited

josephmancuso commented Aug 5, 2021

josephmancuso commented Jul 24, 2021 •

edited

yubarajshrestha commented Jul 25, 2021 •

edited

josephmancuso commented Jul 25, 2021 •

edited

yubarajshrestha commented Jul 26, 2021 •

edited

yubarajshrestha commented Jul 26, 2021 •

edited