pop icon indicating copy to clipboard operation
pop copied to clipboard

belongs_to fk_id doesn't match has_one fk_id

Open trevrosen opened this issue 6 years ago • 7 comments

The format for struct tag values used to designate an alternative foreign key are different in a has_one relationship vs a belongs_to relationship.

  • In a has_one, the fk_id struct tag references the value of the db struct tag on the struct field that holds the foreign key value. So if the foreign key struct field's struct tag is db:"creator_id", the struct tag for specifying a nonstandard foreign key will be like fk_id: "creator_id"
  • In a belongs_to, the fk_id struct tag references the stringified name of the struct field. I.e. if the struct field name is CreatorID, the struct tag is like fk_id:"CreatorID" (implemented in this PR)

IMO, the belongs_to behavior is not intuitive. I'm curious why this decision was made and if indeed there's just no way to have the two behaviors match for some reason.

cc @u007

trevrosen avatar Jun 15 '18 20:06 trevrosen

@markbates ive concern regards to fk_id using actual field name because if we use that, we may not be able to point it back to the field of the current class. if we tried to get the field name by camel case the field name, it might not turn out to be the actual field. if we use fk_id and point to go attribute name, we can then obtain the actual field name via tag: db what do you think? shall we remain this key to be attribute name and change has_one to use the same?

in ruby on rails, it is okay to use snake based name because the ruby class will have the same attribute name as the actual field.

this will be quite a breaking change.

u007 avatar Jun 19 '18 03:06 u007

@u007 I’m probably the wrong person to weigh in here since I’m a) traveling and b) don’t quite understand the problem. :)

@stanislas-m and @larrymjordan could either of you jump in on this?

markbates avatar Jun 19 '18 15:06 markbates

okay the problem is, has_one fk_id is using actual table field name, while belongs_to fk_id is using class attribute name, i was thinking to standardise this, but i cannot change fk_id for belongs_to to use table field name

u007 avatar Jun 19 '18 16:06 u007

I should clarify a bit: I'm just hoping for consistency. Whether it's named for the db column or the struct field is an implementation detail from my POV. I'd personally be in favor of whatever makes Pop work better internally, but I do feel strongly that having these behaviors match honors the principle of least surprise.

trevrosen avatar Jun 19 '18 16:06 trevrosen

we can only find ways to turn has_one to use attribute name as well, but this will break existing functionality

u007 avatar Jun 24 '18 15:06 u007

Why do we need to access the field in belongs_to but not in has_one?

If it's required, I'm not against refactoring fk_id to use the field instead. To ensure back-compatibility, we can try to get the field first, then fallback with a warning on the db column.

stanislas-m avatar Jun 29 '18 15:06 stanislas-m

This is still quite confusing and not very well documented. Is a fix for this to be anticipated (seeing that the issue is over a year old)? I think the behavior should be consistent. I have to constantly look up this issue to figure out which way is which.

aeneasr avatar Dec 18 '19 14:12 aeneasr