500 error when submitting a review with diff notes that only have quick actions
This is related to !170913 (merged).
If you click Submit Review on a merge request that has a note with only quick actions, you get a 500 error.
To reproduce:
- On a merge request diff, click on a line and comment,
/spend 1d. - Click on
Add to Review. - At the bottom of the window, click "Finish review"
The 500 error appears to happen when DraftNotes::PublishService attempts to call capture_diff_note_positions, but no note actually is persisted:
"exception.class": "ActiveRecord::NotNullViolation",
"exception.message": "PG::NotNullViolation: ERROR: null value in column \"note_id\" of relation \"diff_note_positions\" violates not-null constraint\nDETAIL: Failing row contains (12, null, null, 4, 0, 0, ab2a5e29d532692efdc3860b7899d9a26e6898f5_0_4, \\x66626232666466663937663835326438306664363664323865393963373436..., \\x66626232666466663937663835326438306664363664323865393963373436..., \\x66636532356137356638616263626636386265616263656430313339643766..., billboard_top_10.py, billboard_top_10.py).\n",
"exception.backtrace": [
"activerecord (7.0.8.4) lib/active_record/connection_adapters/postgresql_adapter.rb:768:in `exec_params'",
"activerecord (7.0.8.4) lib/active_record/connection_adapters/postgresql_adapter.rb:768:in `block (2 levels) in exec_no_cache'",
"activesupport (7.0.8.4) lib/active_support/concurrency/share_lock.rb:187:in `yield_shares'",
"activesupport (7.0.8.4) lib/active_support/dependencies/interlock.rb:41:in `permit_concurrent_loads'",
"activerecord (7.0.8.4) lib/active_record/connection_adapters/postgresql_adapter.rb:767:in `block in exec_no_cache'",
"activesupport (7.0.8.4) lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `handle_interrupt'",
"activesupport (7.0.8.4) lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `block in synchronize'",
"activesupport (7.0.8.4) lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `handle_interrupt'",
"activesupport (7.0.8.4) lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `synchronize'",
"activerecord (7.0.8.4) lib/active_record/connection_adapters/abstract_adapter.rb:752:in `block in log'",
"activesupport (7.0.8.4) lib/active_support/notifications/instrumenter.rb:24:in `instrument'",
"activerecord (7.0.8.4) lib/active_record/connection_adapters/abstract_adapter.rb:743:in `log'",
"activerecord (7.0.8.4) lib/active_record/connection_adapters/postgresql_adapter.rb:766:in `exec_no_cache'",
"activerecord (7.0.8.4) lib/active_record/connection_adapters/postgresql_adapter.rb:745:in `execute_and_clear'",
"marginalia (1.11.1) lib/marginalia.rb:91:in `execute_and_clear_with_marginalia'",
"activerecord (7.0.8.4) lib/active_record/connection_adapters/postgresql/database_statements.rb:54:in `exec_query'",
"activerecord (7.0.8.4) lib/active_record/connection_adapters/abstract/database_statements.rb:150:in `exec_insert_all'",
"activerecord (7.0.8.4) lib/active_record/connection_adapters/abstract/query_cache.rb:22:in `exec_insert_all'",
"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>'",
"activerecord (7.0.8.4) lib/active_record/insert_all.rb:41:in `execute'",
"activerecord (7.0.8.4) lib/active_record/persistence.rb:333:in `upsert_all'",
"activerecord (7.0.8.4) lib/active_record/persistence.rb:223:in `upsert'",
"app/models/diff_note_position.rb:40:in `create_or_update_for'",
"app/services/discussions/capture_diff_note_position_service.rb:27:in `execute'",
"app/services/draft_notes/publish_service.rb:95:in `block in capture_diff_note_positions'",
"app/services/draft_notes/publish_service.rb:94:in `each'",
"app/services/draft_notes/publish_service.rb:94:in `capture_diff_note_positions'",
"app/services/draft_notes/publish_service.rb:48:in `publish_draft_notes'",
"app/services/draft_notes/publish_service.rb:13:in `execute'",
"app/controllers/projects/merge_requests/drafts_controller.rb:60:in `publish'",
"actionpack (7.0.8.4) lib/action_controller/metal/basic_implicit_render.rb:6:in `send_action'",
"actionpack (7.0.8.4) lib/abstract_controller/base.rb:215:in `process_action'",
"actionpack (7.0.8.4) lib/action_controller/metal/rendering.rb:165:in `process_action'",
"actionpack (7.0.8.4) lib/abstract_controller/callbacks.rb:234:in `block in process_action'",
"activesupport (7.0.8.4) lib/active_support/callbacks.rb:118:in `block in run_callbacks'",
"lib/gitlab/ip_address_state.rb:11:in `with'",
"ee/app/controllers/ee/application_controller.rb:45:in `set_current_ip_address'",
"activesupport (7.0.8.4) lib/active_support/callbacks.rb:127:in `block in run_callbacks'",
"app/controllers/application_controller.rb:505:in `set_current_admin'",
"activesupport (7.0.8.4) lib/active_support/callbacks.rb:127:in `block in run_callbacks'",
"lib/gitlab/session.rb:11:in `with_session'",
"app/controllers/application_controller.rb:496:in `set_session_storage'",
"activesupport (7.0.8.4) lib/active_support/callbacks.rb:127:in `block in run_callbacks'",
"lib/gitlab/i18n.rb:114:in `with_locale'",
"lib/gitlab/i18n.rb:120:in `with_user_locale'",
"app/controllers/application_controller.rb:487:in `set_locale'",
"activesupport (7.0.8.4) lib/active_support/callbacks.rb:127:in `block in run_callbacks'",
"app/controllers/application_controller.rb:480:in `set_current_context'",
"activesupport (7.0.8.4) lib/active_support/callbacks.rb:127:in `block in run_callbacks'",
"marginalia (1.11.1) lib/marginalia.rb:109:in `record_query_comment'",
"activesupport (7.0.8.4) lib/active_support/callbacks.rb:127:in `block in run_callbacks'",
"sentry-rails (5.21.0) lib/sentry/rails/controller_transaction.rb:32:in `block in sentry_around_action'",
"sentry-ruby (5.21.0) lib/sentry/hub.rb:108:in `with_child_span'",
"sentry-ruby (5.21.0) lib/sentry-ruby.rb:499:in `with_child_span'",
"sentry-rails (5.21.0) lib/sentry/rails/controller_transaction.rb:18:in `sentry_around_action'",
"activesupport (7.0.8.4) lib/active_support/callbacks.rb:127:in `block in run_callbacks'",
"activesupport (7.0.8.4) lib/active_support/callbacks.rb:138:in `run_callbacks'",
"actionpack (7.0.8.4) lib/abstract_controller/callbacks.rb:233:in `process_action'",
"actionpack (7.0.8.4) lib/action_controller/metal/rescue.rb:23:in `process_action'",
"actionpack (7.0.8.4) lib/action_controller/metal/instrumentation.rb:67:in `block in process_action'",
"activesupport (7.0.8.4) lib/active_support/notifications.rb:206:in `block in instrument'",
"activesupport (7.0.8.4) lib/active_support/notifications/instrumenter.rb:24:in `instrument'",
"activesupport (7.0.8.4) lib/active_support/notifications.rb:206:in `instrument'",
"actionpack (7.0.8.4) lib/action_controller/metal/instrumentation.rb:66:in `process_action'",
"actionpack (7.0.8.4) lib/action_controller/metal/params_wrapper.rb:259:in `process_action'",
"activerecord (7.0.8.4) lib/active_record/railties/controller_runtime.rb:27:in `process_action'",
"actionpack (7.0.8.4) lib/abstract_controller/base.rb:151:in `process'",
"actionview (7.0.8.4) lib/action_view/rendering.rb:39:in `process'",
"actionpack (7.0.8.4) lib/action_controller/metal.rb:188:in `dispatch'",
"actionpack (7.0.8.4) lib/action_controller/metal.rb:249:in `block in dispatch'",
"lib/gitlab/middleware/action_controller_static_context.rb:23:in `call'",
"actionpack (7.0.8.4) lib/action_controller/metal.rb:249:in `dispatch'",
"actionpack (7.0.8.4) lib/action_dispatch/routing/route_set.rb:49:in `dispatch'",
"actionpack (7.0.8.4) lib/action_dispatch/routing/route_set.rb:32:in `serve'",
"actionpack (7.0.8.4) lib/action_dispatch/routing/mapper.rb:18:in `block in <class:Constraints>'",
"actionpack (7.0.8.4) lib/action_dispatch/routing/mapper.rb:48:in `serve'",
"actionpack (7.0.8.4) lib/action_dispatch/journey/router.rb:50:in `block in serve'",
"actionpack (7.0.8.4) lib/action_dispatch/journey/router.rb:32:in `each'",
"actionpack (7.0.8.4) lib/action_dispatch/journey/router.rb:32:in `serve'",
"actionpack (7.0.8.4) lib/action_dispatch/routing/route_set.rb:852:in `call'",
"gitlab-experiment (0.9.1) lib/gitlab/experiment/middleware.rb:19:in `call'",
"flipper (0.26.2) lib/flipper/middleware/memoizer.rb:72:in `memoized_call'",
"flipper (0.26.2) lib/flipper/middleware/memoizer.rb:37:in `call'",
"lib/gitlab/metrics/elasticsearch_rack_middleware.rb:16:in `call'",
"lib/gitlab/middleware/sidekiq_shard_awareness_validation.rb:20:in `block in call'",
"lib/gitlab/sidekiq_sharding/validator.rb:42:in `enabled'",
"lib/gitlab/middleware/sidekiq_shard_awareness_validation.rb:20:in `call'",
"lib/gitlab/middleware/memory_report.rb:13:in `call'",
"lib/gitlab/middleware/speedscope.rb:13:in `call'",
"lib/gitlab/database/load_balancing/rack_middleware.rb:23:in `call'",
"lib/gitlab/middleware/rails_queue_duration.rb:33:in `call'",
"lib/gitlab/etag_caching/middleware.rb:21:in `call'",
"lib/gitlab/metrics/rack_middleware.rb:16:in `block in call'",
"lib/gitlab/metrics/web_transaction.rb:46:in `run'",
"lib/gitlab/metrics/rack_middleware.rb:16:in `call'",
"lib/gitlab/middleware/go.rb:21:in `call'",
"lib/gitlab/middleware/query_analyzer.rb:11:in `block in call'",
"lib/gitlab/database/query_analyzer.rb:83:in `within'",
"lib/gitlab/middleware/query_analyzer.rb:11:in `call'",
"lib/ci/job_token/middleware.rb:11:in `call'",
"batch-loader (2.0.5) lib/batch_loader/middleware.rb:11:in `call'",
"rack-attack (6.7.0) lib/rack/attack.rb:103:in `call'",
"apollo_upload_server (2.1.6) lib/apollo_upload_server/middleware.rb:19:in `call'",
"lib/gitlab/middleware/multipart.rb:173:in `call'",
"rack-attack (6.7.0) lib/rack/attack.rb:127:in `call'",
"warden (1.2.9) lib/warden/manager.rb:36:in `block in call'",
"warden (1.2.9) lib/warden/manager.rb:34:in `catch'",
"warden (1.2.9) lib/warden/manager.rb:34:in `call'",
"rack-cors (2.0.2) lib/rack/cors.rb:102:in `call'",
"rack (2.2.10) lib/rack/tempfile_reaper.rb:15:in `call'",
"rack (2.2.10) lib/rack/etag.rb:27:in `call'",
"rack (2.2.10) lib/rack/conditional_get.rb:40:in `call'",
"rack (2.2.10) lib/rack/head.rb:12:in `call'",
"actionpack (7.0.8.4) lib/action_dispatch/http/permissions_policy.rb:38:in `call'",
"actionpack (7.0.8.4) lib/action_dispatch/http/content_security_policy.rb:36:in `call'",
"lib/gitlab/middleware/read_only/controller.rb:50:in `call'",
"lib/gitlab/middleware/read_only.rb:18:in `call'",
"lib/gitlab/middleware/unauthenticated_session_expiry.rb:18:in `call'",
"rack (2.2.10) lib/rack/session/abstract/id.rb:266:in `context'",
"rack (2.2.10) lib/rack/session/abstract/id.rb:260:in `call'",
"actionpack (7.0.8.4) lib/action_dispatch/middleware/cookies.rb:704:in `call'",
"lib/gitlab/middleware/strip_cookies.rb:29:in `call'",
"lib/gitlab/middleware/same_site_cookies.rb:27:in `call'",
"actionpack (7.0.8.4) lib/action_dispatch/middleware/callbacks.rb:27:in `block in call'",
"activesupport (7.0.8.4) lib/active_support/callbacks.rb:99:in `run_callbacks'",
"actionpack (7.0.8.4) lib/action_dispatch/middleware/callbacks.rb:26:in `call'",
"sentry-rails (5.21.0) lib/sentry/rails/rescued_exception_interceptor.rb:14:in `call'",
"actionpack (7.0.8.4) lib/action_dispatch/middleware/debug_exceptions.rb:28:in `call'",
"lib/gitlab/middleware/path_traversal_check.rb:35:in `call'",
"lib/gitlab/middleware/handle_malformed_strings.rb:21:in `call'",
"sentry-ruby (5.21.0) lib/sentry/rack/capture_exceptions.rb:30:in `block (2 levels) in call'",
"sentry-ruby (5.21.0) lib/sentry/hub.rb:265:in `with_session_tracking'",
"sentry-ruby (5.21.0) lib/sentry-ruby.rb:412:in `with_session_tracking'",
"sentry-ruby (5.21.0) lib/sentry/rack/capture_exceptions.rb:21:in `block in call'",
"sentry-ruby (5.21.0) lib/sentry/hub.rb:59:in `with_scope'",
"sentry-ruby (5.21.0) lib/sentry-ruby.rb:392:in `with_scope'",
"sentry-ruby (5.21.0) lib/sentry/rack/capture_exceptions.rb:20:in `call'",
"actionpack (7.0.8.4) lib/action_dispatch/middleware/show_exceptions.rb:29:in `call'",
"lib/gitlab/middleware/basic_health_check.rb:25:in `call'",
"lograge (0.11.2) lib/lograge/rails_ext/rack/logger.rb:15:in `call_app'",
"railties (7.0.8.4) lib/rails/rack/logger.rb:25:in `block in call'",
"activesupport (7.0.8.4) lib/active_support/tagged_logging.rb:99:in `block in tagged'",
"activesupport (7.0.8.4) lib/active_support/tagged_logging.rb:37:in `tagged'",
"activesupport (7.0.8.4) lib/active_support/tagged_logging.rb:99:in `tagged'",
"railties (7.0.8.4) lib/rails/rack/logger.rb:25:in `call'",
"actionpack (7.0.8.4) lib/action_dispatch/middleware/remote_ip.rb:93:in `call'",
"lib/gitlab/middleware/handle_ip_spoof_attack_error.rb:25:in `call'",
"lib/gitlab/middleware/request_context.rb:15:in `call'",
"lib/gitlab/middleware/webhook_recursion_detection.rb:15:in `call'",
"request_store (1.5.1) lib/request_store/middleware.rb:19:in `call'",
"rack (2.2.10) lib/rack/method_override.rb:24:in `call'",
"rack (2.2.10) lib/rack/runtime.rb:22:in `call'",
"rack-timeout (0.7.0) lib/rack/timeout/core.rb:154:in `block in call'",
"rack-timeout (0.7.0) lib/rack/timeout/support/timeout.rb:19:in `timeout'",
"rack-timeout (0.7.0) lib/rack/timeout/core.rb:153:in `call'",
"config/initializers/fix_local_cache_middleware.rb:11:in `call'",
"lib/gitlab/middleware/compressed_json.rb:44:in `call'",
"actionpack (7.0.8.4) lib/action_dispatch/middleware/executor.rb:14:in `call'",
"lib/gitlab/middleware/rack_multipart_tempfile_factory.rb:19:in `call'",
"rack (2.2.10) lib/rack/sendfile.rb:110:in `call'",
"lib/gitlab/middleware/sidekiq_web_static.rb:20:in `call'",
"lib/gitlab/metrics/requests_rack_middleware.rb:79:in `call'",
"gitlab-labkit (0.36.1) lib/labkit/middleware/rack.rb:22:in `block in call'",
"gitlab-labkit (0.36.1) lib/labkit/context.rb:35:in `with_context'",
"gitlab-labkit (0.36.1) lib/labkit/middleware/rack.rb:21:in `call'",
"actionpack (7.0.8.4) lib/action_dispatch/middleware/request_id.rb:26:in `call'",
"actionpack (7.0.8.4) lib/action_dispatch/middleware/host_authorization.rb:131:in `call'",
"railties (7.0.8.4) lib/rails/engine.rb:530:in `call'",
"railties (7.0.8.4) lib/rails/railtie.rb:226:in `public_send'",
"railties (7.0.8.4) lib/rails/railtie.rb:226:in `method_missing'",
"lib/gitlab/middleware/release_env.rb:12:in `call'",
"rack (2.2.10) lib/rack/urlmap.rb:74:in `block in call'",
"rack (2.2.10) lib/rack/urlmap.rb:58:in `each'",
"rack (2.2.10) lib/rack/urlmap.rb:58:in `call'",
"puma (6.5.0) lib/puma/configuration.rb:279:in `call'",
"puma (6.5.0) lib/puma/request.rb:99:in `block in handle_request'",
"puma (6.5.0) lib/puma/thread_pool.rb:389:in `with_force_shutdown'",
"puma (6.5.0) lib/puma/request.rb:98:in `handle_request'",
"puma (6.5.0) lib/puma/server.rb:468:in `process_client'",
"puma (6.5.0) lib/puma/server.rb:249:in `block in run'",
"puma (6.5.0) lib/puma/thread_pool.rb:166:in `block in spawn_thread'"
],
"exception.cause_class": "PG::NotNullViolation",
"exception.sql": "/*application:web,correlation_id:01JDQ0QDFE54MNP1BR3B9K5J1B,endpoint_id:Projects::MergeRequests::DraftsController#publish,db_config_database:gitlabhq_production,db_config_name:main*/ INSERT INTO \"diff_note_positions\" (\"base_sha\",\"start_sha\",\"head_sha\",\"old_path\",\"new_path\",\"old_line\",\"new_line\",\"diff_content_type\",\"diff_type\",\"line_code\",\"note_id\") VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) ON CONFLICT (\"note_id\",\"diff_type\") DO UPDATE SET \"base_sha\"=excluded.\"base_sha\",\"start_sha\"=excluded.\"start_sha\",\"head_sha\"=excluded.\"head_sha\",\"old_path\"=excluded.\"old_path\",\"new_path\"=excluded.\"new_path\",\"old_line\"=excluded.\"old_line\",\"new_line\"=excluded.\"new_line\",\"diff_content_type\"=excluded.\"diff_content_type\",\"line_code\"=excluded.\"line_code\" RETURNING \"id\"",
"db_duration_s": 0.36615,
"view_duration_s": 0.0,
"duration_s": 2.74249
}We may want to add note.valid? or note.note.present?:
diff --git a/app/services/draft_notes/publish_service.rb b/app/services/draft_notes/publish_service.rb
index c4ebaf7aab8c..385e0d6386ed 100644
--- a/app/services/draft_notes/publish_service.rb
+++ b/app/services/draft_notes/publish_service.rb
@@ -92,7 +92,7 @@ def capture_diff_note_positions(notes)
capture_service = Discussions::CaptureDiffNotePositionService.new(merge_request, paths.compact)
notes.each do |note|
- capture_service.execute(note.discussion) if note.diff_note? && note.start_of_discussion?
+ capture_service.execute(note.discussion) if note.diff_note? && note.start_of_discussion? && note.note.present?
end
endEdited by Stan Hu