Fix PreventCrossDatabaseModification query parser prepended comments
What does this MR do and why?
As part of our efforts to decompose our database, this class is designed to parse all SQL queries and detect violations where we query different databases while a transaction is open.
Previously this code did not correctly handle prepended SQL comments as
it was using sql.start_with?
. We didn't notice this was a problem
until we tried to use it in production where we learnt that this does
not work due marginalia comments being prepended in production per
https://gitlab.com/gitlab-org/gitlab/-/blob/f7963d40f6c865839d66e2d325e47a91caa7f7b5/config/initializers/0_marginalia.rb#L15
.
Screenshots or screen recordings
Queries in production look like:
/*application:test,correlation_id:f9cad756694e585678eb1186432ef10e,db_config_name:main*/ SAVEPOINT active_record_3
/*application:test,correlation_id:f9cad756694e585678eb1186432ef10e,db_config_name:main*/ SELECT "projects"."id", "projects"."name", "projects"."path", "projects"."description", "projects"."created_at", "projects"."updated_at", "projects"."creator_id", "projects"."namespace_id", "projects"."last_activity_at", "projects"."import_url", "projects"."visibility_level", "projects"."archived", "projects"."avatar", "projects"."merge_requests_template", "projects"."star_count", "projects"."merge_requests_rebase_enabled", "projects"."import_type", "projects"."import_source", "projects"."approvals_before_merge", "projects"."reset_approvals_on_push", "projects"."merge_requests_ff_only_enabled", "projects"."issues_template", "projects"."mirror", "projects"."mirror_user_id", "projects"."shared_runners_enabled", "projects"."runners_token", "projects"."build_coverage_regex", "projects"."build_allow_git_fetch", "projects"."build_timeout", "projects"."mirror_trigger_builds", "projects"."pending_delete", "projects"."public_builds", "projects"."last_repository_check_failed", "projects"."last_repository_check_at", "projects"."only_allow_merge_if_pipeline_succeeds", "projects"."has_external_issue_tracker", "projects"."repository_storage", "projects"."repository_read_only", "projects"."request_access_enabled", "projects"."has_external_wiki", "projects"."ci_config_path", "projects"."lfs_enabled", "projects"."description_html", "projects"."only_allow_merge_if_all_discussions_are_resolved", "projects"."repository_size_limit", "projects"."printing_merge_request_link_enabled", "projects"."auto_cancel_pending_pipelines", "projects"."service_desk_enabled", "projects"."cached_markdown_version", "projects"."delete_error", "projects"."last_repository_updated_at", "projects"."disable_overriding_approvers_per_merge_request", "projects"."storage_version", "projects"."resolve_outdated_diff_discussions", "projects"."remote_mirror_available_overridden", "projects"."only_mirror_protected_branches", "projects"."pull_mirror_available_overridden", "projects"."jobs_cache_index", "projects"."external_authorization_classification_label", "projects"."mirror_overwrites_diverged_branches", "projects"."pages_https_only", "projects"."external_webhook_token", "projects"."packages_enabled", "projects"."merge_requests_author_approval", "projects"."pool_repository_id", "projects"."runners_token_encrypted", "projects"."bfg_object_map", "projects"."detected_repository_languages", "projects"."merge_requests_disable_committers_approval", "projects"."require_password_to_approve", "projects"."emails_disabled", "projects"."max_pages_size", "projects"."max_artifacts_size", "projects"."remove_source_branch_after_merge", "projects"."marked_for_deletion_at", "projects"."marked_for_deletion_by_user_id", "projects"."autoclose_referenced_issues", "projects"."suggestion_commit_message", "projects"."project_namespace_id" FROM "projects" WHERE "projects"."id" = $1 LIMIT $2
/*application:test,correlation_id:f9cad756694e585678eb1186432ef10e,db_config_name:main*/ UPDATE "projects" SET "updated_at" = $1 WHERE "projects"."id" = $2
/*application:test,correlation_id:f9cad756694e585678eb1186432ef10e,db_config_name:main*/ SELECT "ci_pipelines".* FROM "ci_pipelines" WHERE "ci_pipelines"."id" = $1 LIMIT $2
/*application:test,correlation_id:f9cad756694e585678eb1186432ef10e,db_config_name:main*/ UPDATE "ci_pipelines" SET "updated_at" = $1, "lock_version" = $2 WHERE "ci_pipelines"."id" = $3 AND "ci_pipelines"."lock_version" = $4
/*application:test,correlation_id:f9cad756694e585678eb1186432ef10e,db_config_name:main*/ RELEASE SAVEPOINT active_record_3
When you call pg.deparse
on the parsed query you get:
SAVEPOINT active_record_3
SELECT projects.id, projects.name, projects.path, projects.description, projects.created_at, projects.updated_at, projects.creator_id, projects.namespace_id, projects.last_activity_at, projects.import_url, projects.visibility_level, projects.archived, projects.avatar, projects.merge_requests_template, projects.star_count, projects.merge_requests_rebase_enabled, projects.import_type, projects.import_source, projects.approvals_before_merge, projects.reset_approvals_on_push, projects.merge_requests_ff_only_enabled, projects.issues_template, projects.mirror, projects.mirror_user_id, projects.shared_runners_enabled, projects.runners_token, projects.build_coverage_regex, projects.build_allow_git_fetch, projects.build_timeout, projects.mirror_trigger_builds, projects.pending_delete, projects.public_builds, projects.last_repository_check_failed, projects.last_repository_check_at, projects.only_allow_merge_if_pipeline_succeeds, projects.has_external_issue_tracker, projects.repository_storage, projects.repository_read_only, projects.request_access_enabled, projects.has_external_wiki, projects.ci_config_path, projects.lfs_enabled, projects.description_html, projects.only_allow_merge_if_all_discussions_are_resolved, projects.repository_size_limit, projects.printing_merge_request_link_enabled, projects.auto_cancel_pending_pipelines, projects.service_desk_enabled, projects.cached_markdown_version, projects.delete_error, projects.last_repository_updated_at, projects.disable_overriding_approvers_per_merge_request, projects.storage_version, projects.resolve_outdated_diff_discussions, projects.remote_mirror_available_overridden, projects.only_mirror_protected_branches, projects.pull_mirror_available_overridden, projects.jobs_cache_index, projects.external_authorization_classification_label, projects.mirror_overwrites_diverged_branches, projects.pages_https_only, projects.external_webhook_token, projects.packages_enabled, projects.merge_requests_author_approval, projects.pool_repository_id, projects.runners_token_encrypted, projects.bfg_object_map, projects.detected_repository_languages, projects.merge_requests_disable_committers_approval, projects.require_password_to_approve, projects.emails_disabled, projects.max_pages_size, projects.max_artifacts_size, projects.remove_source_branch_after_merge, projects.marked_for_deletion_at, projects.marked_for_deletion_by_user_id, projects.autoclose_referenced_issues, projects.suggestion_commit_message, projects.project_namespace_id FROM projects WHERE projects.id = $1 LIMIT $2
UPDATE projects SET updated_at = $1 WHERE projects.id = $2
SELECT ci_pipelines.* FROM ci_pipelines WHERE ci_pipelines.id = $1 LIMIT $2
UPDATE ci_pipelines SET updated_at = $1, lock_version = $2 WHERE ci_pipelines.id = $3 AND ci_pipelines.lock_version = $4
RELEASE active_record_3
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Related to #341783 (closed)