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

Has_one relationship #298

Closed
tpow opened this issue Jan 6, 2021 · 4 comments · Fixed by #299
Closed

Has_one relationship #298

tpow opened this issue Jan 6, 2021 · 4 comments · Fixed by #299
Labels
feature request A feature that does not yet exist but will be a good addition to the library medium These issues are geared for people who have contributed to the project before

Comments

@tpow
Copy link

tpow commented Jan 6, 2021

I believe that has_one is helpful and should be implemented. Although it behaves similarly to belongs_to, it has a semantic difference, and I find it more readable, and informative in well designed models and databases. It should indicate where the foreign key is placed. Generally, belongs_to should be used in the model that has the foreign key and has_one should be in the other.

I believe part of the confusion with the key order (when using has_one in the predecessor Orator ORM) is due to code that misuses has_one and belongs_to and therefore needs to specify both keys. When using the decorators with a well designed database that has id columns in tables and <other>_id columns when reference other tables you would ideally not need to specify any keys or at worst the opposite key. This is why has_one has the other key first -- that is generally all you need to provide because the local key of id can be assumed (and hopefully omitted).

That said, I'm not too stuck on the key order, although I do think it is logical. It is handy to allow specifying a single key in some cases. Perhaps it could use keyword arguments.

Here's an example:

class Employee(Model):

  @has_one('employee_id', 'id')  # in other_key, local_key order.
  # Ideally, @has_one is all that's needed. This works with Orator ORM.
  # It should assume the foreign key is employee_id based on the model name.
  def office(self):
    from app.models import Office
    return Office

class Office(Model):
  @belongs_to('employee_id', 'id')  # in local_key, other_key order
  # Ideally, @belongs_to is all that is needed (and works in this case with Orator)
  def employee(self):
    from app.models import Employee
    return Employee

I just reviewed my code to make sure I was using belongs_to and has_one consistently as I described in this issue and discovered that I wasn't in a few cases. Once I made corrections, I have relatively few usages of has_one. I should note that they are all using the @has_one decorator with no keys specified. Orator ORM made the correct assumptions about the keys. In order to migrate to Masonite ORM it would be a shame to have to use belongs_to losing the semantic meaning and needing to specify the keys.

@josephmancuso
Copy link
Member

I wouldn't be against adding a has_one. I think its pointless. I've seen the creator of Laravel say that he's never even used the has_one relationship before. And I personally never have either. I can see the benefit of semantics but if its semantics alone you can just alias the import:

from masoniteorm.relationships import belongs_to
from masoniteorm.relationships import belongs_to as has_one

@josephmancuso josephmancuso added the feature request A feature that does not yet exist but will be a good addition to the library label Jan 6, 2021
@josephmancuso josephmancuso added the medium These issues are geared for people who have contributed to the project before label Jan 8, 2021
@tpow
Copy link
Author

tpow commented Jan 8, 2021

Wow, that was super fast. I appreciate it! This will make the transition to the new ORM easier.

Tim

@josephmancuso
Copy link
Member

Good lol. I built the ORM in a way where its super fast to fix issues and make new features so its good someone else noticed lol. I just need to do some more testing before I merge it in and release it

@josephmancuso
Copy link
Member

This is added in version 1.0.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request A feature that does not yet exist but will be a good addition to the library medium These issues are geared for people who have contributed to the project before
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants