Resolve cross join issues in ee/app/services/vulnerabilities/security_finding/create_merge_request_service.rb
Summary
Decomposition of some vulnerability tables has resulted in cross join issues in the listed service which have been allowed to progress anyways. Likely these will be resolvable simply once all the related tables have been decomposed to the correct schema.
Further details
Specs fail with CrossDatabaseModificationAcrossUnsupportedTablesError errors
when temporary_ignore_tables_in_transaction is removed from
Vulnerabilities::SecurityFinding::CreateMergeRequestService#execute.
NOTE: This references Resolve cross join issues in services/merge_req... (#480003 - closed) but it should reference #480359 (closed) (this very issue) instead.
module Vulnerabilities
module SecurityFinding
class CreateMergeRequestService < ::BaseProjectService
def execute
enforce_authorization!
@error_message = nil
merge_request = nil
Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModification.temporary_ignore_tables_in_transaction(
%w[
internal_ids
merge_requests
merge_request_user_mentions
vulnerability_merge_request_links
], url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/480003'
) do
merge_request = ApplicationRecord.transaction do
vulnerability = find_or_create_vulnerability
create_merge_request(vulnerability).tap do |merge_request|
create_vulnerability_merge_request_link(merge_request, vulnerability)
end
end
endWe have two sequences of SQL queries causing failures.
0: INSERT INTO vulnerability_statistics AS target (project_id, total, unknown, letter_grade, created_at, updated_at)
VALUES ($1, $2, $3, $4, now(), now())
ON CONFLICT (project_id)
DO UPDATE SET
"total" = GREATEST(TARGET."total" + $5, $6), "unknown" = GREATEST(TARGET."unknown" + $7, $8), letter_grade = (CASE
WHEN TARGET.critical + EXCLUDED.critical > $9 THEN
$10
WHEN TARGET.high + TARGET.unknown + EXCLUDED.high + EXCLUDED.unknown > $11 THEN
$12
WHEN TARGET.medium + EXCLUDED.medium > $13 THEN
$14
WHEN TARGET.low + EXCLUDED.low > $15 THEN
$16
ELSE
$17
END
), updated_at = now()
/*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test_sec,db_config_name:sec,line:/ee/app/services/vulnerabilities/statistics/update_service.rb:40:in `execute'*/
1: UPDATE "internal_ids" SET "last_value" = ("internal_ids"."last_value" + $1) WHERE "internal_ids"."project_id" = $2 AND "internal_ids"."usage" = $3 RETURNING "last_value" /*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test,db_config_name:main,line:/app/models/internal_id.rb:164:in `update_record!'*/ 0: UPDATE "internal_ids" SET "last_value" = ("internal_ids"."last_value" + $1) WHERE "internal_ids"."project_id" = $2 AND "internal_ids"."usage" = $3 RETURNING "last_value" /*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test,db_config_name:main,line:/app/models/internal_id.rb:164:in `update_record!'*/
1: INSERT INTO "internal_ids" ("project_id","namespace_id","usage","last_value") VALUES ($1, $2, $3, $4) ON CONFLICT DO NOTHING RETURNING "id" /*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test,db_config_name:main,line:/app/models/internal_id.rb:178:in `create_record!'*/
2: INSERT INTO "merge_requests" ("target_branch", "source_branch", "source_project_id", "author_id", "title", "created_at", "updated_at", "target_project_id", "iid", "description", "merge_params", "title_html", "description_html", "cached_markdown_version", "lock_version") VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15) RETURNING "id" /*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test,db_config_name:main,line:/app/services/issuable_base_service.rb:370:in `transaction_create'*/
3: DELETE FROM "merge_request_user_mentions" WHERE "merge_request_user_mentions"."merge_request_id" = $1 AND "merge_request_user_mentions"."note_id" IS NULL /*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test,db_config_name:main,line:/app/models/concerns/cache_markdown_field.rb:168:in `store_mentions!'*/
4: UPDATE "merge_requests" SET "merge_status" = $1 WHERE "merge_requests"."id" = $2 /*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test,db_config_name:main,line:/lib/gitlab/timeless.rb:9:in `timeless'*/
5: INSERT INTO "vulnerability_merge_request_links" ("vulnerability_id", "merge_request_id", "created_at", "updated_at") VALUES ($1, $2, $3, $4) RETURNING "id" /*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test_sec,db_config_name:sec,line:/ee/app/services/vulnerability_merge_request_links/create_service.rb:9:in `execute'*/Spec failures
failure for first sequence of queries
7) Vulnerabilities::SecurityFinding::CreateMergeRequestService#execute when there is an existing vulnerability for the security finding does not create a new Vulnerability, but creates a new MergeRequest, and a MergeRequestLink
Failure/Error: raise CrossDatabaseModificationAcrossUnsupportedTablesError, messages.join("\n\n")
Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModification::CrossDatabaseModificationAcrossUnsupportedTablesError:
Cross-database data modification of 'gitlab_main_cell, gitlab_sec, gitlab_main' were detected within a transaction modifying the 'internal_ids, merge_requests, merge_request_user_mentions, vulnerability_merge_request_links' tables.
Please refer to https://docs.gitlab.com/ee/development/database/multiple_databases.html#removing-cross-database-transactions for details on how to resolve this exception.
0: UPDATE "internal_ids" SET "last_value" = ("internal_ids"."last_value" + $1) WHERE "internal_ids"."project_id" = $2 AND "internal_ids"."usage" = $3 RETURNING "last_value" /*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test,db_config_name:main,line:/app/models/internal_id.rb:164:in `update_record!'*/
1: INSERT INTO "internal_ids" ("project_id","namespace_id","usage","last_value") VALUES ($1, $2, $3, $4) ON CONFLICT DO NOTHING RETURNING "id" /*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test,db_config_name:main,line:/app/models/internal_id.rb:178:in `create_record!'*/
2: INSERT INTO "merge_requests" ("target_branch", "source_branch", "source_project_id", "author_id", "title", "created_at", "updated_at", "target_project_id", "iid", "description", "merge_params", "title_html", "description_html", "cached_markdown_version", "lock_version") VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15) RETURNING "id" /*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test,db_config_name:main,line:/app/services/issuable_base_service.rb:370:in `transaction_create'*/
3: DELETE FROM "merge_request_user_mentions" WHERE "merge_request_user_mentions"."merge_request_id" = $1 AND "merge_request_user_mentions"."note_id" IS NULL /*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test,db_config_name:main,line:/app/models/concerns/cache_markdown_field.rb:168:in `store_mentions!'*/
4: UPDATE "merge_requests" SET "merge_status" = $1 WHERE "merge_requests"."id" = $2 /*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test,db_config_name:main,line:/lib/gitlab/timeless.rb:9:in `timeless'*/
5: INSERT INTO "vulnerability_merge_request_links" ("vulnerability_id", "merge_request_id", "created_at", "updated_at") VALUES ($1, $2, $3, $4) RETURNING "id" /*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test_sec,db_config_name:sec,line:/ee/app/services/vulnerability_merge_request_links/create_service.rb:9:in `execute'*/
# ./lib/gitlab/database/query_analyzers/prevent_cross_database_modification.rb:127:in `analyze'
# ./lib/gitlab/database/query_analyzer.rb:134:in `block in process_sql'
# ./lib/gitlab/database/query_analyzer.rb:131:in `each'
# ./lib/gitlab/database/query_analyzer.rb:131:in `process_sql'
# ./lib/gitlab/database/query_analyzer.rb:74:in `block (2 levels) in hook!'
# ./lib/gitlab/database/query_analyzer.rb:146:in `with_ignored_recursive_calls'
# ./lib/gitlab/database/query_analyzer.rb:73:in `block in hook!'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:127:in `public_send'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:127:in `block in write_using_load_balancer'
# ./lib/gitlab/database/load_balancing/load_balancer.rb:141:in `block in read_write'
# ./lib/gitlab/database/load_balancing/load_balancer.rb:228:in `retry_with_backoff'
# ./lib/gitlab/database/load_balancing/load_balancer.rb:130:in `read_write'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:126:in `write_using_load_balancer'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:61:in `block (2 levels) in <class:ConnectionProxy>'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:127:in `public_send'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:127:in `block in write_using_load_balancer'
# ./lib/gitlab/database/load_balancing/load_balancer.rb:141:in `block in read_write'
# ./lib/gitlab/database/load_balancing/load_balancer.rb:228:in `retry_with_backoff'
# ./lib/gitlab/database/load_balancing/load_balancer.rb:130:in `read_write'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:126:in `write_using_load_balancer'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:78:in `transaction'
# ./ee/app/services/vulnerability_merge_request_links/create_service.rb:9:in `execute'
# ./ee/app/services/vulnerabilities/security_finding/create_merge_request_service.rb:32:in `execute_service!'
# ./ee/app/services/vulnerabilities/security_finding/create_merge_request_service.rb:56:in `create_vulnerability_merge_request_link'
# ./ee/app/services/vulnerabilities/security_finding/create_merge_request_service.rb:16:in `block (2 levels) in execute'
# ./ee/app/services/vulnerabilities/security_finding/create_merge_request_service.rb:15:in `block in execute'
# ./app/models/concerns/cross_database_modification.rb:91:in `block in transaction'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:127:in `public_send'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:127:in `block in write_using_load_balancer'
# ./lib/gitlab/database/load_balancing/load_balancer.rb:141:in `block in read_write'
# ./lib/gitlab/database/load_balancing/load_balancer.rb:228:in `retry_with_backoff'
# ./lib/gitlab/database/load_balancing/load_balancer.rb:130:in `read_write'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:126:in `write_using_load_balancer'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:78:in `transaction'
# ./lib/gitlab/database.rb:394:in `block in transaction'
# ./lib/gitlab/database.rb:393:in `transaction'
# ./app/models/concerns/cross_database_modification.rb:82:in `transaction'
# ./ee/app/services/vulnerabilities/security_finding/create_merge_request_service.rb:13:in `execute'
# ./ee/spec/services/vulnerabilities/security_finding/create_merge_request_service_spec.rb:33:in `block (2 levels) in <top (required)>'
# ./ee/spec/services/vulnerabilities/security_finding/create_merge_request_service_spec.rb:96:in `block (4 levels) in <top (required)>'
# ./ee/spec/services/vulnerabilities/security_finding/create_merge_request_service_spec.rb:96:in `block (3 levels) in <top (required)>'
# ./spec/spec_helper.rb:474:in `block (3 levels) in <top (required)>'
# ./lib/gitlab/sidekiq_sharding/validator.rb:42:in `enabled'
# ./spec/spec_helper.rb:473:in `block (2 levels) in <top (required)>'
# ./spec/spec_helper.rb:468:in `block (3 levels) in <top (required)>'
# ./spec/support/sidekiq_middleware.rb:9:in `with_sidekiq_server_middleware'
# ./spec/spec_helper.rb:459:in `block (2 levels) in <top (required)>'
# ./spec/spec_helper.rb:455:in `block (3 levels) in <top (required)>'
# ./lib/gitlab/application_context.rb:97:in `with_raw_context'
# ./spec/spec_helper.rb:455:in `block (2 levels) in <top (required)>'
# ./spec/spec_helper.rb:426:in `block (3 levels) in <top (required)>'
# ./lib/gitlab/ci/config/feature_flags.rb:38:in `ensure_correct_usage'
# ./spec/spec_helper.rb:425:in `block (2 levels) in <top (required)>'
# ./spec/spec_helper.rb:271:in `block (2 levels) in <top (required)>'
# ./spec/support/system_exit_detected.rb:7:in `block (2 levels) in <main>'
# ./spec/support/database/prevent_cross_joins.rb:106:in `block (3 levels) in <main>'
# ./spec/support/database/prevent_cross_joins.rb:60:in `with_cross_joins_prevented'
# ./spec/support/database/prevent_cross_joins.rb:106:in `block (2 levels) in <main>'
failure for second sequence of SQL queries
8) Vulnerabilities::SecurityFinding::CreateMergeRequestService#execute when there is an existing vulnerability for the security finding does not change the vulnerability present_on_default_branch value
Failure/Error: raise CrossDatabaseModificationAcrossUnsupportedTablesError, messages.join("\n\n")
Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModification::CrossDatabaseModificationAcrossUnsupportedTablesError:
Cross-database data modification of 'gitlab_main_cell, gitlab_sec, gitlab_main' were detected within a transaction modifying the 'internal_ids, merge_requests, merge_request_user_mentions, vulnerability_merge_request_links' tables.
Please refer to https://docs.gitlab.com/ee/development/database/multiple_databases.html#removing-cross-database-transactions for details on how to resolve this exception.
0: UPDATE "internal_ids" SET "last_value" = ("internal_ids"."last_value" + $1) WHERE "internal_ids"."project_id" = $2 AND "internal_ids"."usage" = $3 RETURNING "last_value" /*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test,db_config_name:main,line:/app/models/internal_id.rb:164:in `update_record!'*/
1: INSERT INTO "internal_ids" ("project_id","namespace_id","usage","last_value") VALUES ($1, $2, $3, $4) ON CONFLICT DO NOTHING RETURNING "id" /*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test,db_config_name:main,line:/app/models/internal_id.rb:178:in `create_record!'*/
2: INSERT INTO "merge_requests" ("target_branch", "source_branch", "source_project_id", "author_id", "title", "created_at", "updated_at", "target_project_id", "iid", "description", "merge_params", "title_html", "description_html", "cached_markdown_version", "lock_version") VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15) RETURNING "id" /*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test,db_config_name:main,line:/app/services/issuable_base_service.rb:370:in `transaction_create'*/
3: DELETE FROM "merge_request_user_mentions" WHERE "merge_request_user_mentions"."merge_request_id" = $1 AND "merge_request_user_mentions"."note_id" IS NULL /*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test,db_config_name:main,line:/app/models/concerns/cache_markdown_field.rb:168:in `store_mentions!'*/
4: UPDATE "merge_requests" SET "merge_status" = $1 WHERE "merge_requests"."id" = $2 /*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test,db_config_name:main,line:/lib/gitlab/timeless.rb:9:in `timeless'*/
5: INSERT INTO "vulnerability_merge_request_links" ("vulnerability_id", "merge_request_id", "created_at", "updated_at") VALUES ($1, $2, $3, $4) RETURNING "id" /*application:test,correlation_id:ad95f787b441057ad9f8addce47aa727,db_config_database:gitlabhq_test_sec,db_config_name:sec,line:/ee/app/services/vulnerability_merge_request_links/create_service.rb:9:in `execute'*/
# ./lib/gitlab/database/query_analyzers/prevent_cross_database_modification.rb:127:in `analyze'
# ./lib/gitlab/database/query_analyzer.rb:134:in `block in process_sql'
# ./lib/gitlab/database/query_analyzer.rb:131:in `each'
# ./lib/gitlab/database/query_analyzer.rb:131:in `process_sql'
# ./lib/gitlab/database/query_analyzer.rb:74:in `block (2 levels) in hook!'
# ./lib/gitlab/database/query_analyzer.rb:146:in `with_ignored_recursive_calls'
# ./lib/gitlab/database/query_analyzer.rb:73:in `block in hook!'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:127:in `public_send'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:127:in `block in write_using_load_balancer'
# ./lib/gitlab/database/load_balancing/load_balancer.rb:141:in `block in read_write'
# ./lib/gitlab/database/load_balancing/load_balancer.rb:228:in `retry_with_backoff'
# ./lib/gitlab/database/load_balancing/load_balancer.rb:130:in `read_write'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:126:in `write_using_load_balancer'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:61:in `block (2 levels) in <class:ConnectionProxy>'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:127:in `public_send'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:127:in `block in write_using_load_balancer'
# ./lib/gitlab/database/load_balancing/load_balancer.rb:141:in `block in read_write'
# ./lib/gitlab/database/load_balancing/load_balancer.rb:228:in `retry_with_backoff'
# ./lib/gitlab/database/load_balancing/load_balancer.rb:130:in `read_write'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:126:in `write_using_load_balancer'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:78:in `transaction'
# ./ee/app/services/vulnerability_merge_request_links/create_service.rb:9:in `execute'
# ./ee/app/services/vulnerabilities/security_finding/create_merge_request_service.rb:32:in `execute_service!'
# ./ee/app/services/vulnerabilities/security_finding/create_merge_request_service.rb:56:in `create_vulnerability_merge_request_link'
# ./ee/app/services/vulnerabilities/security_finding/create_merge_request_service.rb:16:in `block (2 levels) in execute'
# ./ee/app/services/vulnerabilities/security_finding/create_merge_request_service.rb:15:in `block in execute'
# ./app/models/concerns/cross_database_modification.rb:91:in `block in transaction'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:127:in `public_send'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:127:in `block in write_using_load_balancer'
# ./lib/gitlab/database/load_balancing/load_balancer.rb:141:in `block in read_write'
# ./lib/gitlab/database/load_balancing/load_balancer.rb:228:in `retry_with_backoff'
# ./lib/gitlab/database/load_balancing/load_balancer.rb:130:in `read_write'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:126:in `write_using_load_balancer'
# ./lib/gitlab/database/load_balancing/connection_proxy.rb:78:in `transaction'
# ./lib/gitlab/database.rb:394:in `block in transaction'
# ./lib/gitlab/database.rb:393:in `transaction'
# ./app/models/concerns/cross_database_modification.rb:82:in `transaction'
# ./ee/app/services/vulnerabilities/security_finding/create_merge_request_service.rb:13:in `execute'
# ./ee/spec/services/vulnerabilities/security_finding/create_merge_request_service_spec.rb:33:in `block (2 levels) in <top (required)>'
# ./ee/spec/services/vulnerabilities/security_finding/create_merge_request_service_spec.rb:104:in `block (4 levels) in <top (required)>'
# ./ee/spec/services/vulnerabilities/security_finding/create_merge_request_service_spec.rb:104:in `block (3 levels) in <top (required)>'
# ./spec/spec_helper.rb:474:in `block (3 levels) in <top (required)>'
# ./lib/gitlab/sidekiq_sharding/validator.rb:42:in `enabled'
# ./spec/spec_helper.rb:473:in `block (2 levels) in <top (required)>'
# ./spec/spec_helper.rb:468:in `block (3 levels) in <top (required)>'
# ./spec/support/sidekiq_middleware.rb:9:in `with_sidekiq_server_middleware'
# ./spec/spec_helper.rb:459:in `block (2 levels) in <top (required)>'
# ./spec/spec_helper.rb:455:in `block (3 levels) in <top (required)>'
# ./lib/gitlab/application_context.rb:97:in `with_raw_context'
# ./spec/spec_helper.rb:455:in `block (2 levels) in <top (required)>'
# ./spec/spec_helper.rb:426:in `block (3 levels) in <top (required)>'
# ./lib/gitlab/ci/config/feature_flags.rb:38:in `ensure_correct_usage'
# ./spec/spec_helper.rb:425:in `block (2 levels) in <top (required)>'
# ./spec/spec_helper.rb:271:in `block (2 levels) in <top (required)>'
# ./spec/support/system_exit_detected.rb:7:in `block (2 levels) in <main>'
# ./spec/support/database/prevent_cross_joins.rb:106:in `block (3 levels) in <main>'
# ./spec/support/database/prevent_cross_joins.rb:60:in `with_cross_joins_prevented'
# ./spec/support/database/prevent_cross_joins.rb:106:in `block (2 levels) in <main>Proposal
- Remove
temporary_ignore_tables_in_transactionfrom Vulnerabilities::SecurityFinding::CreateMergeRequestService#execute. - Implement the rollback mechanism in the service class itself,
and destroy objects when the service fails.
We can rely on the
ActiveRecord::Rollbackexception that's already raised when any of the servicesCreateMergeRequestServicedepends on fails.
Same as #503978 (closed), so doing on issue should close the other.
Edited by Fabien Catteau