Sending an empty note in diff results in 500 error
Summary
Sending an empty note in a diff results in a 500 error. An empty note is not normally possible via the UI but if you use the keyboard shortcut (CTRL+Enter/CMD+Enter), it's possible to send an empty note.
Steps to reproduce
- Create a new comment on an MR's diff
- CTRL+Enter/CMD+Enter to start the review
- Submit review
Example Project
What is the current bug behavior?
Empty comment throws 500 error.
What is the expected correct behavior?
Empty comment either cannot be sent and/or backend inserts into the DB but with an empty string value rather than a NULL
.
Relevant logs and/or screenshots
The contents of the POST to GitLab.com has an empty string:
{"noteable_type":"MergeRequest","noteable_id":269987583,"note":"","approve":false,"approval_password":"","reviewer_state":"reviewed"}
Error reported by GitLab.com
{
"action": "publish",
"cf_ray": "8375b695d8361a02-KIX",
"component": "gitlab",
"controller": "Projects::MergeRequests::DraftsController",
"correlation_id": "01HHXXEPGFTP0CQHPT0M92JFR0",
"cpu_s": 0.326046,
"db_cached_count": 7,
"db_ci_cached_count": 0,
"db_ci_count": 0,
"db_ci_duration_s": 0,
"db_ci_replica_cached_count": 0,
"db_ci_replica_count": 0,
"db_ci_replica_duration_s": 0,
"db_ci_replica_wal_cached_count": 0,
"db_ci_replica_wal_count": 0,
"db_ci_wal_cached_count": 0,
"db_ci_wal_count": 0,
"db_count": 29,
"db_duration_s": 0.03498,
"db_embedding_cached_count": 0,
"db_embedding_count": 0,
"db_embedding_duration_s": 0,
"db_embedding_replica_cached_count": 0,
"db_embedding_replica_count": 0,
"db_embedding_replica_duration_s": 0,
"db_embedding_replica_wal_cached_count": 0,
"db_embedding_replica_wal_count": 0,
"db_embedding_wal_cached_count": 0,
"db_embedding_wal_count": 0,
"db_main_cached_count": 2,
"db_main_count": 13,
"db_main_duration_s": 0.016,
"db_main_replica_cached_count": 5,
"db_main_replica_count": 16,
"db_main_replica_duration_s": 0.01,
"db_main_replica_wal_cached_count": 0,
"db_main_replica_wal_count": 0,
"db_main_wal_cached_count": 0,
"db_main_wal_count": 0,
"db_primary_cached_count": 2,
"db_primary_count": 13,
"db_primary_duration_s": 0.016,
"db_primary_wal_cached_count": 0,
"db_primary_wal_count": 0,
"db_replica_cached_count": 5,
"db_replica_count": 16,
"db_replica_duration_s": 0.01,
"db_replica_wal_cached_count": 0,
"db_replica_wal_count": 0,
"db_write_count": 2,
"duration_s": 0.37798,
"environment": "gprd",
"exception.backtrace": [
"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:235: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>'",
"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:99:in `block in capture_diff_note_positions'",
"app/services/draft_notes/publish_service.rb:98:in `each'",
"app/services/draft_notes/publish_service.rb:98:in `capture_diff_note_positions'",
"app/services/draft_notes/publish_service.rb:45:in `publish_draft_notes'",
"app/services/draft_notes/publish_service.rb:11:in `execute'",
"app/controllers/projects/merge_requests/drafts_controller.rb:56:in `publish'",
"ee/lib/gitlab/ip_address_state.rb:10:in `with'",
"ee/app/controllers/ee/application_controller.rb:45:in `set_current_ip_address'",
"app/controllers/application_controller.rb:470:in `set_current_admin'",
"lib/gitlab/session.rb:11:in `with_session'",
"app/controllers/application_controller.rb:461:in `set_session_storage'",
"lib/gitlab/i18n.rb:114:in `with_locale'",
"lib/gitlab/i18n.rb:120:in `with_user_locale'",
"app/controllers/application_controller.rb:452:in `set_locale'",
"app/controllers/application_controller.rb:445:in `set_current_context'",
"ee/lib/omni_auth/strategies/group_saml.rb:41:in `other_phase'",
"lib/gitlab/metrics/elasticsearch_rack_middleware.rb:16: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:20:in `call'",
"lib/gitlab/middleware/query_analyzer.rb:11:in `block in call'",
"lib/gitlab/database/query_analyzer.rb:37:in `within'",
"lib/gitlab/middleware/query_analyzer.rb:11:in `call'",
"lib/gitlab/middleware/multipart.rb:173:in `call'",
"lib/gitlab/middleware/read_only/controller.rb:50:in `call'",
"lib/gitlab/middleware/read_only.rb:18:in `call'",
"lib/gitlab/middleware/same_site_cookies.rb:27:in `call'",
"lib/gitlab/middleware/path_traversal_check.rb:35:in `call'",
"lib/gitlab/middleware/handle_malformed_strings.rb:21:in `call'",
"lib/gitlab/middleware/basic_health_check.rb:25: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'",
"config/initializers/fix_local_cache_middleware.rb:11:in `call'",
"lib/gitlab/middleware/compressed_json.rb:44:in `call'",
"lib/gitlab/middleware/rack_multipart_tempfile_factory.rb:19:in `call'",
"lib/gitlab/middleware/sidekiq_web_static.rb:20:in `call'",
"lib/gitlab/metrics/requests_rack_middleware.rb:79:in `call'",
"lib/gitlab/middleware/release_env.rb:13:in `call'"
],
"exception.cause_class": "PG::NotNullViolation",
"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 (227827481, null, null, 13, 0, 0, 8ec9a00bfd09b3190ac6b22251dbb1aa95a0579d_13_13, \\x33343036666136303236633461356137393637356630313730306537343537..., \\x33343036666136303236633461356137393637356630313730306537343537..., \\x39383531366231643764613165653433343430303534373831343039373131..., README.md, README.md).\n",
"exception.sql": "/*application:web,correlation_id:01HHXXEPGFTP0CQHPT0M92JFR0,endpoint_id:Projects::MergeRequests::DraftsController#publish,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\"",
"format": "json",
"gitaly_calls": 13,
"gitaly_duration_s": 0.164586,
"logtag": "F",
"mem_bytes": 9910664,
"mem_mallocs": 44941,
"mem_objects": 131490,
"mem_total_bytes": 15170264,
"meta.caller_id": "Projects::MergeRequests::DraftsController#publish",
"meta.client_id": "user/5749275",
"meta.feature_category": "code_review_workflow",
"meta.project": "mbadeau/zd-477403",
"meta.remote_ip": "",
"meta.root_namespace": "mbadeau",
"meta.user": "mbadeau",
"meta.user_id": 5749275,
"method": "POST",
"params": [
{
"key": "noteable_type",
"value": "MergeRequest"
},
{
"key": "noteable_id",
"value": "269987583"
},
{
"key": "note",
"value": "[FILTERED]"
},
{
"key": "approve",
"value": "false"
},
{
"key": "approval_password",
"value": "[FILTERED]"
},
{
"key": "reviewer_state",
"value": "reviewed"
},
{
"key": "namespace_id",
"value": "mbadeau"
},
{
"key": "project_id",
"value": "zd-477403"
},
{
"key": "merge_request_id",
"value": "1"
},
{
"key": "draft",
"value": "{\"noteable_type\"=>\"MergeRequest\", \"noteable_id\"=>269987583, \"note\"=>\"[FILTERED]\", \"approve\"=>false, \"approval_password\"=>\"[FILTERED]\", \"reviewer_state\"=>\"reviewed\"}"
}
],
"path": "/mbadeau/zd-477403/-/merge_requests/1/drafts/publish",
"pid": 71,
"queue_duration_s": 0.022256,
"rate_limiting_gates": [],
"redis_allowed_cross_slot_calls": 1,
"redis_calls": 13,
"redis_cluster_shared_state_calls": 1,
"redis_cluster_shared_state_duration_s": 0.000478,
"redis_cluster_shared_state_read_bytes": 2,
"redis_cluster_shared_state_write_bytes": 94,
"redis_db_load_balancing_calls": 3,
"redis_db_load_balancing_duration_s": 0.000999,
"redis_db_load_balancing_write_bytes": 180,
"redis_duration_s": 0.005646,
"redis_feature_flag_calls": 1,
"redis_feature_flag_duration_s": 0.000617,
"redis_feature_flag_read_bytes": 199,
"redis_feature_flag_write_bytes": 67,
"redis_queues_calls": 2,
"redis_queues_duration_s": 0.000523,
"redis_queues_metadata_calls": 1,
"redis_queues_metadata_duration_s": 0.000868,
"redis_queues_metadata_read_bytes": 2,
"redis_queues_metadata_write_bytes": 208,
"redis_queues_read_bytes": 6,
"redis_queues_write_bytes": 840,
"redis_rate_limiting_calls": 1,
"redis_rate_limiting_duration_s": 0.000555,
"redis_rate_limiting_read_bytes": 2,
"redis_rate_limiting_write_bytes": 78,
"redis_read_bytes": 1016,
"redis_repository_cache_calls": 1,
"redis_repository_cache_duration_s": 0.000467,
"redis_repository_cache_read_bytes": 102,
"redis_repository_cache_write_bytes": 50,
"redis_sessions_allowed_cross_slot_calls": 1,
"redis_sessions_calls": 3,
"redis_sessions_duration_s": 0.001139,
"redis_sessions_read_bytes": 703,
"redis_sessions_write_bytes": 628,
"redis_write_bytes": 2145,
"remote_ip": "",
"request_urgency": "low",
"shard": "default",
"stage": "main",
"status": 500,
"subcomponent": "production_json",
"target_duration_s": 5,
"tier": "sv",
"time": "2023-12-18T07:32:40.496Z",
"type": "web",
"ua": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:120.0) Gecko/20100101 Firefox/120.0",
"user_id": 5749275,
"username": "mbadeau",
"view_duration_s": 0,
"worker_id": "puma_2"
}
Output of checks
This bug happens on GitLab.com
Results of GitLab environment info
Expand for output related to GitLab environment info
(For installations with omnibus-gitlab package run and paste the output of: `sudo gitlab-rake gitlab:env:info`) (For installations from source run and paste the output of: `sudo -u git -H bundle exec rake gitlab:env:info RAILS_ENV=production`)
Results of GitLab application Check
Expand for output related to the GitLab application check
(For installations with omnibus-gitlab package run and paste the output of:
sudo gitlab-rake gitlab:check SANITIZE=true
)(For installations from source run and paste the output of:
sudo -u git -H bundle exec rake gitlab:check RAILS_ENV=production SANITIZE=true
)(we will only investigate if the tests are passing)