steebchen
Consider returning an error from relationship function calls instead of panicking
Currently, relationship methods on a model will panic if such a method is called without the query containing a With clause for the relationship:
func (f Foo) Bars() (value []BarModel) {
if f.RelationsFoo.Bars == nil {
panic("attempted to access bars but did not fetch it using the .With() syntax")
}
return f.RelationsFoo.Bars
}
@steebchen and I discussed whether it would make more sense for these methods to return an error instead of panicking. While users should be encouraged to fix an issue like this in development, it's impossible for someone writing a helper function to know ahead of time that users will write their queries correctly before calling the helper. Additionally, having an error in the signature alerts users to the fact that they need to perform some steps ahead of time to ensure the function returns correctly.
Opening this issue for consideration, since this would be a breaking change.
An open question:
The current signature for optional fields is (v, ok). What should we do about that? Return an error ErrNull?
The current signature for optional fields is
(v, ok). What should we do about that? Return an errorErrNull?
I would argue to keep this the way it is. If the end-user has already marked the field as optional, then the value being undefined is an expected outcome and not really an error.
I want to prevent the case of returning nil every time even though the user didn't fetch for it. Although I see your point.
However, then I think you might argue that instead of returning an error, we might just a nil pointer for required fields instead of erroring? The advantage of that being that it won't break existing users syntax-wise and also doesn't bloat the API with the extra error check. Or do you think it should still return an error in that case?
If I understand correctly, you're suggesting that the existing API would stay the same, and there would be no panic, and instead the relation method would just return nil? If that's what you're suggesting, I think that's a reasonable change.
I ran into this too just now, what one can do is to just check the data first before accessing the data, so that it can't panic
if portfolio.RelationsPortfolio.Transactions != nil {
transactions = polar.ExpandTransactions(c, portfolio.Transactions())
}
I just ran into this as well, the panic is definitely unexpected behavior.
It could be nice to have a method Has<Relation>, for example HasTransactions() bool, to check whether they've been populated. I think the caller shouldn't have to be aware of the Relations field in order to check whether the relation has been fetched.
Otherwise returning nil for unfetched relations makes sense to me as well.
