Skip to content

MCR-3639 Add support for annotation based configuration of string-valued maps and lists#2855

Merged
yagee-de merged 1 commit intomainfrom
issues/MCR-3639_Add-support-for-annotation-based-configuration-of-string-valued-maps-and-lists
Mar 24, 2026
Merged

MCR-3639 Add support for annotation based configuration of string-valued maps and lists#2855
yagee-de merged 1 commit intomainfrom
issues/MCR-3639_Add-support-for-annotation-based-configuration-of-string-valued-maps-and-lists

Conversation

@toKrause
Copy link
Contributor

Link to jira.

Pull Request Checklist (Author)

Please go through the following checklist before assigning the PR for review:

Ticket & Documentation

  • The issue in the ticket is clearly described and the solution is documented.
  • Design decisions (if any) are explained.
  • The ticket references the correct source and target branches.
  • The fixed-version is correctly set in the ticket and matches the PR's target branch (main).

Feature & Improvement Specific Checks

  • Instructions on how to test or use the feature are included or linked (e.g. to documentation).
  • For UI changes: before & after screenshots are attached.
  • New features or migrations are documented.
  • Does this change affect existing applications, data, or configurations?
    • Yes: Is a migration required? If yes, describe it.
    • Breaking change is marked in the commit message.

Testing

  • I have tested the changes locally.
  • The feature behaves as described in the ticket.
  • Were existing tests modified?
    • Yes: explain the changes for reviewers.

As per Scout rule: Test property names and values used in MCRConfigurableInstanceHelperNestedTest have been changed to align with the new test class MCRConfigurableInstanceHelperCollectionTest which has similar tests (the names and values have become a bit unwieldy). List#getFirst() has been replaced with List#get(0) in the tests, b/c it better reflects the indexed-based access to the tested lists.

MCR Conventions & Metadata

  • MCR naming conventions are followed
  • If the public API has changed:
    • Old API is deprecated or a migration is documented.
    • If not, no action needed.
  • Java license headers are added where necessary.
  • Javadoc is written for non-self-explanatory classes/methods (Clean Code).
  • All configuration options are documented in Javadoc and mycore.properties.
  • No default properties are hardcoded — all set via mycore.properties.

Multi-Repo Considerations

  • Is an equivalent PR in MIR required?
    • If yes, is it already created?

@toKrause toKrause requested a review from rsteph-de March 11, 2026 16:11
@toKrause toKrause force-pushed the issues/MCR-3639_Add-support-for-annotation-based-configuration-of-string-valued-maps-and-lists branch from 896bfd3 to 3e64ccb Compare March 11, 2026 16:39
@toKrause toKrause marked this pull request as draft March 12, 2026 16:58
@toKrause toKrause marked this pull request as ready for review March 13, 2026 12:29
@toKrause toKrause force-pushed the issues/MCR-3639_Add-support-for-annotation-based-configuration-of-string-valued-maps-and-lists branch 5 times, most recently from 1582327 to d150d3f Compare March 13, 2026 13:54
@toKrause toKrause requested review from sebhofmann and removed request for rsteph-de March 17, 2026 14:43
@toKrause toKrause force-pushed the issues/MCR-3639_Add-support-for-annotation-based-configuration-of-string-valued-maps-and-lists branch 2 times, most recently from 93cb5de to 0182e94 Compare March 19, 2026 09:27
@toKrause toKrause force-pushed the issues/MCR-3639_Add-support-for-annotation-based-configuration-of-string-valued-maps-and-lists branch 2 times, most recently from 7567935 to f40f5f5 Compare March 23, 2026 10:18
Copy link
Member

@sebhofmann sebhofmann left a comment

Choose a reason for hiding this comment

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

Some fixes required.

}

SortedMap<Integer, String> orderedPropertyMap = new TreeMap<>();
propertyMap.forEach((key, value) -> orderedPropertyMap.put(Integer.parseInt(key), value));
Copy link
Member

Choose a reason for hiding this comment

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

This will throw a "cryptic" message if they keys are no integer. Maybe throw a better error like:

  propertyMap.forEach((key, value) -> {
      try {
          orderedPropertyMap.put(Integer.parseInt(key), value);
      } catch (NumberFormatException e) {
          throw new MCRConfigurationException(
              "Property list key '" + prefix + key + "' is not a valid integer index", e);
      }
  });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is being done as part of #2861


private Map<String, String> parseShortFormMap(String value) {
return MCRConfiguration2.splitValue(value)
.map(s -> s.split(":", 2))
Copy link
Member

Choose a reason for hiding this comment

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

This should maybe print a warning if there are some wrong mappings without :

  private Map<String, String> parseShortFormMap(String value) {
      return MCRConfiguration2.splitValue(value)
          .map(s -> s.split(":", 2))
          .filter(parts -> {
              if (parts.length == 1) {
                  LOGGER.warn("Ignoring short form map entry '{}' - missing ':' separator", parts[0]);
                  return false;
              }
              return true;
          })
          .filter(parts -> !parts[1].isBlank())
          .collect(Collectors.toMap(parts -> parts[0].trim(), parts -> parts[1].trim()));
  }

@toKrause toKrause force-pushed the issues/MCR-3639_Add-support-for-annotation-based-configuration-of-string-valued-maps-and-lists branch 3 times, most recently from febdd04 to fcd035e Compare March 23, 2026 20:51
@toKrause toKrause force-pushed the issues/MCR-3639_Add-support-for-annotation-based-configuration-of-string-valued-maps-and-lists branch from fcd035e to 74745ca Compare March 23, 2026 21:14
@toKrause toKrause requested a review from sebhofmann March 24, 2026 07:50
@toKrause
Copy link
Contributor Author

Some fixes required.

Thx for the feedback. Issues should be fixed now.

@yagee-de yagee-de merged commit 79fb4d2 into main Mar 24, 2026
3 checks passed
@yagee-de yagee-de deleted the issues/MCR-3639_Add-support-for-annotation-based-configuration-of-string-valued-maps-and-lists branch March 24, 2026 13:40
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.

3 participants