setting option to enable/disable backup during delete record by wangf1122 · Pull Request #8728 · geonetwork/core-geonetwork

wangf1122 · 2025-04-04T00:44:35Z

The current delete record process will always force to backup the data and do delete after. The issue is if there is very big size attachment (for example over 1tb) with the metadata. The tomcat server will easily get clogged.

So this feature added such option to the setting table

It will override or do nothing to the API withbackup parameter. There are three scenarios

When the parameter is not supplied, it is currently defaulted as true. It will remain like this unless the setting is set to ForceNoBackup and this flag will override the withbackup to be false
When the parameter is supplied and set to true, it will remain as true unless the setting is set to ForceNoBackup, therefore it will cause some conflicting error
When the parameter is supplied and set to false, it will remain as false unless the setting is set to ForceBackup, therefore it will cause some conflicting error

ianwallen · 2025-04-04T11:06:13Z


      this.cancelWorkingCopy = function (md) {
-        return gnMetadataManager.remove(md.id);
+        return gnMetadataManager.remove(md.id, gnConfig["metadata.delete.enablebackup"]);


The parameter should be system wide and have nothing to do with extra parameters supplied to an api.

i.e. If backup option is disabled, it should not be possible for someone to indicate that they do want a backup by setting the backup flag to true on the api.

Maybe if someone does call the api with the backup option set to true then it should fail if backup option is disabled.

Not sure if the configuration options should be
Backup options

Enabled

Disabled (Optionally Enabled in API)

Disabled

In this case your current PR seem to be implementing "Disabled (Optionally Enabled in API)". But in some case, the option should be completely disabled and no API option exists to perform the backup.

I question a use case where someone would mostly want the backups disabled when deleting records however sometimes want the backup?

@josegar74 - what are your thought on this implementation.

I see that the backend service has a default of true (

core-geonetwork/services/src/main/java/org/fao/geonet/api/records/MetadataInsertDeleteApi.java

Line 218 in 8983b50

@Parameter(description = API_PARAM_BACKUP_FIRST, required = false) @RequestParam(required = false, defaultValue = "true") boolean withBackup,

)

Maybe it can be changed like this:

If the parameter is provided, use what is provided.

If the parameter is not provided, use the new setting.

There are 3 options

Enabled

Disabled (Optionally Enabled in API)

Disabled

The suggestion that @josegar74 supplied is cleaner than modifying the ui however it still uses the option " Disabled (Optionally Enabled in API)"

In some cases, we may want to completely disable the backup. And if someone supplied withBackup=true then we want it to fail indicating that that feature has been disabled. (This would be the "Disabled" option)

Maybe the configuration options should be the following?

Enabled (Default)

Disabled (Optionally enabled in API using withBackup=true)

Disabled Feature. (Will fail if API attempts to supply withBackup=true)

@ianwallen @josegar74

I scan the project and I dont see anywhere it intentionally to pass this parameter. Therefore it was defaulted to true.

I have simplify the logic. Simply allow nullable to this withBackup param. So it will default to the setting value. Otherwise, we will do mismatch checking and throw an error accordingly. It will fit into the scenario that you are describing here. Please take a look of the code.

…alue.

…up.option

ianwallen · 2025-04-23T10:46:52Z

 INSERT INTO Settings (name, value, datatype, position, internal) VALUES ('metadata/history/accesslevel', 'Editor', 0, 12021, 'n');

 INSERT INTO Settings (name, value, datatype, position, internal) VALUES ('metadata/delete/profilePublishedMetadata', 'Editor', 0, 12011, 'n');
+INSERT INTO Settings (name, value, datatype, position, internal) VALUES ('metadata/delete/backupOptions', 'NoPreference', 0, 12012, 'n');


NoPreference is confusing? What does NoPreference? does it mean that a backup will be performed or not be performed?

Add what @josegar74 suggested for the name change

josegar74 · 2025-04-25T13:39:57Z

@wangf1122 would not be better like this?

Force backup: always makes a backup. Ignores the API parameter.
Force no backup: never makes a backup. Ignores the API parameter.
Use API parameter (NoPreference, but with a slightly better name, I think):
- If provided, uses the value provided.
- If not provided, it makes a backup (current default behaviour).

wangf1122 · 2025-04-29T02:11:57Z

Use API parameter

@josegar74

I have change the name for the option called "Use API parameter" and made the help text description change accordingly

josegar74 · 2025-05-12T13:16:06Z

+                          {{'Force backup' | translate}}
+                        </option>
+                        <option
+                          value="ForceNoBackup"
+                          ng-selected="'ForceNoBackup' == s.value"
+                        >
+                          {{'Force no backup' | translate}}
+                        </option>
+                        <option value="UseAPIParameter" ng-selected="'UseAPIParameter' == s.value">
+                          {{'UseAPIParameter' | translate}}


Add translation keys for these values.

I added the translation in en-admin.json. Please check

josegar74

Tested and works fine, once updated the translations looks fine to be merged.

geonetworkbuild · 2025-05-12T16:54:50Z

The backport to 4.2.x failed:

The process '/usr/bin/git' failed with exit code 1

stderr

error: could not apply 4ed8e7d4df... setting option to enable/disable backup during delete record
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
hint: Disable this message with "git config set advice.mergeConflict false"

stdout

Auto-merging web-ui/src/main/resources/catalog/components/catalog/CatalogService.js
Auto-merging web-ui/src/main/resources/catalog/locales/en-admin.json
CONFLICT (modify/delete): web/src/main/webapp/WEB-INF/classes/setup/sql/migrate/v447/migrate-default.sql deleted in HEAD and modified in 4ed8e7d4df (setting option to enable/disable backup during delete record).  Version 4ed8e7d4df (setting option to enable/disable backup during delete record) of web/src/main/webapp/WEB-INF/classes/setup/sql/migrate/v447/migrate-default.sql left in tree.

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-4.2.x 4.2.x
# Navigate to the new working tree
cd .worktrees/backport-4.2.x
# Create a new branch
git switch --create backport-8728-to-4.2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick 4ed8e7d4dfcf04d487dc47e9b84f6e8d420d6c59,17b4ed4402239105b8797daa7f277aeb59517e83,71bb392c41fbee52b30fb2ce6ac399aeb8a89efc,1077817a8ce60f13f6e3887fa78f31435e9d23ad,d1d7ab1a95d9cd4c493f14da3daec00a5448a417,2419f219552eeb2e41f5e905b46028fa0b569b94
# Push it to GitHub
git push --set-upstream origin backport-8728-to-4.2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-4.2.x

Then, create a pull request where the base branch is 4.2.x and the compare/head branch is backport-8728-to-4.2.x.

…e record (geonetwork#8728) * setting option to enable/disable backup during delete record * Change default of withBackup to allow null and default with setting value. * System setting of delete record backup options * Change no preference to Use API parameter option * update translation

…e record (#8728) (#8784) * setting option to enable/disable backup during delete record * Change default of withBackup to allow null and default with setting value. * System setting of delete record backup options * Change no preference to Use API parameter option * update translation Co-authored-by: wangf1122 <74916635+wangf1122@users.noreply.github.com>

setting option to enable/disable backup during delete record

4ed8e7d

ianwallen reviewed Apr 4, 2025

View reviewed changes

ianwallen added the backport 4.2.x label Apr 4, 2025

ianwallen added this to the 4.4.7 milestone Apr 4, 2025

ianwallen requested a review from josegar74 April 4, 2025 11:39

Change default of withBackup to allow null and default with setting v…

17b4ed4

…alue.

ianwallen reviewed Apr 8, 2025

View reviewed changes

Comment thread services/src/main/java/org/fao/geonet/api/records/MetadataInsertDeleteApi.java Outdated

ianwallen reviewed Apr 8, 2025

View reviewed changes

Comment thread web/src/main/webapp/WEB-INF/classes/setup/sql/migrate/v447/migrate-default.sql Outdated

josegar74 modified the milestones: 4.4.7, 4.4.8 Apr 10, 2025

wangf1122 and others added 2 commits April 11, 2025 15:59

Merge branch 'geonetwork:main' into main.delete.metadata.without.back…

71bb392

…up.option

System setting of delete record backup options

1077817

wangf1122 force-pushed the main.delete.metadata.without.backup.option branch from c081ffb to 1077817 Compare April 11, 2025 21:33

ianwallen reviewed Apr 23, 2025

View reviewed changes

Change no preference to Use API parameter option

d1d7ab1

josegar74 reviewed May 12, 2025

View reviewed changes

update translation

2419f21

wangf1122 requested a review from josegar74 May 12, 2025 13:27

josegar74 approved these changes May 12, 2025

View reviewed changes

ianwallen merged commit 2cf79d8 into geonetwork:main May 12, 2025
7 checks passed

geonetworkbuild added the failed backport label May 12, 2025

ianwallen mentioned this pull request May 12, 2025

[Backport 4.2.x] setting option to enable/disable backup during delete record #8784

Merged

josegar74 added the changelog label May 24, 2025

Uh oh!

Conversation

wangf1122 commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josegar74 commented Apr 25, 2025

Uh oh!

wangf1122 commented Apr 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josegar74 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

geonetworkbuild commented May 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wangf1122 commented Apr 4, 2025 •

edited

Loading