text:
SpeciesList + QueryRecord — Rewrite SQL -> Arel

Tiny PR. See comments for discussions about converting these to AR.
update_all-type method easier to convert than delete_all method.
Two models affected: SpeciesList and QueryRecord

Coverage increased (+0.003%) to 93.034% when pulling 57014ef105b9a2df07f4ea14777fbb4c68fd6c80 on nimmo-SQL-in-models-one-by-one into 131a79a4360b91f16716591f4c04971bcdf65036 on master.

Ok, that's all for this PR. Seems still small enough to fit under the 15-minute mark :)

Why is table_name a variable?  It's always going to be "query_records".
On Sat, May 7, 2022 at 8:58 PM andrew nimmo @.***>
wrote:

@.**** commented on this pull request.
In app/models/query_record.rb
https://github.com/MushroomObserver/mushroom-observer/pull/916#discussion_r867419560
:

@@ -15,12 +19,28 @@ def self.cleanup
(@@last_cleanup < 5.minutes.ago) ||
::Rails.env.test?

connection.delete(%(

 DELETE FROM #{table_name}

 WHERE

   access_count = 0 AND updated_at < DATE_SUB(NOW(), INTERVAL 6 HOUR) OR

   access_count > 0 AND updated_at < DATE_SUB(NOW(), INTERVAL 1 DAY)

))

delete_manager = arel_delete_cleanup(table_name)

Nimmo note: Reviewers can examine the generated SQL like so:

puts(delete_manager.to_sql)

connection.delete(delete_manager.to_sql)

I did however simplify the conditional.
—
Reply to this email directly, view it on GitHub
https://github.com/MushroomObserver/mushroom-observer/pull/916#discussion_r867419560,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAYTNNJDCUGWBOQC3BMNP23VI4GT5ANCNFSM5VJNGEKA
.
You are receiving this because your review was requested.Message ID:
@.***>

Nimmo:
Thanks for continuing this series of PRs.
It makes it super easy to review, and I'm learning a lot from them.
. I won't have a chance to review it until Tues.
Joe
On Sat, May 7, 2022 at 7:50 AM Jason Hollinger @.***>
wrote:

@.**** commented on this pull request.
In app/models/query_record.rb
https://github.com/MushroomObserver/mushroom-observer/pull/916#discussion_r867360626
:

@@ -15,12 +19,28 @@ def self.cleanup
(@@last_cleanup < 5.minutes.ago) ||
::Rails.env.test?

connection.delete(%(

 DELETE FROM #{table_name}

 WHERE

   access_count = 0 AND updated_at < DATE_SUB(NOW(), INTERVAL 6 HOUR) OR

   access_count > 0 AND updated_at < DATE_SUB(NOW(), INTERVAL 1 DAY)

))

delete_manager = arel_delete_cleanup(table_name)

Nimmo note: Reviewers can examine the generated SQL like so:

puts(delete_manager.to_sql)

connection.delete(delete_manager.to_sql)

What would you and Joe think about simplifying the condition -- ignoring
access_count and letting everything live for a day? Would that let you do
this in AR instead of Arel? I never remember the syntax of date comparisons
in ActiveRecord.
—
Reply to this email directly, view it on GitHub
https://github.com/MushroomObserver/mushroom-observer/pull/916#pullrequestreview-965338385,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAALDFBO6FDBSMRI4E7EH73VIZ7MNANCNFSM5VJNGEKA
.
You are receiving this because your review was requested.Message ID:
@.***>

Jason -
LOL! I had no idea why -- and could not find any definition of the variable. Just assumed it was a utility method built for versatility.
Joe -
Thanks!

On May 7, 2022, at 7:02 PM, Joseph D. Cohen @.***> wrote:
Nimmo:
Thanks for continuing this series of PRs.
It makes it super easy to review, and I'm learning a lot from them.
. I won't have a chance to review it until Tues.
Joe
On Sat, May 7, 2022 at 7:50 AM Jason Hollinger @.***>
wrote:

@.**** commented on this pull request.
In app/models/query_record.rb
https://github.com/MushroomObserver/mushroom-observer/pull/916#discussion_r867360626
:

@@ -15,12 +19,28 @@ def self.cleanup
(@@last_cleanup < 5.minutes.ago) ||
::Rails.env.test?

connection.delete(%(
DELETE FROM #{table_name}
WHERE
access_count = 0 AND updated_at < DATE_SUB(NOW(), INTERVAL 6 HOUR) OR
access_count > 0 AND updated_at < DATE_SUB(NOW(), INTERVAL 1 DAY)
))

delete_manager = arel_delete_cleanup(table_name)

Nimmo note: Reviewers can examine the generated SQL like so:

puts(delete_manager.to_sql)

connection.delete(delete_manager.to_sql)

What would you and Joe think about simplifying the condition -- ignoring
access_count and letting everything live for a day? Would that let you do
this in AR instead of Arel? I never remember the syntax of date comparisons
in ActiveRecord.
—
Reply to this email directly, view it on GitHub
https://github.com/MushroomObserver/mushroom-observer/pull/916#pullrequestreview-965338385,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAALDFBO6FDBSMRI4E7EH73VIZ7MNANCNFSM5VJNGEKA
.
You are receiving this because your review was requested.Message ID:
@.***>

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.

Jason -
In case it was not clear, the original SQL used that variable.
Maybe have a quick look at the Query class module that calls #cleanup ... If you're sure the table_name is always going to be QueryRecord i will be happy to rewrite!

On May 7, 2022, at 8:31 PM, nimmo @.***> wrote:
Jason -
LOL! I had no idea why -- and could not find any definition of the variable. Just assumed it was a utility method built for versatility.
Joe -
Thanks!

On May 7, 2022, at 7:02 PM, Joseph D. Cohen @.***> wrote:
Nimmo:
Thanks for continuing this series of PRs.
It makes it super easy to review, and I'm learning a lot from them.
. I won't have a chance to review it until Tues.
Joe
On Sat, May 7, 2022 at 7:50 AM Jason Hollinger @.***>
wrote:

@.**** commented on this pull request.
In app/models/query_record.rb
https://github.com/MushroomObserver/mushroom-observer/pull/916#discussion_r867360626
:

@@ -15,12 +19,28 @@ def self.cleanup
(@@last_cleanup < 5.minutes.ago) ||
::Rails.env.test?

connection.delete(%(
DELETE FROM #{table_name}
WHERE
access_count = 0 AND updated_at < DATE_SUB(NOW(), INTERVAL 6 HOUR) OR
access_count > 0 AND updated_at < DATE_SUB(NOW(), INTERVAL 1 DAY)
))

delete_manager = arel_delete_cleanup(table_name)

Nimmo note: Reviewers can examine the generated SQL like so:

puts(delete_manager.to_sql)

connection.delete(delete_manager.to_sql)

What would you and Joe think about simplifying the condition -- ignoring
access_count and letting everything live for a day? Would that let you do
this in AR instead of Arel? I never remember the syntax of date comparisons
in ActiveRecord.
—
Reply to this email directly, view it on GitHub
https://github.com/MushroomObserver/mushroom-observer/pull/916#pullrequestreview-965338385,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAALDFBO6FDBSMRI4E7EH73VIZ7MNANCNFSM5VJNGEKA
.
You are receiving this because your review was requested.Message ID:
@.***>

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.

Yes, I'm positive.  No idea why I wrote it that way.  Obviously it works,
but I can see how it gives the impression that it was intended to be
generalized, but it never was.  I can't imagine ever using it anywhere else
for any other table.  Sorry about that.
On Sat, May 7, 2022 at 11:38 PM andrew nimmo @.***>
wrote:

Jason -
In case it was not clear, the original SQL used that variable.
Maybe have a quick look at the Query class module that calls #cleanup
... If you're sure the table_name is always going to be QueryRecord i
will be happy to rewrite!

On May 7, 2022, at 8:31 PM, nimmo @.***> wrote:
Jason -
LOL! I had no idea why -- and could not find any definition of the
variable. Just assumed it was a utility method built for versatility.
Joe -
Thanks!

On May 7, 2022, at 7:02 PM, Joseph D. Cohen @.***> wrote:
Nimmo:
Thanks for continuing this series of PRs.
It makes it super easy to review, and I'm learning a lot from them.
. I won't have a chance to review it until Tues.
Joe
On Sat, May 7, 2022 at 7:50 AM Jason Hollinger @.***>
wrote:

@.**** commented on this pull request.
In app/models/query_record.rb
<
https://github.com/MushroomObserver/mushroom-observer/pull/916#discussion_r867360626

:

@@ -15,12 +19,28 @@ def self.cleanup
(@@last_cleanup < 5.minutes.ago) ||
::Rails.env.test?

connection.delete(%(
DELETE FROM #{table_name}
WHERE
access_count = 0 AND updated_at < DATE_SUB(NOW(), INTERVAL 6 HOUR)
OR
access_count > 0 AND updated_at < DATE_SUB(NOW(), INTERVAL 1 DAY)
))

delete_manager = arel_delete_cleanup(table_name)

Nimmo note: Reviewers can examine the generated SQL like so:

puts(delete_manager.to_sql)

connection.delete(delete_manager.to_sql)

What would you and Joe think about simplifying the condition --
ignoring
access_count and letting everything live for a day? Would that let
you do
this in AR instead of Arel? I never remember the syntax of date
comparisons
in ActiveRecord.
—
Reply to this email directly, view it on GitHub
<
https://github.com/MushroomObserver/mushroom-observer/pull/916#pullrequestreview-965338385
,
or unsubscribe
<
https://github.com/notifications/unsubscribe-auth/AAALDFBO6FDBSMRI4E7EH73VIZ7MNANCNFSM5VJNGEKA

.
You are receiving this because your review was requested.Message ID:
@.***>

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.

—
Reply to this email directly, view it on GitHub
https://github.com/MushroomObserver/mushroom-observer/pull/916#issuecomment-1120343111,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAYTNNPCINU53SS5WW2RA3DVI4ZKPANCNFSM5VJNGEKA
.
You are receiving this because your review was requested.Message ID:
@.***>

Rewritten.  Still uses a smidge of Arel syntax because in this case, I find it easier to read.
QueryRecord.where(
  QueryRecord[:updated_at] < (Time.zone.now - 1.day)
).delete_all

The alternative in "pure" AR would be
QueryRecord.where('updated_at < ?', (Time.zone.now - 1.day)).delete_all

but I was never crazy about syntax with strings like this, in any language.

@pellaea Just added a comment block at the head of the model, but I'd love a review of it.
Tried to follow template of other models.

(I hope this response goes in the right place!)
"Time.zone.now - 1.day" should be equivalent to "1.day.ago".  I think.
Would that make it any more readable?
On Sun, May 8, 2022 at 3:22 PM andrew nimmo @.***>
wrote:

Rewritten. Still uses a smidge of Arel syntax because in this case, I find
it easier to read.
QueryRecord.where(
QueryRecord[:updated_at] < (Time.zone.now - 1.day)).delete_all
The alternative in "pure" AR would be
QueryRecord.where('updated_at < ?', (Time.zone.now - 1.day)).delete_all
but I was never crazy about syntax with strings like this, in any language.
—
Reply to this email directly, view it on GitHub
https://github.com/MushroomObserver/mushroom-observer/pull/916#issuecomment-1120473237,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAYTNNNXL63F6BKY62IBXU3VJAH6HANCNFSM5VJNGEKA
.
You are receiving this because your review was requested.Message ID:
@.***>

Good idea. I switched it to 1.day.ago.
(It's the .where('string_parsing > ?', thing) syntax that i don't like to read. To me the syntax now is very transparent — it says we're checking the value of a table column.)

Good catch on the extra quotes, Joe!!
On Mon, May 9, 2022 at 8:54 PM Joseph D. Cohen @.***>
wrote:

@.**** requested changes on this pull request.
Thanks again for all the great work, digging thru Arel, and dividing the
PRs into easily reviewable pieces.
In app/models/query_record.rb
https://github.com/MushroomObserver/mushroom-observer/pull/916#discussion_r868153587
:

@@ -1,26 +1,53 @@

frozen_string_literal: true
+#  = Query Record Model
+#
+#  Query Records store the parameters of recent user queries for quicker access.
+#  Queries can be nested (inner/outer); if query_record is for an inner query,
+#  the outer_id is also stored.
+#  Used by MO's Query::Modules::ActiveRecord
+#
+#  == Attributes
+#
+#  id::             Locally unique numerical id.
⬇ Suggested change
-#  id::             Locally unique numerical id.
+#  id::             Unique numerical id.
"Locally" is an ancient hangover from an MO version that allowed multiple
servers. (A "locally" unique id was unique to a particular server.) That
code was ripped out long ago.
In app/models/query_record.rb
https://github.com/MushroomObserver/mushroom-observer/pull/916#discussion_r868169924
:

def self.cleanup

 return unless !defined?(@@last_cleanup) ||

               (@@last_cleanup < 5.minutes.ago) ||

               ::Rails.env.test?

connection.delete(%(

 DELETE FROM #{table_name}

 WHERE

   access_count = 0 AND updated_at < DATE_SUB(NOW(), INTERVAL 6 HOUR) OR

   access_count > 0 AND updated_at < DATE_SUB(NOW(), INTERVAL 1 DAY)

))

QueryRecord.where(

 QueryRecord[:updated_at] < 1.day.ago

).delete_all

 @@last_cleanup = Time.zone.now

end
Rather than @@last_cleanup could we get use a class instance variable, or
even a plain old instance variable?
See https://www.ruby-lang.org/en/documentation/faq/8/,
https://docs.rubocop.org/rubocop/1.0/cops_style.html#styleclassvars
⬇ Suggested change

def self.cleanup

return unless !defined?(@@last_cleanup) ||

             (@@last_cleanup < 5.minutes.ago) ||

             ::Rails.env.test?

connection.delete(%(

 DELETE FROM #{table_name}

 WHERE

   access_count = 0 AND updated_at < DATE_SUB(NOW(), INTERVAL 6 HOUR) OR

   access_count > 0 AND updated_at < DATE_SUB(NOW(), INTERVAL 1 DAY)

))

QueryRecord.where(

 QueryRecord[:updated_at] < 1.day.ago

).delete_all

@@last_cleanup = Time.zone.now

end

def self.cleanup

return unless @.***_cleanup) ||

             ***@***.***_cleanup < 5.minutes.ago) ||

             ::Rails.env.test?

QueryRecord.where(

 QueryRecord[:updated_at] < 1.day.ago

).delete_all

@last_cleanup = Time.zone.now

end

In app/models/species_list.rb
https://github.com/MushroomObserver/mushroom-observer/pull/916#discussion_r868683351
:

Note: Need to use connection.quote_string here

old_name = SpeciesList.connection.quote_string(old_name)

new_name = SpeciesList.connection.quote_string(location.name)

SpeciesList.where(where: old_name).update_all(

 where: new_name, location_id: location.id

)

⬇ Suggested change

Note: Need to use connection.quote_string here

old_name = SpeciesList.connection.quote_string(old_name)

new_name = SpeciesList.connection.quote_string(location.name)

SpeciesList.where(where: old_name).update_all(

 where: new_name, location_id: location.id

)

SpeciesList.where(where: old_name).update_all(

 where: location.name, location_id: location.id

)

I get easily confused by sql and Arel. But I think AR takes care of the
quotes, and that connection.quote_string adds an extra, and therefore
incorrect, level of quotes.
For example:
2.6.6 :001 > Observation.last.where
...
=> "Secret 'hidey hole', Oregon, USA"
2.6.6 :002 > Observation.where(where: "Secret 'hidey hole', Oregon, USA")
Observation Load (302.8ms)  SELECT  observations.* FROM observations WHERE observations.where = 'Secret 'hidey hole', Oregon, USA' LIMIT 11
=> #<ActiveRecord::Relation [#<Observation id: 485741, created_at: "2022-05-09 23:51:49", updated_at: "2022-05-10 00:37:23", when: "2022-05-09", user_id: 4468, specimen: false, notes: {}, thumb_image_id: nil, name_id: 1, location_id: nil, is_collection_location: true, vote_cache: 0.0, num_views: 1, last_view: "2022-05-10 00:36:57", rss_log_id: 594018, lat: nil, long: nil, where: "Secret 'hidey hole', Oregon, USA", alt: nil, lifeform: " ", text_name: "Fungi", classification: "Domain: Eukarya", gps_hidden: false>]>
2.6.6 :003 > Observation.where(where: Observation.connection.quote_string("Secret 'hidey hole', Oregon, USA"))
Observation Load (288.7ms)  SELECT  observations.* FROM observations WHERE observations.where = 'Secret \'hidey hole\', Oregon, USA' LIMIT 11
=> #<ActiveRecord::Relation []>
I will also shortly push a new test to cover this method.
Please change/revert it if you don't like it.
—
Reply to this email directly, view it on GitHub
https://github.com/MushroomObserver/mushroom-observer/pull/916#pullrequestreview-966371959,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAYTNNJ6A3KIK2ZZBF4E5MLVJGXUVANCNFSM5VJNGEKA
.
You are receiving this because you were mentioned.Message ID:
@.***>

Sure, works for me.
On Mon, May 9, 2022 at 9:34 PM andrew nimmo @.***>
wrote:

@.**** commented on this pull request.
In app/models/query_record.rb
https://github.com/MushroomObserver/mushroom-observer/pull/916#discussion_r868716270
:

def self.cleanup
return unless !defined?(@@last_cleanup) ||
(@@last_cleanup < 5.minutes.ago) ||
::Rails.env.test?

connection.delete(%(

 DELETE FROM #{table_name}

 WHERE

   access_count = 0 AND updated_at < DATE_SUB(NOW(), INTERVAL 6 HOUR) OR

   access_count > 0 AND updated_at < DATE_SUB(NOW(), INTERVAL 1 DAY)

))

QueryRecord.where(

 QueryRecord[:updated_at] < 1.day.ago

).delete_all
@@last_cleanup = Time.zone.now
end

I like it. @pellaea https://github.com/pellaea does @last_cleanup work
for you?
—
Reply to this email directly, view it on GitHub
https://github.com/MushroomObserver/mushroom-observer/pull/916#discussion_r868716270,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAYTNNKYCEK6DVIWTCB5UI3VJG4I5ANCNFSM5VJNGEKA
.
You are receiving this because you were mentioned.Message ID:
@.***>

authors: JoeCohen coveralls nimmolo pellaea