Skip to content

Treat empty markdown and html strings as valid cached text, not missing cache…

Gregory Stark requested to merge optimize-issues-avoid-noop-empty-cache-updates2 into master

What does this MR do?

Are there points in the code the reviewer needs to double check?

I don't know how exhaustive the tests are for this class. I don't see any tests for a newly created object, only updates to existing objects so I'm not clear if newly created objects are created with empty text or nil text or what.

Also, note that I don't know what I'm doing with rspec tests. They seem to be working but it's entirely possible I've done something crazy here.

Why was this MR needed?

Every page load of an issue (or presumable a MR) with an empty description or a description which renders as empty HTML would treat the empty cached HTML as a missing cached html and perform at least one and in some cases two updates against the table.

e.g. sytses/test-2#1 does:

13.406ms	UPDATE "issues" SET "title_html" = 'Test', "description_html" = '', "cached_markdown_version" = 2 WHERE "issues"."id" = 5462937

and https://gitlab.com/gitlab-org/gitlab-ce/issues/41006 does:

23.559ms	UPDATE "issues" SET "title_html" = 'test issue', "description_html" = '', "cached_markdown_version" = 2 WHERE "issues"."id" = 8148101
 9.323ms	UPDATE "issues" SET "title_html" = 'test issue', "description_html" = '', "cached_markdown_version" = 2 WHERE "issues"."id" = 8148101

Screenshots (if relevant)

Tests before:

Failures:

  1) CacheMarkdownField when a markdown field set repeatedly to empty string should receive refresh_markdown_cache(*(any args)) 1 time
     [31mFailure/Error:[0m
     [31m  [0mrun_callbacks [33m:update[0m [32mdo[0m[0m
     [31m    [0mchanges_applied[0m
     [31m  [0m[32mend[0m[0m
     [31m[0m
     [31m  (#<ThingWithMarkdownFields:0x00562ce4b94940 @changed_attributes={}, @foo="", @foo_html="<p dir=\"auto\"><code>Foo</code></p>", @cached_markdown_version=2, @previously_changed={"foo"=>["`Foo`", ""]}>).refresh_markdown_cache(no args)[0m
     [31m      expected: 1 time with any arguments[0m
     [31m      received: 2 times[0m
     [36m# ./spec/models/concerns/cache_markdown_field_spec.rb:59:in `save'[0m
     [36m# ./spec/models/concerns/cache_markdown_field_spec.rb:111:in `block (3 levels) in <top (required)>'[0m

  2) CacheMarkdownField a markdown field set repeatedly to a string which renders as empty html should receive refresh_markdown_cache(*(any args)) 1 time
     [31mFailure/Error:[0m
     [31m  [0mrun_callbacks [33m:update[0m [32mdo[0m[0m
     [31m    [0mchanges_applied[0m
     [31m  [0m[32mend[0m[0m
     [31m[0m
     [31m  (#<ThingWithMarkdownFields:0x00562ce4c1a1a8 @changed_attributes={}, @foo="[//]: # (This is also a comment.)", @foo_html="<p dir=\"auto\"><code>Foo</code></p>", @cached_markdown_version=2, @previously_changed={"foo"=>["`Foo`", "[//]: # (This is also a comment.)"]}>).refresh_markdown_cache(no args)[0m
     [31m      expected: 1 time with any arguments[0m
     [31m      received: 2 times[0m
     [36m# ./spec/models/concerns/cache_markdown_field_spec.rb:59:in `save'[0m
     [36m# ./spec/models/concerns/cache_markdown_field_spec.rb:121:in `block (3 levels) in <top (required)>'[0m

Finished in 39.93 seconds (files took 20.55 seconds to load)
[31m38 examples, 2 failures[0m

And after:

Finished in 23.88 seconds (files took 19.26 seconds to load)
[32m38 examples, 0 failures[0m

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Edited by Gregory Stark

Merge request reports