Revision history erratically loads and removes historic changes

[Tony-metabase]

Describe the bug
Revision history erratically loads historic changes

To Reproduce
Steps to reproduce the behavior (if you can reproduce the bug using the Sample Database, we will find the issue faster):

1. Go to New -> Dashboard -> Edit -> Add a textbox -> Save (notice 2 revisions)

2. Edit -> Add a textbox -> Save -> Repeat for more than 8 times --- Notice how only 7 revisions will show

3. Edit -> Type something in just 1 textbox -> Save (notice that the Revision list went down form 7 to 6)

4. Edit -> Type something in just 1 textbox -> Save (repeat this for 7 times)

Notice how the revision history vanished. To add with the above You will see that the revision history is always consistent via the API and it always limits to 14

Expected behavior
Consistency with the API

Information about your Metabase Installation:

Happens in 1.46.1 and master

Additional context

Please note that in master for some reason only 4 revisions show up

[EmmadUsmani]

I dug into it and it appears to be a backend issue, as the backend is not correctly computing the revision history.

After creating the dashboard and adding 5 cards, this is what the API (/api/revision?entity=dashboard&id=<id>) returns

The last element in the array is the creation revision (is_creation: true). The one before that is the addition of the first card, description is added a card. and the diff object is populated.

At this point the sidebar still renders the history correctly, although there are still errors in the API response. The other revisions for adding the later cards have incorrect diff objects.

cards in before is null (should be populated with the cards that were already on the dashboard), and after shows the first two cards as null. Also, in the first screenshot we see that there are many extra revision objects (a total of 15 when only a 5 cards were added). Most of these revisions are invalid. They don't have a description or diff, nor are they creations or reversions since is_created and is_reversion are false.

After this point, adding a sixth card causes the revision history to be more corrupt to the point where is starts appearing erroneous on the frontend.

The creation event is no longer in the array (I'm assuming this is because we only return 15 revisions, although most of these are invalid), and the earliest card add revision has lost its diff object, which is now null. Because the diff object is null, the frontend treats this revision as invalid and filters it out of the list, which is why the client then only shows 4 You added a card revisions (assuming another one was lost due to the 15 revision limit).

After adding text to some cards, the revision history returned by the API is populated with more "phantom" revisions that are invalid, which causes the card add revisions to be pushed out of the array, which is why they start disappearing in the sidebar.

[dpsutton]

Some notes:
the revision is not corrupt. the FE needs to handle this.

the way revisions work is that they have an id and a copy of the object at that time. To revert to that point, you hit the endpoint

(api/defendpoint-schema POST "/revert"
  "Revert an object to a prior revision."
  [:as {{:keys [entity id revision_id]} :body}]
...)

the important bits are the revision_id. Everything else is for consistency.

Here's what we store in a single revision:

{:is_creation true,
  :model_id 451,
  :id 341,
  :is_reversion false,
  :user_id 1,
  :timestamp #object[java.time.OffsetDateTime
                     "0x21d2410a"
                     "2023-04-17T22:08:12.709263Z"],
  :object {:description nil,
           :name "dash with revisions",
           :cache_ttl nil,
           :cards []},
  :message nil,
  :model "Dashboard"}

This is the initial dashboard creation (note :is_creation true,). When you add a card to it you get the following revision:

{:is_creation false,
  :model_id 451,
  :id 342,
  :is_reversion false,
  :user_id 1,
  :timestamp #object[java.time.OffsetDateTime
                     "0x12edaa4a"
                     "2023-04-17T22:08:23.693741Z"],
  :object {:description nil,
           :name "dash with revisions",
           :cache_ttl nil,
           :cards [{:size_x 4,
                    :size_y 4,
                    :row 0,
                    :col 0,
                    :id 614,
                    :card_id nil,
                    :series []}]},
  :message nil,
  :model "Dashboard"}

And the diff is that is has a card added. These are gotten from evaluating (revision/revisions Dashboard dash-id).

Where this gets more confusing is that we don't send exactly this to the FE. We send the results of (revision/revisions+details Dashboard dash-id) over. This does not send the object but tries to get a meaningful diff and create a diff string. We aren't always perfect at this so we should see if we can extend this for text cards and link cards.

One issue we have is that we create far more revisions than we need to due to the event system. An area for improvement is to not save a revision if it is identical to the previous object. This was attempted to be solved on the FE by ignoring revisions with an empty diff string. But it sounds like we are making too many and the FE is collapsing them all out. So it's both a FE and BE bug. We should prevent making so many diffs and try to be better at our diffstrings. But the fact that the :diff key is null is not really indicative of an error and will occur. We only keep the 15 most recent copies of an object so at some point there is no previous version so there will always be a null key involved.

[qnkhuat]

The main reason this happened is that when updating the dashboard, more than one revision is recorded with no change the same object.

For example, on 46, if I add a dashcard, we'll have 2 revisions with topics dashboard-update and dashboard-add-cards; both have the same revision object.

This is even trickier on master because we recently did a Refactor editing dashboard's cards in which we combined PUT/DELETE/POST api/dashboard/:id/cards into one single PUT api/dashboard/:id/cards.

With this, when adding a card to a dashboard you'll have at least 3 revisions with the same object dashboard-update, dashboard-add-cards, dashboard-reposition-cards.

So I think the solution is as Dan suggested: record a revision if and only if the object changes.

But in addition to that, we also should make sure when :dashboard-add-cards, dashboard-remove-cards, dashboard-reposition-cards are combined into one single event(maybe dashboard-cards-update) so that we don't fire 3 events on calling PUT /api/dashboard/:id/cards. ( this only applies for master)