Skip to content

Subtemplates fixes #8893

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Subtemplates fixes #8893

wants to merge 4 commits into from

Conversation

sebr72
Copy link
Contributor

@sebr72 sebr72 commented Jun 25, 2025

This PR fixes the following issues:

  • Ensure the service context is not 'corrupted', to avoid loss of ui-language while processing subtemplates
  • On subtemplate's update, trigger indexation for every md referencing it
  • Perform validation on MD with substituted subtemplates (xsd and schematron)

Checklist

  • I have read the contribution guidelines

  • Pull request provided for main branch, backports managed with label

  • Good housekeeping of code, cleaning up comments, tests, and documentation

  • Clean commit history broken into understandable chucks, avoiding big commits with hundreds of files, cautious of reformatting and whitespace changes

  • Clean commit messages, longer verbose messages are encouraged

  • API Changes are identified in commit messages

  • Testing provided for features or enhancements using automatic tests

  • User documentation provided for new features or enhancements in manual

  • Build documentation provided for development instructions in README.md files

  • Library management using pom.xml dependency management. Update build documentation with intended library use and library tutorials or documentation

  • Funded by https://www.swisstopo.admin.ch/en

@sebr72 sebr72 force-pushed the subtemplates-fixes branch from 8f6a389 to d85b6ce Compare June 25, 2025 16:07
@cmangeat cmangeat force-pushed the subtemplates-fixes branch 4 times, most recently from 4577f96 to 0307b20 Compare July 2, 2025 11:44
@sebr72 sebr72 force-pushed the subtemplates-fixes branch from 0307b20 to 398c814 Compare July 2, 2025 11:53
@sebr72 sebr72 marked this pull request as ready for review July 2, 2025 12:46
@@ -55,7 +55,7 @@
lazy-init="true"/>

<bean id="ThesaurusManager" class="org.fao.geonet.kernel.ThesaurusManager" lazy-init="true">
<property name="thesaurusCacheMaxSize" value="\${thesaurus.cache.maxsize}"/>
<property name="thesaurusCacheMaxSize" value="${thesaurus.cache.maxsize}"/>
Copy link
Contributor Author

@sebr72 sebr72 Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fxprunayre Hello. We couldn't work out the meaning of the "\" you introduced. We had to remove it to make our tests pass. Could you please have a look and confirm this is OK with you ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure but all properties loaded from config.properties (cf. https://github.com/geonetwork/core-geonetwork/blob/main/web/src/main/webResources/WEB-INF/config.properties#L1) are escaped (eg. https://github.com/geonetwork/core-geonetwork/blob/main/web/src/main/webResources/WEB-INF/config-spring-geonetwork.xml#L76-L77) if we don't want them to be filtered by maven filtered resource ?
Can be that thesaurus.cache.maxsize is missing somewhere when running test?

Copy link
Contributor

@cmangeat cmangeat Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be simplier for first approach to keep the escaping \: with escaping \ test runs green from bash with mvn but fails with ide:
Error creating bean with name 'org.fao.geonet.kernel.datamanager.base.BaseMetadataManager#0': Unsatisfied dependency expressed through field 'thesaurusManager'; nested exception is org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'ThesaurusManager' defined in URL [file:/home/cmangeat/sources/core-geonetwork/core/target/classes/config-spring-geonetwork.xml]: Initialization of bean failed; nested exception is org.springframework.beans.TypeMismatchException: Failed to convert property value of type 'java.lang.String' to required type 'int' for property 'thesaurusCacheMaxSize'; nested exception is java.lang.NumberFormatException: For input string: "\400000" ...

@fxprunayre thank you for the explaination.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fxprunayre @cmangeat Indeed. Thanks for the explanations :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locally, I also get bad substitution message like NumberFormatException: For input string: "\400000" ... in Intellij from time to time but if building from command line, then it is solved (Intellij probably refresh some files). Not sure if it is an Intellij build issue or mis configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fxprunayre @cmangeat @josegar74 Based on your feedback, I reverted this change. I don't mark it as resolved to let you confirm

@@ -0,0 +1,34 @@
//=============================================================================
//=== Copyright (C) 2001-2023 Food and Agriculture Organization of the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//=== Copyright (C) 2001-2023 Food and Agriculture Organization of the
//=== Copyright (C) 2001-2025 Food and Agriculture Organization of the

Copy link
Contributor Author

@sebr72 sebr72 Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josegar74 Done. I don't mark it as resolved to let you confirm.

@@ -1,9 +1,15 @@
package org.fao.geonet.kernel;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the file header.

Copy link
Contributor Author

@sebr72 sebr72 Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josegar74 Done. I don't mark it as resolved to let you confirm.

@@ -0,0 +1,114 @@
package org.fao.geonet.api.processing;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add file header.

Copy link
Contributor Author

@sebr72 sebr72 Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josegar74 Done. I don't mark it as resolved to let you confirm.

List<Hit> hits = (List<Hit>) response.hits().hits();
hits.stream().map(Hit::id).forEach(consumerUuid -> {
try {
String consumerId = this.metadataUtils.getMetadataId(consumerUuid);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you check to retrieve the metadata id from the index? That should avoid querying the database.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josegar74 Indeed it was possible. Thanks. I don't mark it as resolved to let you confirm.

Comment on lines 75 to 77
if (srvContext != null) {
srvContext.setAsThreadLocal();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain this code? It can be good to add some comment to clarify it, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josegar74 Done. I don't mark it as resolved to let you confirm.

String query = String.format("xlink:*%s*", subTemplate.getUuid());
SearchResponse response = this.searchManager.query(query, null, 0, maxMdsReferencingSubTemplate);
if (response.hits().total().value() > maxMdsReferencingSubTemplate) {
throw new GNException("Not implemented");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be relevant to log the details and the exception message can be more clear than Not implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josegar74 Done. I don't mark it as resolved to let you confirm.

//==============================================================================
package org.fao.geonet.exceptions;

public class GNException extends RuntimeException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exception seems only used when the number of metadata referencing the subtemplates exceeds a number. Maybe can be given a clear name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josegar74 Done. I don't mark it as resolved to let you confirm.

@sebr72 sebr72 force-pushed the subtemplates-fixes branch 4 times, most recently from f230412 to 438a82b Compare July 7, 2025 11:59
sebr72 added 4 commits July 7, 2025 14:03
rely on metadataIndexer.batchIndexInThreadPool for async indexing
test threadPoolIndexation starts for expected md
throw exception when updating a subtemplate too resource hungry (too many md to index, i.e. more than 10000)
@sebr72 sebr72 force-pushed the subtemplates-fixes branch from 438a82b to 2b3c10a Compare July 7, 2025 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants