Skip to content

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.

Related to #341783 (closed)

Merge request reports

Loading