avram

avram copied to clipboard

https://gitter.im/luckyframework/Lobby?at=5cd5dd6f79d70050994c8059

class Posts
  table :posts do
    # No conditions
    has_many comments : Comment
    # Only return public comments related to this post
    has_many public_comments : Comment, filter: CommentQuery.new.public(true)
  end
end

@jwoertink @edwardloveall. Thoughts on what to name this option? Is condition good? Maybe scope_with, scope, or filter?

has_many public_comments : Comment, filter: CommentQuery.new.public(true)

I kind of like filter

I kinda like scope. Though, does this really belong in the model? Couldn't you do something like this?

class PostQuery < BaseQuery
  def public_comments
    comments.public(true)
  end
end

You also might consider where although it might become overloaded because of SQL.

I think filter is good. I don't like scope because it has baggage for me (sorry @jwoertink!)

I think I agree with @jwoertink on where this should be placed. Seems better to keep all this logic in the query and keep the models light.

The main reason for this was use in preloads it can get a little tedious to specify those conditionals every time. It also makes it impossible to preload two different kind of associations. Like if you wanted to show the public comments in one spot and archived comments in another spot on the page. You can't do two preload_comments. This would allow you to do it pretty easily:

PostQuery.new.preload_comments.preload_public_comments

I'd say that's the only reason to use those filters. For preloading. You could still use methods on the query that can be used in the scope/filter/condition/whatever

has_many  comments : Comment, filter: CommentQuery.new.newest_first.public.something_else

There is a bit more context in the linked Gitter conversation where someone was doing a join. With that said maybe it isn't worth it since it is probably fairly uncommon to need to preload has_many with different scopes. On the flip side if you need to do that, it's pretty much impossible right now

Ah. I always forget about preloading. That does make sense. I could see needing to do that pretty often in an admin interface sort of deal. With that said, I think filter is fine.

Would there be any pitfalls to doing this? My brain isn't fully on yet, but my initial thought is something similar to the rails scope methods where those queries were ran at boot time as opposed to class methods that were ran at run time which meant (in the case of rails) that you could run in to some weird edge cases of queries being wrong....

Ahh good catch. Yes I think it could run into issues. We should make sure it is always a Proc so that it runs at runtime and not just once at compile time:

has_many recent_comments : Comment, filter: -> { CommentQuery.new.created_at.gte(1.day.ago) }

Alternatively we could wrap it in a Proc inside the macro but we can figure that out during implementation phase and just do what is easiest

Just came up with a new case for this. It's still going to be super tricky to handle due to the preloading edge cases deal states in #584 but...

If you have an association that uses SoftDelete, the association method doesn't take that in to consideration and neither does the counter helper method.

user.followers.size #=> 2
user.followers_count #=> 2
# assume follower was soft deleted
DeleteFollower.destroy(follower, user: user) do |o, u|
end
user.followers.size #=> 2
user.followers_count #=> 2

This is because when you include soft deletes, it's added to the subclassed query class, but we query off the BaseQuery. We would include the module in FollowQuery, but then the association is queried on Follow::BaseQuery.

I think even if we can just get the query class in there, it would make a huge difference.

has_many followers : Follow, query: FollowQuery

I'm just thinking the query class possibly to start because you can set defaults, and have as many different query classes as you needed.

class GoldTokenTransactionQuery < Transaction::BaseQuery
end
class SilverTokenTransactionQuery < Transaction::BaseQuery
end

class User < BaseModel
   table do
    #...
    has_many gold_tokens : Transaction, query: GoldTokenTransactionQuery
    has_many silver_tokens : Transaction, query: SilverTokenTransactionQuery
   end
end

In either case, it'll probably still run in to the same preload issue 😝 @matthewmcgarvey maybe you have some ideas on how to handle this?

We have something that might be related already https://github.com/luckyframework/avram/blob/5e29f75371dca5a2bc16858163dc3790cabfbfcb/src/avram/model.cr#L182-L194

It's not used in many situations but it could be a start for something like that

While we don't support Rails' style of polymorphic associations, with being able to specify conditions, you could do it in one way.

Take file attachments for example:

You could have a table called file_attachments with the columns

• name
• attached_to_type
• attached_to_id
• attachment_url (url to file in s3)

You wouldn't be able to go from the attachment to the record but you could go the other way

class User < BaseModel
  table do
    # ...
    has_one profile_picture : FileAttachment,
              query: FileAttachmentQuery.new.attached_to_type(User.name),
              foreign_key: :attached_to_id
  end
end

Another option would be to allow providing a block that the query would be passed into. We could a named arg for specifically passing in the query class as well (which may be a half way step because you could make special query classes for whatever would have been put in the block).

Spent a few minutes trying to figure out if this was implemented yet (I must have hallucinated working on this) but alas. My use case this time was to have a has_many and a has_one because the first record in the has_many is meaningful and I wanted to make sure has_one used an order by id to make sure I always got the right record.

Anyways, this is still a good idea and somebody should work on it.