laravel-doctrine / fluent

Fluent mapping driver for Doctrine2
http://www.laraveldoctrine.org/docs/current/fluent
MIT License
43 stars 22 forks source link

hasOne / belongsTo mapping fails schema validation #58

Closed matt-allan closed 6 years ago

matt-allan commented 6 years ago

Hello 👋

I can't seem to get the hasOne and belongsTo mapping to pass schema validation. If I run php artisan doctrine:schema:validate --skip-sync it gives me an error like this:

* If association App\Domain\User\Settings#user is many-to-one, then the inversed side App\Domain\User#settings has to be one-to-many

It seems like the problem is that hasOne returns a OneToOne and belongsTo returns a ManyToOne? Should hasOne return a OneToMany instead?

I've worked around it in the interim by using oneToOne instead.

I added a test case that illustrates the issue here: https://github.com/yuloh/fluent/tree/hasOne-mapping-issue

Specifically the error I am getting is similar to this one:

If association Tests\Stubs\Entities\StubEntity#parent is many-to-one, then the inversed side Tests\Stubs\Entities\StubEntity#parent has to be one-to-many.
patrickbrouwers commented 6 years ago

Hey @yuloh

hasOne mirrors what hasOne does in Eloquent. (https://laravel.com/docs/5.6/eloquent-relationships#one-to-one) It's correct that it returns a oneToOne.

belongsTo correctly returns a ManyToOne (just like Eloquent).

You don't have to use hasOne and belongsTo if the terms are confusing. They were only added to make the transition from Eloquent easier. Using oneToOne or manyToOne directly is perfectly fine.

matt-allan commented 6 years ago

Hi @patrickbrouwers,

My issue isn't that the terms are confusing; it's that they cause schema validation to fail. If you check out the branch I created and run the tests you can look at the error being reported by doctrine.

Specifically it seems that doctrine thinks that a many-to-one can only be mapped to a one-to-many. Because hasOne is a one-to-one relationship and belongsTo is a many-to-one it causes an error.

The code seems to run fine but it reports an error when using the doctrine:schema:validate command.

patrickbrouwers commented 6 years ago

You should build your mapping just as you would do with any other driver. Fluent is just an alternative syntax to Doctrine's mapping api. In the end it's the user's choice if he uses a one to one or a many to one and make sure the schema is valid. There's nothing Fluent can and should do about that.

Looking at your example:

If association App\Domain\User\Settings#user is many-to-one, then the inversed side App\Domain\User#settings has to be one-to-many

You should have

$user->hasMany(Setting::class)->... (or oneToMany)

and

$setting->belongsTo(User::class)->... (or manyToOne)

It's not Fluent that makes your schema invalid, but your choice of relations.

patrickbrouwers commented 6 years ago

If you actually want to do a one-to-one you should follow doctrine's guidelines for one-to-one (and ignore how Eloquent does it): http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/association-mapping.html#one-to-one-unidirectional

$user->hasOne(Setting::class)->... (or oneToOne)

and

$setting->hasOne(User::class)->... (or oneToOne)

matt-allan commented 6 years ago

Are you saying that hasOne and belongsTo should not be used for one-to-one relationships?

The point I was trying to make is that if you do create a one-to-one relationship using hasOne and belongsTo Doctrine's schema validation will always fail.

The reason I filed a bug report is the fluent docs state:

hasOne Inspired by Laravel's relation syntax, this method maps a one-to-one inverse side relation. belongsTo Inspired by Laravel's relation syntax, this method maps the owning side of a one-to-one relation.

It sounds like you should be able to use hasOne and belongsTo to map a one-to-one relationship, and while it does work, it causes schema:validate to throw an error. The error is not because of my database schema; I am using --skip-sync. The error is because the mapping is considered invalid by doctrine.

patrickbrouwers commented 6 years ago

@guiwoda can you have a look at this?

matt-allan commented 6 years ago

I pushed an example app to illustrate this issue: https://github.com/yuloh/lumen-app

It isn't clear if mappedBy and inversedBy are necessary for a one-to-one with these methods. If I leave them off I get this error:

$ php artisan doctrine:schema:validate --skip-sync

Validating for default entity manager...
[Mapping]  FAIL - The entity-class 'App\Entities\Scientist' mapping is invalid:

* The association App\Entities\Scientist#profile refers to the owning side field App\Entities\Profile#profile which does not exist.
Database] SKIPPED - The database was not checked for synchronicity.

Once they are added it returns this error:

$ php artisan doctrine:schema:validate --skip-sync

Validating for default entity manager...
[Mapping]  FAIL - The entity-class 'App\Entities\Profile' mapping is invalid:

* If association App\Entities\Profile#scientist is many-to-one, then the inversed side App\Entities\Scientist#profile has to be one-to-many.
Database] SKIPPED - The database was not checked for synchronicity.

I also added a test to illustrate that it does still work correctly.

guiwoda commented 6 years ago

@patrickbrouwers @yuloh Yes, indeed this is correct. The SchemaValidator expects a ONE_TO_ONE <> ONE_TO_ONE mapping, or a ONE_TO_MANY <> MANY_TO_ONE mapping. We are doing ONE_TO_ONE <> MANY_TO_ONE because on the implementation side, it doesn't matter. But the SchemaValidator is more strict than the implementation itself.

To allow a generic belongsTo semantic, we should parse the inverse side and decide to turn belongsTo into either oneToOne (owner) or manyToOne, just to please the SchemaValidator. In any case, you can avoid the belongsTo method if you need the validator to work.