last method always returns a hash

[gizotti]

I have the following repository:

class PostRepository < Hanami::Repository
  def newest_post
     root.order(:created_at).as(:entity).last
  end
end

The newest_post method will always return a Hash instead of an Post instance.
If I change last to first, I get a Post instance as I want.

I have no clue where to start looking for the problem. If someone could give me a clue, I can try to fix this one myself.

[davydovanton]

@gizotti confirmed. I have this bug too 👍
Also, I founded that lasts returns hash only with where method, e.g:

?> UserRepository.new.last
=> #<User:0x007fad6a392550 @attributes={:id=>2, :name=>"Anton Davydov", :login=>"banned_davydovanton", :email=>"mail@davydovanton.com", :avatar_url=>"https://avatars.githubusercontent.com/u/1147484", :bio=>""}>

>> UserRepository.new.users.where(id: 2).last
=> {:id=>2, :name=>"Anton Davydov", :login=>"banned_davydovanton", :email=>"mail@davydovanton.com", :avatar_url=>"https://avatars.githubusercontent.com/u/1147484", :bio=>""}

[gizotti]

@davydovanton do you know where I can start looking for it? I'd love to dive into this one.
However, if someone already know exactly where to fix it and can get that done faster than me thats ok for me too.

[davydovanton]

@gizotti it's a problem of ROM::Repository::RelationProxy class and rom-repository project. I created PR with reproducing this problem.

/cc @solnic, @flash-gordon and @AMHOL

[solnic]

This won't be fixed in rom because it's not a bug in rom. See here for more info.

[davydovanton]

@solnic oh, sorry it's my mistake 😞 Thanks for explanation 👍

@jodosha, @hanami/documentation I think we need to update documentation for this case. WDYT?

[solnic]

FWIW I don't think that having first and last is a good practice, I know it's common because it exists in Array and popular ORMs provide it too, the problem is that you can't really tell what the order is, and you can't possibly expect to get consistent results when order is not enforced in any way. Personally I see this is as a weird antipattern that we've happily adopted :(

[gizotti]

@solnic fair enough :) Thanks for the reply.

@davydovanton @jodosha, would it be worth implementing a "fix" on hanami-model? Or because of how it depends on rom-repository that wouldn't be possible?

[davydovanton]

@solnic I understand right that now you prefer to use something like #to_a with #last/#first for solving this problem?

[solnic]

@davydovanton we have one and one! so:

users.one # returns first struct based on default by-pk sorting
users.reverse.one # returns last struct based on default by-pk sorting

[jodosha]

@solnic

FWIW I don't think that having first and last is a good practice, I know it's common because it exists in Array and popular ORMs provide it too, the problem is that you can't really tell what the order is, and you can't possibly expect to get consistent results when order is not enforced in any way. Personally I see this is as a weird antipattern that we've happily adopted :(

You're right, Repository#first and #last were introduced for convenience and to make the transition from AR more soft.

---

But here we have another problem. The line root.order(:created_at).as(:entity) returns a ROM::Relation::Composite which applies the :entity mapping only when #to_a is triggered, but not when #last is invoked.

---

To recap: Repository#first and #last can be considered an antipattern, but how one can pick the last record of a tuple?

---

For me root.one raises an error ROM::TupleCountMismatchError: The relation consists of more than one tuple is there anything that we should add in Hanami::Model to prevent this error?

Thanks 😄

[solnic]

@jodosha yeah you could just do this:

def first
  root.limit(1).one
end

def last
  root.limit(1).reverse.one
end

This will make sure you only fetch a single tuple, and it uses default ordering by pk.

[jodosha]

@solnic Thanks for the quick reply. We'll document this.

[jodosha]

Cross referencing rom-rb/rom-sql#64

[cllns]

We're working on documenting this, but I wonder if we can't do something in the code to help avoid this. Before 1.0.0.rc1, should we do any of the following:

1. remove first and last from Hanami::Repository altogether? Instead we could encourage limit & one (and, optionally, order). @solnic mentioned considering removing first and last from rom-sql, and if we can be ahead of this change, it could be good :) If so, should combine with 3.

2. change first implementation in Hanami::Repository to root.limit(1).one (and last to root.limit(1).reverse.one), to avoid the Hash/entity issue last method always returns a hash #380 (comment). Not sure this would work when chaining other methods.

3. add deprecation warnings to first and last, discouraging their use, in favor or limit and one?

(I originally wrote this comment on the documentation fix, but it belongs here instead)

If it's too late for any of these, that's fine. Just thought I'd bring it up :)
@hanami/core @jodosha @solnic

[gizotti]

@cllns IMO the second would be the way to go. first and last are convenient methods to quickly access that data.

If we remove those methods we would just end up making people write their own first and last methods when they are implementing their own repos.

Now, when they are writing their own queries inside their repos I think we should enocurage limit and one over first and last.

Does that make sense?

[cllns]

Yes, that makes sense @gizotti. Thanks for your feedback. I'm not sure of how the internals work here, so we'll @jodosha's input. I'm not sure if 2) is easily implementable. Also we might not be able to get any changes in to v1.0, but we'll see 🤞

We're interested in hearing what others think is the right course of action as well :)

[davydovanton]

@cllns about the second point: I think we need to use #one! here, WDYT?

[cllns]

Maybe! We can discuss, if that option is feasible and we decide to go for it :)

[jodosha]

Folks, I went straight with the refactoring regarding Repository#first and #last, as you suggested. See 5737794

The reason why this is a good idea is simple: it removes Sequel from the implementation of #last. See the commit diff.

---

But please note that this GH issue is NOT about Repository#first and #last, as they already return entities by default.

This is about a ROM::Repository::RelationProxy don't returning an entity with #last by ROM design.

Now the "fix" for this is documenting the use case in the description of this PR: fetch the most recent post (as instance of Post).

[jodosha]

@cllns I assigned this to you as you're working on the documentation for this. Can we get a PR out in a couple of days? Thank you!

[mereghost]

Any progress on this, @cllns ?

[Mehonoshin]

Guys, I'm getting started with hanami, and this issue confused me very much.
We need to document it properly.
I guess everyone who switches from Rails - will meet this.

Does anyone work on it? I can do, if there are no volunteers.

[cllns]

@Mehonoshin Sorry you ran into this issue. Yes, please open a PR :) <3

[solnic]

FYI we started working on rom-4 and I already ported auto-map/struct feature from repository to rom core and now this works consistently:

[5] pry(main)> users = rom.relations[:users]
[6] pry(main)> users.with(auto_struct: true).first
=> #<ROM::Struct::User id=1 name="Joe Dane">
[7] pry(main)> users.with(auto_struct: true).last
=> #<ROM::Struct::User id=1 name="Joe Dane">

This means that hanami will be able to enable auto-map/struct for its relations and it'll work consistently.

Furthermore, you'll be able to rely on built-in rom structs, as we added a feature where you can provide your own namespace and base struct class, which will be used to infer structs automatically and use your struct class as the base. You'll be able to either rely on inferred attributes, or define them explicitly, up to you.