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

Allow explicit FK fields for inverse relations #237

Open
josephbuchma opened this issue Sep 24, 2017 · 8 comments
Open

Allow explicit FK fields for inverse relations #237

josephbuchma opened this issue Sep 24, 2017 · 8 comments

Comments

@josephbuchma
Copy link

josephbuchma commented Sep 24, 2017

From readme:

Foreign keys do not have to be in the model, they are automagically managed underneath by kallax.

AFAICT there is no way to get fk value without querying relation. And when I include foreign key to struct, kallax generates invalid code.

type Post struct {
  kallax.Model
  AuthorID int64
  Author *User `fk:",inverse"`
  // ...
}

Kallax generates duplicate switch statement cases for "author_id" field in func (r *Post) ColumnAddress and func (r *Post) Value methods.

UPDATE
And by the way, how to load "inverse" relations for object without re-querying everything using With<Relation> methods ? This "virtual columns" seem to just add more complexity and inconveniences without providing reasonable value.

@roobre
Copy link
Contributor

roobre commented Sep 28, 2017

You can query FK values with:

post.Value(Schema.Post.AuthorFK.String())

But is a bit ugly indeed. There is no easy way to access them because when you use an ORM, you usually want to hide that complexity. That's a design decision I don't agree with, tho.

@josephbuchma
Copy link
Author

josephbuchma commented Sep 28, 2017

I'd rather consider this as extra complexity:

post.Value(Schema.Post.AuthorFK.String())

And yes, it's ugly. Option to allow using struct fields for FKs would be awesome (and backward compatible).

What about loading relations on existing (already fetched) object? I mean something like

p := PostStore(db).FindOne(NewPostQuery().FindByID(id)
// something happen and suddenly you also need to load author
p.LoadAuthor() // <- how to do this without using p.Value(Schema.Post.AuthorFK.String())?

@roobre
Copy link
Contributor

roobre commented Sep 28, 2017

You can use the same trick:

authorId, err := post.Value(Schema.Post.AuthorFK.String())
// TODO: check err
authorStore.FindOne(NewAuthorQuery().FindByID(int64(authorId)))

int64 or whatever type your ID field has.

@josephbuchma
Copy link
Author

without using p.Value(Schema.Post.AuthorFK.String())?

Anyway, looks like you have no idea either...
"Hiding" FKs is not a bad idea. But not allowing to explicitly adding them to struct is definitely bad one.

@roobre
Copy link
Contributor

roobre commented Sep 28, 2017

Oh, sorry, I totally missed the comment. I'm afraid it is not possible to do that without asking the schema for stuff. This is by design, changing this behaviour would require a major refactor of how virtual columns and FKs work.

@roobre
Copy link
Contributor

roobre commented Nov 22, 2017

Status update

This is indeed a desired feature. However, adding an extra field named WhateverID feels quite ugly to me. FKs should be exposed read-only, explicitly via a new API. Something in the lines of post.GetFK(Schema.Author).

This is certainly on my TODO list.

@roobre roobre self-assigned this Nov 22, 2017
@josephbuchma
Copy link
Author

@roobre sometimes you can assign FK without fetching full related records, in such cases read-only API may lead to redundant work.

@roobre
Copy link
Contributor

roobre commented Nov 24, 2017

Maybe we can cover that on #219.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants