Skip to content

MCR-3787 Harmonize handling of instance and property annotations - #3074

Merged
sebhofmann merged 4 commits into
mainfrom
issues/MCR-3787_Harmonize-handling-of-instance-and-proeprty-annotations
Sep 29, 2026
Merged

sebhofmann merged 4 commits into
mainfrom
issues/MCR-3787_Harmonize-handling-of-instance-and-proeprty-annotations

Conversation

@toKrause

@toKrause toKrause commented Jul 27, 2026 •

Copy link
Copy Markdown
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.

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 force-pushed the issues/MCR-3787_Harmonize-handling-of-instance-and-proeprty-annotations branch from ddc9476 to b0403b7 Compare July 27, 2026 20:06
@toKrause toKrause changed the title MCR-3787 harmonize handling of instance and proeprty annotations MCR-3787 Harmonize handling of instance and proeprty annotations Jul 27, 2026
@toKrause
toKrause force-pushed the issues/MCR-3787_Harmonize-handling-of-instance-and-proeprty-annotations branch from b0403b7 to fedb784 Compare July 28, 2026 19:41
@toKrause toKrause changed the title MCR-3787 Harmonize handling of instance and proeprty annotations MCR-3787 Harmonize handling of instance and property annotations Jul 28, 2026
@toKrause
toKrause force-pushed the issues/MCR-3787_Harmonize-handling-of-instance-and-proeprty-annotations branch 4 times, most recently from ffdc15f to 6f67c26 Compare August 1, 2026 12:54
@toKrause
toKrause force-pushed the issues/MCR-3787_Harmonize-handling-of-instance-and-proeprty-annotations branch from 6f67c26 to b49b6d7 Compare August 18, 2026 09:33
@toKrause
toKrause force-pushed the issues/MCR-3787_Harmonize-handling-of-instance-and-proeprty-annotations branch from b49b6d7 to 2ee5057 Compare August 31, 2026 12:58
@toKrause
toKrause force-pushed the issues/MCR-3787_Harmonize-handling-of-instance-and-proeprty-annotations branch 6 times, most recently from 614efbf to 5fd60cc Compare September 15, 2026 21:12

@sebhofmann sebhofmann left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed the branch locally. All 1274 tests in mycore-base pass and Checkstyle is clean.

One finding blocks: the short form insertion point in MCRValueListSourceBase can throw IndexOutOfBoundsException. See the inline comment for the reproduction.

One point has no line to attach to. MCRInstanceConfiguration.nested(Class, String), both nestedMap(...) overloads, MCRSourceContext.incompatibilityException and MCRSourceContext.orderedKeys(Map) are public API shipped in v2026.06.0 and are removed here, while neither commit message marks a breaking change. Nothing outside mycore-base uses them in my workspace, so this is about the marker, not about the removal itself. Along with them MCRInstanceConfigurationTest (352 lines) is dropped, which leaves the nestedMap edge cases (empty prefix, mixed nesting depth) uncovered.

The remaining inline comments are nits.

return reducedProperties;
}

protected static Set<String> nextNestedKeys(Map<String, String> properties) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This turns intermediate segments of deeper keys into list and map keys. For lists that reaches orderedIntegerKeys, so

Foo.Values.1        = A
Foo.Values.Oops.Sub = X

now throws has element with non-integer key Oops, while on main the indexOf('.', keyPrefixLength) == -1 filter dropped such keys silently. For instance lists the stricter behaviour looks right, for @MCRPropertyList it is a behaviour change that can surface on update in an application with a grown property file. If it is intended, it seems worth naming in the ticket and the documentation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've added this as an example benefit of the harmonization in the ticket description.


}

protected static Map<String, String> reduceProperties(Map<String, String> properties, String prefix) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is line for line identical to the private MCRInstanceConfiguration.reduceProperties (line 194 there). One of the two should call the other.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Both occurrences are going to be removed in MCR-3703. I've added a copy temporarily, so I do not have make one method public and remove a public method in the next PR.

Map<String, String> fullProperties) {

MCRInstanceConfiguration<?> configuration = MCRInstanceConfiguration.ofComponents(
superClass, MCRInstanceName.of(context.property()), properties, fullProperties);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The name is recovered from the property string here, while the previous code built it structurally via name.nested(prefix). MCRInstanceName.of strips a trailing .Class, so an instance map entry keyed Class ends up with a name short by one segment in messages and in the [IMPLICIT] log. Pathological, but avoidable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What?

@toKrause
toKrause force-pushed the issues/MCR-3787_Harmonize-handling-of-instance-and-proeprty-annotations branch from 5fd60cc to 156c326 Compare September 21, 2026 12:26
@toKrause
toKrause force-pushed the issues/MCR-3787_Harmonize-handling-of-instance-and-proeprty-annotations branch from 156c326 to e32c67e Compare September 28, 2026 17:01
@sebhofmann
sebhofmann merged commit 75f2e71 into main Sep 29, 2026
3 checks passed
@sebhofmann
sebhofmann deleted the issues/MCR-3787_Harmonize-handling-of-instance-and-proeprty-annotations branch September 29, 2026 07:58
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.

2 participants