Fix “Authorized applications” inefficiently and incorrectly getting last use date

[ClearlyClaire]

Fixes #20477

Thanks @renchap for writing the more efficient query.

[renchap]

👍

[mjankowski]

I realize this is more than a year old, but bumped into this looking at something else. In this query, this line (the DISTINCT portion) doesnt actually come through in the generated sql.

I think what happens is the pluck overwrites the select line here, so the distinct portion of query gets dropped ... but then also, the final to_h essentially does what we want in terms of preparing a hash with unique keys (application IDs) each pointing to a datetime value for the view where this is used to consume.

That said, I'm not sure that the correct time gets picked. Example here...

mastodon(dev)>     @last_used_at_by_app = Doorkeeper::AccessToken
mastodon(dev)>                            .select('DISTINCT ON (application_id) application_id, last_used_at')
mastodon(dev)>                            .where(resource_owner_id: current_resource_owner.id)
mastodon(dev)>                            .where.not(last_used_at: nil)
mastodon(dev)>                            .order(application_id: :desc, last_used_at: :desc)
mastodon(dev)>                            .pluck(:application_id, :last_used_at)
mastodon(dev)>                            .to_h
mastodon(dev)> 
  Doorkeeper::AccessToken Pluck (0.2ms)  SELECT "oauth_access_tokens"."application_id", "oauth_access_tokens"."last_used_at" FROM "oauth_access_tokens" WHERE "oauth_access_tokens"."resource_owner_id" = $1 AND "oauth_access_tokens"."last_used_at" IS NOT NULL ORDER BY "oauth_access_tokens"."application_id" DESC, "oauth_access_tokens"."last_used_at" DESC  [["resource_owner_id", 3]]
=> {3=>Sat, 09 Nov 2024 17:46:10.071951000 UTC +00:00}
mastodon(dev)>     @last_used_at_by_app = Doorkeeper::AccessToken
mastodon(dev)>                            .select('DISTINCT ON (application_id) application_id, last_used_at')
mastodon(dev)>                            .where(resource_owner_id: current_resource_owner.id)
mastodon(dev)>                            .where.not(last_used_at: nil)
mastodon(dev)>                            .order(application_id: :desc, last_used_at: :desc)
mastodon(dev)>                            .pluck(:application_id, :last_used_at)
mastodon(dev)> 
  Doorkeeper::AccessToken Pluck (0.2ms)  SELECT "oauth_access_tokens"."application_id", "oauth_access_tokens"."last_used_at" FROM "oauth_access_tokens" WHERE "oauth_access_tokens"."resource_owner_id" = $1 AND "oauth_access_tokens"."last_used_at" IS NOT NULL ORDER BY "oauth_access_tokens"."application_id" DESC, "oauth_access_tokens"."last_used_at" DESC  [["resource_owner_id", 3]]
=> [[3, Thu, 14 Nov 2024 17:38:12.812377000 UTC +00:00], [3, Sat, 09 Nov 2024 17:46:10.071951000 UTC +00:00]]

Note how the first one (copied straight from here, including final to_h) does get us a uniquely-keyed hash, the second one (exact same, drops final to_h) shows the two values we could have chosen. Presumably we would have wanted the latter (11-14 in this example, not 11-09) value to be chosen?

If I remove the pluck line, the DISTINCT bit does run, and the one that comes back has been uniqued/ordered correctly (its the 11-14 one from my example).

Let me know if that analysis makes sense or I'm missing something here ... if it's a bug I'll take a stab at resolving.

[ClearlyClaire]

Very good catch!

I guess we could remove the select and rewrite the pluck as follows:

@last_used_at_by_app = Doorkeeper::AccessToken
                           .where(resource_owner_id: current_resource_owner.id)
                           .where.not(last_used_at: nil)
                           .order(application_id: :desc, last_used_at: :desc)
                           .pluck(Arel.sql('DISTINCT ON (application_id) application_id, last_used_at'))
                           .to_h

[mjankowski]

Cool, thanks for confirming -- will attempt to fix, and may move this logic elsewhere in the process (it feels weird to add a controller spec or view spec for this ... maybe move to method on user or something to test)