Fix issue with saving metadata status causes indexing of metadata which causes issues for db rollbacks

[wangf1122]

The issue happens in this API where the index was update prematurely.

It's supposed to be done here

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

Line 508 in e75ee33

metadataIndexer.indexMetadata(String.valueOf(metadata.getId()), true, IndexingMode.full);

But was updated within this code above the thread

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

Line 502 in e75ee33

Map<Integer, StatusChangeType> statusUpdate = sa.onStatusChange(listOfStatusChange);

We should add such option to prevent the index updated.

The issue is if something happens in the publish event into the customized event listener. There are exceptions can be thrown due to the business logic check

core-geonetwork/core/src/main/java/org/fao/geonet/kernel/metadata/DefaultStatusActions.java

Lines 203 to 206 in e75ee33

	context.getApplicationContext().publishEvent(new MetadataStatusChanged(
	metadataUtils.findOne(mid),
	status.getStatusValue(), status.getChangeMessage(),
	status.getUserId()

The database will roll back properly. But the search engine will have the updated status. If we refresh the web page, it will display the wrong status.

For example,

Retired failed due to some business logic check in event listener.

Here is the search engine's result
http://localhost:8080/catalogue/srv/eng/q?_content_type=json&_draft=y+or+n+or+e&_isTemplate=y+or+n&fast=index&uuid=476399f3-ec21-4792-9a7b-8b36800946dc

[ianwallen]

So the issue is that the application is calling the function to set the status.
But then an exception is later thrown and the changes are rolled back but the index was updated during the setting of the status.

There are a few ways to fix this.
The way you have done it seems to work however I'm wondering why setting the status is updating the index?

The following comments indicates that the set status is not supposed to update the index? Not sure if this comment should be updated?

core-geonetwork/core/src/main/java/org/fao/geonet/kernel/datamanager/base/BaseMetadataStatus.java

Line 149 in e75ee33

* Set status of metadata id and reindex metadata id afterwards.

One option could be to change the call to so that instead of
metadataStatusManager.setStatusExt(status);

It would instead call the following instead?
metadataStatusRepository.save(metatatStatus);

Maybe others have some input on this issue?

[josegar74]

@ianwallen

The following comments indicates that the set status is not supposed to update the index? Not sure if this comment should be updated?

I think that comment means that the database is updated and the metadata reindexed, as it's implemented.

---

About your change looks ok (need to test it), but I would be good to document in which cases should be pass a false value to the method.

---

Something to explore would be to check if indexing can be done in the transaction manager after commit, but I'm afraid that will require some major refactoring to store the list of metadata to reindex and verify that it doesn't affect other functionality.

[ianwallen]

@josegar74

I think that comment means that the database is updated and the metadata reindexed, as it's implemented.

There is no javadocs for the function being modified
If you look at the 2 functions that do have this comment (Both seem to be override version of the same api), they don't modify the index at all.

So I think the comment means that it is expected that you will need to later re-index after the function is called.

It is odd to have an override version which does do indexing? It seems inconsistent to me.

If all the process which depend on setStatusExt function would handle re-indexing then this issue may not have occurred?

[josegar74]

@ianwallen in the interface, both definitions for setStatusExt, indicate about not reindexing:

core-geonetwork/core/src/main/java/org/fao/geonet/kernel/datamanager/IMetadataStatus.java

Lines 63 to 76 in e75ee33

	/**
	* Set status of metadata id and do not reindex metadata id afterwards.
	*
	* @return the saved status entity object
	*/
	@Deprecated
	MetadataStatus setStatusExt(ServiceContext context, int id, int status, ISODate changeDate, String changeMessage) throws Exception;
	/**
	* Set status of metadata id and do not reindex metadata id afterwards.
	*
	* @return the saved status entity object
	*/
	MetadataStatus setStatusExt(MetadataStatus status) throws Exception;

This implementation looks wrong according to the previous documentation:

core-geonetwork/core/src/main/java/org/fao/geonet/kernel/datamanager/base/BaseMetadataStatus.java

Lines 163 to 167 in e75ee33

	public MetadataStatus setStatusExt(MetadataStatus metatatStatus) throws Exception {
	metadataStatusRepository.save(metatatStatus);
	metadataIndexer.indexMetadata(metatatStatus.getMetadataId() + "", true, IndexingMode.full);
	return metatatStatus;
	}

If you compare with this other implementation that doesn't reindex as indicated in the interface:

core-geonetwork/core/src/main/java/org/fao/geonet/kernel/datamanager/base/BaseMetadataStatus.java

Lines 175 to 195 in e75ee33

	public MetadataStatus setStatusExt(ServiceContext context, int id, int status, ISODate changeDate,
	String changeMessage) throws Exception {
	Optional<StatusValue> statusValue = statusValueRepository.findById(status);
	if (!statusValue.isPresent()) {
	throw new IllegalArgumentException("The workflow status change requested is not valid: " + status);
	}
	MetadataStatus metatatStatus = new MetadataStatus();
	metatatStatus.setChangeMessage(changeMessage);
	metatatStatus.setStatusValue(statusValue.get());
	int userId = context.getUserSession().getUserIdAsInt();
	metatatStatus.setMetadataId(id);
	metatatStatus.setChangeDate(changeDate);
	metatatStatus.setUserId(userId);
	metatatStatus.setUuid(metadataUtils.getMetadataUuid(Integer.toString(id)));
	metatatStatus.setTitles(metadataUtils.extractTitles(Integer.toString(id)));
	return metadataStatusRepository.save(metatatStatus);
	}

I'm fine with the change, but please update the interface documentation also.

[josegar74]

applyStatusChange is used in StatusActions.onStatusChage that is call when creating a draft version:

core-geonetwork/core/src/main/java/org/fao/geonet/kernel/datamanager/draft/DraftMetadataUtils.java

Line 592 in 1f16287

sa.onStatusChange(listOfStatusChange);

Previously was indexed the metadata and with this change no. Have you check that creating a draft contains the proper status in the index? Otherwise DraftMetadataUtils.createDraft needs to explicit index the draft metadata.

[wangf1122]

I tested the code. This create draft only happens when creating working copy. Its working fine without indexing.

[wangf1122]

@josegar74

Sorry I misunderstood your comment previously. You are right, it used to explicit the indexing. So it's better to introduce at its parent level onStatusChange(boolean updateIndex). I made a new commit and please check it out. The only place does not need indexing is

[wangf1122]

setStatusExt

I will update the java doc for sure. As for the logic, sure I can do that "not updating the index". But you see the issue of the original code is it does opposite. It forces to update the index instead of "not update". So if you are sure, the flag i place here as false is good, then i will go with the change.

Edited: Sorry, I missed some of your comment. Looks like the original implementation is wrong as you said. I will update the flag as well

[josegar74]

setStatusExt

I will update the java doc for sure. As for the logic, sure I can do that "not updating the index". But you see the issue of the original code is it does opposite. It forces to update the index instead of "not update".

@wangf1122 yes, the original code of forces to update:

core-geonetwork/core/src/main/java/org/fao/geonet/kernel/datamanager/base/BaseMetadataStatus.java

Lines 163 to 167 in e75ee33

	public MetadataStatus setStatusExt(MetadataStatus metatatStatus) throws Exception {
	metadataStatusRepository.save(metatatStatus);
	metadataIndexer.indexMetadata(metatatStatus.getMetadataId() + "", true, IndexingMode.full);
	return metatatStatus;
	}

Contrary to what the doc indicates and the other setStatusExt implements (correctly)

core-geonetwork/core/src/main/java/org/fao/geonet/kernel/datamanager/base/BaseMetadataStatus.java

Lines 175 to 195 in e75ee33

	public MetadataStatus setStatusExt(ServiceContext context, int id, int status, ISODate changeDate,
	String changeMessage) throws Exception {
	Optional<StatusValue> statusValue = statusValueRepository.findById(status);
	if (!statusValue.isPresent()) {
	throw new IllegalArgumentException("The workflow status change requested is not valid: " + status);
	}
	MetadataStatus metatatStatus = new MetadataStatus();
	metatatStatus.setChangeMessage(changeMessage);
	metatatStatus.setStatusValue(statusValue.get());
	int userId = context.getUserSession().getUserIdAsInt();
	metatatStatus.setMetadataId(id);
	metatatStatus.setChangeDate(changeDate);
	metatatStatus.setUserId(userId);
	metatatStatus.setUuid(metadataUtils.getMetadataUuid(Integer.toString(id)));
	metatatStatus.setTitles(metadataUtils.extractTitles(Integer.toString(id)));
	return metadataStatusRepository.save(metatatStatus);
	}

So if you are sure, the flag i place here as false is good, then i will go with the change

With the change you've done, I think #7514 (comment) not index the metadata, while with the previous implementation was indexed, no?

[josegar74]

I've tested the changes with the workflow enabled, but submitting a metadata for revision doesn't change the status from draft to submitted, the popup displays that the status has been changed successfully, but it doesn't. Please review it.

[wangf1122]

I've tested the changes with the workflow enabled, but submitting a metadata for revision doesn't change the status from draft to submitted, the popup displays that the status has been changed successfully, but it doesn't. Please review it.

@josegar74
That was a small bug which I introduced in the previous commit. I fixed it. Please check again.

[josegar74]

Tested, looks fine.

[geonetworkbuild]

The backport to 3.12.x failed:

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

stderr

error: could not apply 01a7f2f623... option to reindexing
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".

stdout

Auto-merging core/src/main/java/org/fao/geonet/kernel/datamanager/IMetadataStatus.java
Auto-merging core/src/main/java/org/fao/geonet/kernel/datamanager/base/BaseMetadataStatus.java
CONFLICT (content): Merge conflict in core/src/main/java/org/fao/geonet/kernel/datamanager/base/BaseMetadataStatus.java
Auto-merging core/src/main/java/org/fao/geonet/kernel/metadata/DefaultStatusActions.java
CONFLICT (content): Merge conflict in core/src/main/java/org/fao/geonet/kernel/metadata/DefaultStatusActions.java
Auto-merging listeners/src/main/java/org/fao/geonet/listener/metadata/draft/ApprovePublishedRecord.java
Auto-merging listeners/src/main/java/org/fao/geonet/listener/metadata/draft/ApproveRecord.java

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-3.12.x 3.12.x
# Navigate to the new working tree
cd .worktrees/backport-3.12.x
# Create a new branch
git switch --create backport-7514-to-3.12.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick 01a7f2f623c71b8ae6b6c3112239e460214fb75d,8a49652c30471a7cde6de41565eb66c8c4174daf,71368be4855748e2cc2bfb3264c0a9fdfd92e218,d4a4255f1045c7f0b872a4408d606a1342b06fd2
# Push it to GitHub
git push --set-upstream origin backport-7514-to-3.12.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-3.12.x

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