Rename audit logs table name #15719

remidej · 2023-02-06T10:50:39Z

What does it do?

Rename audit log table from audit_logs to strapi_audit_logs
Auto migrates the table when Strapi detects the admin::audit-logs content type changes the collection name from audit_logs to strapi_audit_logs

Why is it needed?

To be consistent with other internal Strapi tables that use the strapi_ prefix
To help users customers who were previously using a custom-made audit logs solution to migrate to the official one

How to test it?

1st check that audit logs work as usual: make actions in the admin, make sure you see them in audit logs.

Then test the migration:

to simulate being a 4.6.0 user, change your audit log collection name here to audit_logs, as it was before, and restart the server. All previous audit logs will be deleted, but this is expected.
make an action in the admin, you should see it in audit logs
then, to simulate the upgrade to 4.6.1, change the collection name again to strapi_audit_logs and restart the app
go to the audit logs, make sure you see your previous action, AND that it has the user attached to it
make another action in the admin, check that shows up in audit logs as usual

gu-stav · 2023-02-06T10:51:39Z

Would it make sense to create an internal migration for that to minimize the friction?

codecov · 2023-02-06T10:56:00Z

Codecov Report

Base: 47.60% // Head: 67.21% // Increases project coverage by +19.61% 🎉

Coverage data is based on head (cf4d169) compared to base (7ab5789).
Patch has no changes to coverable lines.

Additional details and impacted files

@@             Coverage Diff             @@
##             main   #15719       +/-   ##
===========================================
+ Coverage   47.60%   67.21%   +19.61%     
===========================================
  Files         448     1098      +650     
  Lines       15785    23289     +7504     
  Branches     3388     4110      +722     
===========================================
+ Hits         7514    15654     +8140     
+ Misses       6864     6753      -111     
+ Partials     1407      882      -525

Flag	Coverage Δ
back	`?`
front	`67.21% <ø> (?)`
unit_back	`?`
unit_front	`67.21% <ø> (?)`

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

Impacted Files	Coverage Δ
packages/core/admin/ee/server/register.js
...t-logs-local/lib/content-types/audit-log/schema.js
packages/core/utils/lib/format-yup-error.js
...pi/lib/core/loaders/plugins/get-enabled-plugins.js
.../utils/typescript/lib/generators/schemas/schema.js
...ore/utils/lib/sanitize/visitors/remove-password.js
...kages/utils/typescript/lib/utils/resolve-outdir.js
packages/core/data-transfer/src/strapi/register.ts
...kages/core/admin/server/controllers/content-api.js
...ackages/core/strapi/lib/services/metrics/sender.js
... and 1536 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

remidej · 2023-02-06T11:00:32Z

@gustav I considered it but since the feature has only been available for EE gold users for 2 weeks, I don't think it impacts many users/events, so I'm not sure it's worth the development and the coded added to the codebase. But to be honest I'm not sure how that works either, maybe it's actually easier than the migration guide. Do you maybe know a place where we've done since before so I can have a reference?

Also one of my concerns is that this issue was brought up by users who built their own audit log feature before, using the same table name. If we do an automated migration, I'm afraid we might rename their database table, which would create issues as they don't have the same schema, and they don't want to lose their previous data

gu-stav · 2023-02-06T11:07:07Z

I think technically changing the name of the table would qualify as a breaking change - even if mentioned in the migration guide - and we should make that transition as easy as possible.

@petersg83 might be able to help you how to setup a migration. Imo it is very easy and renaming the table is not very expensive. I see your point about the tables they have created themselves. I think this could be mitigated by also checking whether the table matches a certain structure to make sure it is not one we have created by our audit logs implementation - maybe there are better ways. In any way I think it would be nice if you could try and explore that together if possible :)

remidej · 2023-02-07T11:20:18Z

packages/core/admin/ee/server/register.js

+  if (shouldMigrate) {
+    // Migrate the name audit logs table
+    if (await strapi.db.getSchemaConnection().hasTable('audit_logs')) {
+      await strapi.db.getSchemaConnection().renameTable('audit_logs', 'strapi_audit_logs');


I wanted to store the schema connection in a variable, but it would throw errors, it looks like I have to get a fresh one every time

remidej · 2023-02-07T11:21:45Z

@gu-stav Pierre did help me, so I updated the PR with an auto-migration 🙂

gu-stav · 2023-02-07T12:44:27Z

@remidej Cool thanks. I'll remove myself from the reviewers and add @petersg83 and @Marc-Roig instead :)

derrickmehaffy · 2023-02-07T13:39:01Z

packages/core/admin/ee/server/register.js

+
+  if (shouldMigrate) {
+    // Migrate the main audit logs table
+    if (await strapi.db.getSchemaConnection().hasTable('audit_logs')) {


We should validate a bit more than the name to make sure we aren't renaming someone's custom implementation. Best to verify the model vs table structure.

Hey Derrick, yes that's a definitely a concern we need to have, but do you think that can still occur here? The hasTable here is just an extra check, but when defining shouldMigrate we're checking if the content type admin::audit-logs has had its collection name updated from audit_logs to strapi_audit_logs. With the admin:: prefix I don't think it would impact any user's content type

Unless they used patch package to inject the content-type. Better safe than sorry to ensure the attributes of the collection type match exactly to the DB tables. This is something @gu-stav mentioned in his comment above too.

Got it, I added another check

markkaylor

then, to simulate the upgrade to 4.6.1, change the collection name again to strapi_audit_logs and restart the app
go to the audit logs, make sure you see your previous action, AND that it has the user attached to it

After these steps I lose my previous actions / user

remidej · 2023-02-07T15:37:35Z

@markkaylor that's weird, could you add some comments to check if the migration code is ever reached?

markkaylor · 2023-02-07T15:39:16Z

that's weird, could you add some comments to check if the migration code is ever reached?

Sure I'll dig a bit, but I think it does reach since I do see the table name changed in the db

remidej · 2023-02-07T15:42:16Z

@markkaylor the db will have the strapi_audit_logs table even without the migration, the migration just makes sure it's created by renaming the previous table, instead of from scratch with no content

markkaylor

@remidej, I forgot to pull latest changes 🤦‍♂️, classic. LGTM!

markkaylor

Tested again, looks good.

petersg83

LGTM :)

@petersg83

Changes applied & approved by @petersg83

Rename audit logs table name

41e7d2a

remidej added source: core:admin Source is core/admin package flag: don't merge This PR should not be merged at the moment pr: fix This PR is fixing a bug flag: EE Issues correlates to internal EE ticket labels Feb 6, 2023

Add auto db migration

ee2f5f0

remidej removed the flag: don't merge This PR should not be merged at the moment label Feb 7, 2023

remidej requested review from Feranchz, markkaylor and gu-stav February 7, 2023 11:19

remidej commented Feb 7, 2023

View reviewed changes

Typo

d1a1754

gu-stav requested review from Marc-Roig and petersg83 and removed request for gu-stav February 7, 2023 12:44

derrickmehaffy previously requested changes Feb 7, 2023

View reviewed changes

Fix test

3b0309b

markkaylor reviewed Feb 7, 2023

View reviewed changes

markkaylor previously approved these changes Feb 7, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into fix/audit-log-table

337df63

remidej dismissed markkaylor’s stale review via 337df63 February 7, 2023 16:35

remidej requested review from markkaylor and derrickmehaffy February 7, 2023 16:36

remidej added 2 commits February 7, 2023 20:00

Check schema equality before migration

a3e5ad4

Compare content type schema against db columns

3a33df8

markkaylor previously approved these changes Feb 8, 2023

View reviewed changes

remidej added this to the 4.6.1 milestone Feb 8, 2023

Move migration to dedicated folder

4bbfab1

remidej dismissed markkaylor’s stale review via 4bbfab1 February 8, 2023 10:39

Merge branch 'main' into fix/audit-log-table

cf4d169

petersg83 approved these changes Feb 8, 2023

View reviewed changes

markkaylor approved these changes Feb 8, 2023

View reviewed changes

remidej merged commit 2ecaa96 into main Feb 8, 2023

remidej deleted the fix/audit-log-table branch February 8, 2023 11:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename audit logs table name #15719

Rename audit logs table name #15719

remidej commented Feb 6, 2023 •

edited

gu-stav commented Feb 6, 2023

codecov bot commented Feb 6, 2023 •

edited

remidej commented Feb 6, 2023

gu-stav commented Feb 6, 2023

remidej Feb 7, 2023

remidej commented Feb 7, 2023

gu-stav commented Feb 7, 2023

derrickmehaffy Feb 7, 2023

remidej Feb 7, 2023 •

edited

derrickmehaffy Feb 7, 2023

remidej Feb 7, 2023

markkaylor left a comment

remidej commented Feb 7, 2023

markkaylor commented Feb 7, 2023

remidej commented Feb 7, 2023 •

edited

markkaylor left a comment

markkaylor left a comment

petersg83 left a comment

Rename audit logs table name #15719

Rename audit logs table name #15719

Conversation

remidej commented Feb 6, 2023 • edited

What does it do?

Why is it needed?

How to test it?

gu-stav commented Feb 6, 2023

codecov bot commented Feb 6, 2023 • edited

Codecov Report

remidej commented Feb 6, 2023

gu-stav commented Feb 6, 2023

remidej Feb 7, 2023

Choose a reason for hiding this comment

remidej commented Feb 7, 2023

gu-stav commented Feb 7, 2023

derrickmehaffy Feb 7, 2023

Choose a reason for hiding this comment

remidej Feb 7, 2023 • edited

Choose a reason for hiding this comment

derrickmehaffy Feb 7, 2023

Choose a reason for hiding this comment

remidej Feb 7, 2023

Choose a reason for hiding this comment

markkaylor left a comment

Choose a reason for hiding this comment

remidej commented Feb 7, 2023

markkaylor commented Feb 7, 2023

remidej commented Feb 7, 2023 • edited

markkaylor left a comment

Choose a reason for hiding this comment

markkaylor left a comment

Choose a reason for hiding this comment

petersg83 left a comment

Choose a reason for hiding this comment

remidej commented Feb 6, 2023 •

edited

codecov bot commented Feb 6, 2023 •

edited

remidej Feb 7, 2023 •

edited

remidej commented Feb 7, 2023 •

edited