Allow self-referential BelongsTo relations #55
Conversation
| assert_ne!(belongs_to.pair, FieldId::placeholder()); | ||
| if model.id != belongs_to.target { | ||
| assert_ne!(belongs_to.pair, FieldId::placeholder()); | ||
| } |
There was a problem hiding this comment.
I wasn't really sure what this assert is protecting against so gated it behind the same check. There's likely a better solution.
There was a problem hiding this comment.
Is this assert being hit? pair should never be equal to FieldId::placeholder() once the schema is built. If the assert is being hit there is probably a bug.
There was a problem hiding this comment.
Hi! Thanks for submitting the PR. Sorry for the review delay. I was on vacation.
I agree that supporting self-referential belongs to is needed. I have a few requests before this gets merged.
First, instead of an example, could you add a test in the tests/client directory?
For the implementation strategy, I left an inline note, but I am wondering if having value in BelongsTo<T> always be boxed would be a better idea. It would simplify the code gen (avoiding having to conditionally box the field) and the structure sizes would be smaller.
what do you think?
| #name: BelongsTo<#ty>, | ||
| if self.model.id == rel.target { | ||
| quote! { | ||
| #name: Box<BelongsTo<#ty>>, |
There was a problem hiding this comment.
Interesting, it might be worth always allocating in BelongsTo, especially since the structs might be big and not always loaded.
| assert_ne!(belongs_to.pair, FieldId::placeholder()); | ||
| if model.id != belongs_to.target { | ||
| assert_ne!(belongs_to.pair, FieldId::placeholder()); | ||
| } |
There was a problem hiding this comment.
Is this assert being hit? pair should never be equal to FieldId::placeholder() once the schema is built. If the assert is being hit there is probably a bug.
The BelongsTo has no indirection so for self-referential relations fails to compile due to an infinitely sized struct. This checks if the model and the relation share the same target and, for BelongsTo relations, adds a Box to the property type.