Skip to content

Only set note visibility_level to if associated with a project

Terri Chu requested to merge terrichu-fix-notes-to-es-support-null-projects into master

Discovered while running the ES migration to backfill notes permission data: !53406 (merged)

Background

That MR was reverted to avoid having the migration run in production and I decided to open a smaller MR to fix the issue before attempting to re-merge the migration. The errors were not from the migration but from the as_indexed_json method for notes. However, it was the right decision to revert the migration because it would have retried the same batch of documents forever. There is a low risk of having this error occur in production during normal create/update index operations because it is due to inconsistent data. Valid indexed comments (notes) always have a project associated. Epics/Personal Snippets are not indexed. The migration MR will be modified to only query the searachable types of notes (Issue, MergeRequest, Snippet, and Commit)

What does this MR do?

Related to #300351 (closed)

There are notes in the database which do not have an associated project (see details in thread below on the data in staging). This was causing errors when indexing the notes to Elasticsearch.

This MR fixes that issue by only setting notes visibility_level to if there is an associated project. Updated specs to confirm the fix and tested manually.

How to test

  • Create a note that has no associated project (assuming variable name note was used, note.project should be nil). Use a note on an Epic or Personal Snippet.

  • Run note.__elasticsesarch__.as_indexed_json

  • Verify there is no exception thrown

  • Verify that visibility_level is not set

Screenshots (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by Terri Chu

Merge request reports