Association improvements

[c910335]

Breaking Changes

1. Define getter based on the nilability like column.
   • getter and getter? for not nil type; getter! and getter for nilable type
   • only for belongs_to and has_one (since has_many simply returns empty collection if not found)
2. Cache associations.
   • #reload_{{name}} to reload
   • another reload option#{{name}}.reload for has_many

[drujensen]

I am finding this very confusing.

If I declare the type as nilable (using a question mark), then I get the default name as nilable and an extra getter name! as not nilable.

If I declare the type as not nilable (without a question mark), then I get the default option name that is not nilable and a name? that is nilable.

Is this what you would expect?

Maybe we always include a ? option as nilable and a '!' as not nilable for both cases. The default would be based on the declaration, but you could override the default by using these options. wdyt?

[c910335]

This design came from field and field! in #181, and they were merged in #346.
Associations should follow the similar syntaxes.
I think getter! for not nil type and getter? for nilable type are just redundant.

[drujensen]

I see, I agree they are redundant, but easy to remember. I guess if you get an error, you will remember that you declared it one way of the other.

[Blacksmoke16]

Related to #370. I'm fine with this for the mean time as it fits with the current setup. However, would like to reach a decision on that PR, which makes things more crystal like id argue versus having to do field! for not nilable.

[Blacksmoke16]

I'm not sure how I feel about the caching side of this yet. I don't like that the ivar is hidden away and can't be altered, via annotations for example. I'm also not sure about this being the default behavior. I thought about caching a while ago and my thought was the query itself should be cached, so that an extra ivar doesn't have to be added to the model.

EDIT: I'd also like to see some sort of interface around how caching should be done, where this would be an implementation of that, like InMemoryCache. Other examples could include like a Hash, or Redis, etc.

[c910335]

@Blacksmoke16 Another purpose of this PR is to solve the N + 1 problem, which cannot be solved by caching the query.
There may be another better approach, but the best way I came up with is this.

[Blacksmoke16]

There may be another better approach, but the best way I came up with is this.

I'd rather hold off on this change and discuss alternative solutions. In its current state this PR would not play well with any third party serializer. If anything it should be an opt in feature via a module or something.

[drujensen]

@Blacksmoke16 I see your concern regarding serialization and agree that we should find another alternative approach.

@c910335 I am wondering if we need the parent query to include the children records. Would that solve the N+1 and allow the db to perform caching?

[c910335]

@drujensen
Something like this.

SELECT * FROM `children` WHERE `children`.`parent_id` IN ('2', '3', '5', '7', '11');

Then assign each child to the instance variable of its parent in Crystal.

[c910335]

Another benefit of this PR is that it can return deleted associations if you cache them before deleting.

[c910335]

Just added .includes for belongs_to and has_one that solves the N + 1 problem. (It's more complicated for has_many)
Let's discuss whether to add this feature.

[Blacksmoke16]

I like the feature, but not the implementation:

1. There is no way to opt out.
2. There is no way to access the cache property, such as for validation/serialization etc.
3. I think it would be better to not make the model itself the cache. For example have some other type that can cache the query/result. Then allow a query to check the cache first. This would also make it so the cache could be various things Hash(String, Granite::Base), Redis, memcached, etc. However I fear this is going to not be trivial without some rethinking/reworking of how Granite works internally.
4. It would also nice to decide upon Refactor initialization #370, as that would affect some of the other nilable/non-nilable methods that are defined.

[drujensen]

Closing this until a better alternative for caching is implemented.