feat(backend revision): add clean-up note revisions job #5349

yamashush · 2024-01-14T11:25:23Z

Component/Part

backend revision

Description

This PR adds note revisions clean-up job.

A new config option HD_REVISION_RETENTION_DAYS that sets the number of days a revision should be kept. A regular nest cronjob will clean all note revisions that are due for deletion.
If the config option is not set or set to 0, all revisions will be kept forever.

Steps

Added implementation
Added / updated tests
Added / updated documentation
I read the contribution documentation and
made sure that:
- My commits are signed-off to accept the DCO.
- This PR targets the correct branch: master for 1.x & docs, develop for 2.x

Related Issue(s)

Configurable note revision clean-up #3727

backend/src/revisions/revisions.service.ts

mrdrogdrog · 2024-01-18T20:10:40Z

Please rebase your branch to the latest version of develop to fix the CI.

yamashush · 2024-01-19T00:12:54Z

Please rebase your branch to the latest version of develop to fix the CI.

Thank you for letting me know.
After rebase, the test seems to work now!

codecov · 2024-01-19T11:33:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.82%. Comparing base (e7f33c9) to head (733afe0).
Report is 39 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5349      +/-   ##
===========================================
+ Coverage    57.66%   57.82%   +0.16%     
===========================================
  Files          418      419       +1     
  Lines        12058    12064       +6     
  Branches      1007      903     -104     
===========================================
+ Hits          6953     6976      +23     
- Misses        5048     5088      +40     
+ Partials        57        0      -57

Flag	Coverage Δ
backend	`87.67% <100.00%> (+0.04%)`	⬆️
backend-e2e-tests	`74.25% <76.92%> (-0.10%)`	⬇️
backend-unit-tests	`85.85% <84.61%> (+0.12%)`	⬆️
e2e-tests	`74.25% <76.92%> (-0.10%)`	⬇️
frontend	`39.54% <ø> (+0.02%)`	⬆️
frontend-unit-tests	`39.54% <ø> (+0.02%)`	⬆️
unit-tests	`51.96% <84.61%> (+0.16%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ErikMichelson

In general this is a good starting point, thanks for that!

However when deleting old revisions, we need to somehow compensate the missing information, don't we? A revision holds the diff (called patch in the entity) to its preceding revision. This is used by the revision viewer to show the differences. When deleting old revisions, this results in an invalid diff. So the oldest not removed revision needs its diff be updated.

Or should we do it differently, @hedgedoc/backend @hedgedoc/frontend ?

ErikMichelson · 2024-01-24T11:22:12Z

docs/content/references/config/revision.md

+| ---------------------------- | ------- | ------- | ------------------------------------------------------------------------------------------------------------------------------ |
+| `HD_REVISION_RETENTION_DAYS` | 0       |         | The number of days a revision should be kept. If the config option is not set or set to 0, all revisions will be kept forever. |
+<!--
+| `HD_IMAGE_PROXY` | -       | `https://image-proxy.example.com`   | **ToDo:** Add description  |


Why is this env variable in the revision section?

I did this because I thought it would be better to separate it by entity.
But maybe it should be in the notes section?

@ErikMichelson I moved it from revesion section to note section!

yamashush · 2024-01-27T11:09:43Z

However when deleting old revisions, we need to somehow compensate the missing information, don't we? A revision holds the diff (called patch in the entity) to its preceding revision. This is used by the revision viewer to show the differences. When deleting old revisions, this results in an invalid diff. So the oldest not removed revision needs its diff be updated.

I see, revisions have not only content but also diff (patch).
Thank you for making me realize that!!

I was referring to the code below that is called from the revision modal.
Does it seem like the diff(patch) needs to be updated here as well?

hedgedoc/backend/src/revisions/revisions.service.ts

Line 63 in 9e31503

return await this.revisionRepository.remove(oldRevisions);

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

backend/src/revisions/revisions.service.spec.ts

+        note.publicId,
+        revision2.content,
+        revision3.content
+      )


ErikMichelson · 2024-05-25T23:56:39Z

backend/src/revisions/revisions.service.spec.ts

+        note.publicId,
+        revision1.content,
+        revision2.content
+      )


This seems to be something that can be fixed throughout the whole project by just running the lint-fix command (yarn lint:fix).

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

yamashush · 2024-05-05T09:29:38Z

@ErikMichelson I have implemented updating patch (content diff), so could you please review it again?

ErikMichelson

Thanks for your contribution and sorry again that this took a while.

I've annotated some minor things.

ErikMichelson · 2024-05-25T23:54:27Z

backend/src/config/note.config.spec.ts

+        },
+      );
+      expect(() => noteConfig()).toThrow(
+        '"forbiddenNoteIds[0]" is not allowed to be empty',


Shouldn't we expect a different error in this case?

ErikMichelson · 2024-05-25T23:55:15Z

backend/src/config/note.config.spec.ts

+      const restore = mockedEnv(
+        {
+          /* eslint-disable @typescript-eslint/naming-convention */
+          HD_FORBIDDEN_NOTE_IDS: invalidforbiddenNoteIds.join(' , '),


Here we should use not the invalid mock data to only test one error case at once.

ErikMichelson · 2024-05-25T23:56:39Z

backend/src/revisions/revisions.service.spec.ts

+        note.publicId,
+        revision1.content,
+        revision2.content
+      )


This seems to be something that can be fixed throughout the whole project by just running the lint-fix command (yarn lint:fix).

ErikMichelson · 2024-05-26T00:00:04Z

backend/src/revisions/revisions.service.spec.ts

+      expect(revision3.patch).toMatchInlineSnapshot(`
+        "Index: test-note
+        ===================================================================
+        --- test-note
+        +++ test-note
+        @@ -0,0 +1,6 @@
+        +---
+        +title: new title
+        +description: new description
+        +tags: [ "tag1" ]
+        +---
+        +new content
+        "
+      `);
+    });


Like already stated in the other PR, I think extracting the embedded snapshots into their own "normal snapshot" file would be good.

ErikMichelson · 2024-05-26T00:04:26Z

backend/src/revisions/revisions.service.ts

+
+  // Delete all old revisions everyday on 0:00 AM
+  @Cron('0 0 * * *')
+  async handleCron(): Promise<void> {


I think we should rename these methods (handleCron and handleTimeout) to indicate that they're used in the context of revision cleanup. Maybe something like handleRevisionCleanupTimeout etc?

ErikMichelson · 2024-05-26T00:05:20Z

backend/src/revisions/revisions.service.ts

+    return await this.removeOldRevisions();
+  }
+
+  async removeOldRevisions(): Promise<void> {


It would be good to have a block of ESDoc here

ErikMichelson · 2024-05-26T00:07:08Z

backend/src/revisions/revisions.service.ts

+      if (!oldRevisions.length) {
+        continue;
+
+      } else if (oldRevisions.length == revisions.length - 1 ){


Please use explicit comparisons always (===).

ErikMichelson · 2024-05-26T00:08:11Z

backend/src/revisions/revisions.service.ts

+        .filter(
+          (revision) =>
+            new Date(revision.createdAt).getTime() <=
+            currentTime - revisionRetentionDays * 24 * 60 * 60 * 1000,


To improve performance, this second value could be calculated outside the loop, so before the .filter method call.

ErikMichelson · 2024-05-26T00:09:41Z

backend/src/revisions/revisions.service.ts

+        const beUpdatedRevision = revisions.slice(-1)[0]
+        beUpdatedRevision.patch = createPatch(
+          note.publicId,
+          '', // donnt exist older revision


Suggested change

'', // donnt exist older revision

'', // there is no older revision

ErikMichelson reviewed Jan 14, 2024

View reviewed changes

backend/src/revisions/revisions.service.ts Outdated Show resolved Hide resolved

yamashush changed the title ~~[wip] feat(backend revision): clean-up note revisions~~ [wip] feat(backend revision): add clean-up note revisions job Jan 16, 2024

yamashush marked this pull request as ready for review January 16, 2024 13:48

yamashush changed the title ~~[wip] feat(backend revision): add clean-up note revisions job~~ feat(backend revision): add clean-up note revisions job Jan 16, 2024

ErikMichelson added the type: feature Adds or requests new functionality label Jan 17, 2024

ErikMichelson added this to the Version 2.0 milestone Jan 17, 2024

ErikMichelson assigned yamashush Jan 17, 2024

yamashush force-pushed the remove-old-revision branch from 10d84da to 1282f66 Compare January 18, 2024 23:03

yamashush force-pushed the remove-old-revision branch 2 times, most recently from 5219200 to 20dbecc Compare January 19, 2024 11:31

ErikMichelson reviewed Jan 24, 2024

View reviewed changes

ErikMichelson linked an issue Mar 25, 2024 that may be closed by this pull request

Configurable note revision clean-up #3727

Open

yamashush added 11 commits April 25, 2024 21:35

test: add cron & timeout tests

c6e30b0

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

feat: add cron for removing old revisions

200af40

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

feat: remove old revisions

a996eb6

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

test: add simple test case

8c6de84

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

test: add removing nothing case

fd7738c

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

feat: add revision config

987d740

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

test: add revision config mock

97adf9c

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

test: add test case when config value is zero

19bae0b

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

doc: add config doc

7d06e46

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

lint: fix lint errors

a7a649f

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

lint: fix lint errors

c06922e

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

DerMolly force-pushed the remove-old-revision branch from 20dbecc to c06922e Compare April 25, 2024 19:35

yamashush added 2 commits May 5, 2024 15:12

test: add boundary test case

7992f44

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

refactor: move sort ope into SQL

30a3ddb

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

test: add updated test of revision patch

fc1ce37

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

github-advanced-security bot found potential problems May 5, 2024

View reviewed changes

yamashush force-pushed the remove-old-revision branch from 6b8fe92 to 2106f68 Compare May 5, 2024 08:14

yamashush added 2 commits May 5, 2024 17:17

feat: add updating revision patch when removing old revision

30652d0

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

chore: fix end commma

078dc96

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

yamashush force-pushed the remove-old-revision branch from 2106f68 to 078dc96 Compare May 5, 2024 08:21

yamashush added 2 commits May 5, 2024 18:19

refactor: move configs from revision to note

883677d

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

refactor: move configs from revision to note

733afe0

Signed-off-by: yamashush <38120991+yamashush@users.noreply.github.com>

yamashush mentioned this pull request May 9, 2024

fix(backend revision): Update patch column when removing revisions #5574

Open

3 tasks

ErikMichelson reviewed May 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(backend revision): add clean-up note revisions job #5349

feat(backend revision): add clean-up note revisions job #5349

yamashush commented Jan 14, 2024 •

edited

mrdrogdrog commented Jan 18, 2024

yamashush commented Jan 19, 2024

codecov bot commented Jan 19, 2024 •

edited

ErikMichelson left a comment

ErikMichelson Jan 24, 2024

yamashush Jan 27, 2024

yamashush May 5, 2024

yamashush commented Jan 27, 2024

ErikMichelson May 25, 2024

yamashush commented May 5, 2024

ErikMichelson left a comment

ErikMichelson May 25, 2024

ErikMichelson May 25, 2024

ErikMichelson May 25, 2024

ErikMichelson May 26, 2024

ErikMichelson May 26, 2024

ErikMichelson May 26, 2024

ErikMichelson May 26, 2024

ErikMichelson May 26, 2024

ErikMichelson May 26, 2024

	'', // donnt exist older revision
	'', // there is no older revision

feat(backend revision): add clean-up note revisions job #5349

Are you sure you want to change the base?

feat(backend revision): add clean-up note revisions job #5349

Conversation

yamashush commented Jan 14, 2024 • edited

Component/Part

Description

Steps

Related Issue(s)

mrdrogdrog commented Jan 18, 2024

yamashush commented Jan 19, 2024

codecov bot commented Jan 19, 2024 • edited

Codecov Report

ErikMichelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamashush commented Jan 27, 2024

Choose a reason for hiding this comment

yamashush commented May 5, 2024

ErikMichelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamashush commented Jan 14, 2024 •

edited

codecov bot commented Jan 19, 2024 •

edited