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

BaseRelationship: s/self/instance #869

Merged
merged 1 commit into from
Apr 7, 2024

Conversation

pmn4
Copy link
Contributor

@pmn4 pmn4 commented Feb 12, 2024

when migrating from Orator to Masonite, several of my relationship methods were not working. Stepping through the code, I found that when referencing the class via self.__class__ inside the User class, self was an instance of BelongsTo, not User. This PR aims to pass an instance of User instead.

class User(Model):

    @belongs_to
    def parent(self):
        # ⚠️ self is an instance of BelongsTo ⚠️ 
        # expecting self to be an instance of User
        return self.__class__

@josephmancuso
Copy link
Member

can you expand on what this is fixing?

@pmn4
Copy link
Contributor Author

pmn4 commented Feb 12, 2024

oops, this isn't a good example. the user method should return the User class. I tried to make my example simpler, and instead made it confusing!
instead, consider a User class that has a parent relationship, where parent is another record in the User table.
you could say:

@belong_to 
def parent(self):
    return User

or

@belong_to 
def parent(self):
    return self.__class__

@@ -52,7 +52,7 @@ def __get__(self, instance, owner):
object -- Either returns a builder or a hydrated model.
"""
attribute = self.fn.__name__
relationship = self.fn(self)()
relationship = self.fn(instance)()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be the intance variable or the owner variable? i think owner. instance would be the class but owner would be the initialized object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

owner appears to be the class, User in my example, whereas instance is an instance of owner. self.fn is a pointer to an instance method (Ruby vernacular) or prototype function (Node) -- apologies, I don't know the corresponding term in Python -- so I believe instance is correct

@@ -40,6 +40,14 @@ def articles(self):
def profile(self):
return Profile

@belongs_to("id", "parent_dynamic_id")
def parent_dynamic(self):
return self.__class__
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously, this test would have failed

Comment on lines +96 to +91
"""SELECT * FROM [users] WHERE EXISTS ("""
"""SELECT * FROM [users] WHERE [users].[parent_specified_id] = [users].[id]"""
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any issue here, not having an alias for [users] when we self-join?

@@ -316,7 +316,7 @@ def test_attribute_check_with_hasattr(self):
self.assertFalse(hasattr(Profile(), "__password__"))


if os.getenv("RUN_MYSQL_DATABASE", None).lower() == "true":
if os.getenv("RUN_MYSQL_DATABASE", "false").lower() == "true":
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(my testing environment wasn't set up, so this caused an error, saying that lower is not a function on NoneType)

@@ -52,7 +52,7 @@ def __get__(self, instance, owner):
object -- Either returns a builder or a hydrated model.
"""
attribute = self.fn.__name__
relationship = self.fn(self)()
relationship = self.fn(instance)()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

owner appears to be the class, User in my example, whereas instance is an instance of owner. self.fn is a pointer to an instance method (Ruby vernacular) or prototype function (Node) -- apologies, I don't know the corresponding term in Python -- so I believe instance is correct

@pmn4 pmn4 requested a review from josephmancuso March 1, 2024 15:52
Copy link
Member

@josephmancuso josephmancuso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine but i need more time to test this change

@josephmancuso josephmancuso merged commit c9e7617 into MasoniteFramework:2.0 Apr 7, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants