-
-
Notifications
You must be signed in to change notification settings - Fork 207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better table name detection for Job queries #119
Better table name detection for Job queries #119
Conversation
@bensheldon There are two commits here. First one just removes the .public from the table name and is a hacky way to display the working version. The second commit tries to include the table_name in scope :joins_advisory_locks, (lambda do
join_sql = <<~SQL
LEFT JOIN pg_locks ON pg_locks.locktype = 'advisory'
AND pg_locks.objsubid = 1
AND pg_locks.classid = ('x'||substr(md5(:table_name || :table_with_primary_key_quoted::text), 1, 16))::bit(32)::int
AND pg_locks.objid = (('x'||substr(md5(:table_name || :table_with_primary_key_quoted::text), 1, 16))::bit(64) << 32)::bit(32)::int
SQL
table_with_primary_key_quoted = [quoted_table_name, quoted_primary_key].join('.')
joins(sanitize_sql([join_sql, { table_name: quoted_table_name, table_with_primary_key_quoted: table_with_primary_key_quoted }]))
end) with Prior does as "good_jobs"."id" but the latter is '"public"."good_jobs"."id"'::text. This results in empty collection. If I were to put the "public"."good_jobs"."id" manually without the single quotes that sanitization produces, all works fine. I assume this is due to md5 generation which includes single quotes in the latter version which then doesn't matchup with other quries/locks(not sure). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I think you have the parameters/interpolation behavior reversed. Please see my comment.
lib/good_job/lockable.rb
Outdated
@@ -13,7 +13,7 @@ module Lockable | |||
|
|||
query = cte_table.project(cte_table[:id]) | |||
.with(composed_cte) | |||
.where(Arel.sql(sanitize_sql_for_conditions(["pg_try_advisory_lock(('x'||substr(md5(:table_name || \"#{cte_table.name}\".\"#{primary_key}\"::text), 1, 16))::bit(64)::bigint)", { table_name: table_name }]))) | |||
.where(Arel.sql(sanitize_sql_for_conditions(["pg_try_advisory_lock(('x'||substr(md5(:table_name || \"#{cte_table.name}\".\"#{primary_key}\"::text), 1, 16))::bit(64)::bigint)", { table_name: quoted_table_name }]))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you have the logic reversed on these.
The parameterized value does not need to be quoted because it is intended to be inserted into the query as a parameterized string.
The interpolated table/columns are the ones that need to be quoted. (Sorry, I'm on my phone so I can't write out code easily).
Eg
.where(Arel.sql(sanitize_sql_for_conditions(["pg_try_advisory_lock(('x'||substr(md5(:table_name || #{quoted_table_name(cte_table.name)}.#{quoted_column_name(primary_key)}::text), 1, 16))::bit(64)::bigint)", { table_name: table_name }])))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted this change as it was unrelated. But than you for mentioning the quotation thing. I removed extra quotations now. The only change I made is in joins_advisory_lock scope which adds quoted_table_name and quoted_primary_key which fixes the issue.
However, I am curious of any side-effects it may produce/or not.
- Adds quoted_table_name and quoted_primary_key in joins_advisory_lock scope to enable queries in multi schema setups when table name contains "schema"."table_name" format
Closes #117 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thank you!
@gadimbaylisahil I just released GoodJob v1.2.4 with this change. |
#117