Skip to content

MCR-3650 Refactor MCRConfigurableInstanceHelper into multiple classes#2870

Merged
sebhofmann merged 1 commit intomainfrom
issues/MCR-3650_Refactor-MCRConfigurableInstanceHelper-into-multiple-classes
May 5, 2026
Merged

MCR-3650 Refactor MCRConfigurableInstanceHelper into multiple classes#2870
sebhofmann merged 1 commit intomainfrom
issues/MCR-3650_Refactor-MCRConfigurableInstanceHelper-into-multiple-classes

Conversation

@toKrause
Copy link
Copy Markdown
Contributor

@toKrause toKrause commented Mar 19, 2026

Link to jira.

This PR only refactors, further simplification & clean-up is done in #2922.

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](https://www.mycore.de/documentation/developer/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-3650_Refactor-MCRConfigurableInstanceHelper-into-multiple-classes branch from 1168254 to 820cf37 Compare March 19, 2026 15:42
@toKrause toKrause force-pushed the issues/MCR-3650_Refactor-MCRConfigurableInstanceHelper-into-multiple-classes branch 7 times, most recently from 0a00587 to d61892f Compare March 23, 2026 21:26
@toKrause toKrause marked this pull request as draft March 24, 2026 14:09
@toKrause toKrause force-pushed the issues/MCR-3650_Refactor-MCRConfigurableInstanceHelper-into-multiple-classes branch 6 times, most recently from d4ba823 to 79f7c69 Compare March 31, 2026 08:54
@toKrause toKrause force-pushed the issues/MCR-3650_Refactor-MCRConfigurableInstanceHelper-into-multiple-classes branch 3 times, most recently from cb570b2 to 26cced8 Compare April 1, 2026 15:49
@toKrause toKrause changed the title MCR-3650 Refactor MCRConfigurableInstanceHelper into multiple classes MCR-3650 Refactor MCRConfigurableInstanceHelper Apr 1, 2026
@toKrause toKrause force-pushed the issues/MCR-3650_Refactor-MCRConfigurableInstanceHelper-into-multiple-classes branch 7 times, most recently from 76cb02f to 48a4916 Compare April 7, 2026 14:20
@toKrause toKrause marked this pull request as ready for review April 7, 2026 14:21
@toKrause toKrause force-pushed the issues/MCR-3650_Refactor-MCRConfigurableInstanceHelper-into-multiple-classes branch from 48a4916 to 156469e Compare April 7, 2026 14:23
@toKrause toKrause changed the title MCR-3650 Refactor MCRConfigurableInstanceHelper MCR-3650 Refactor MCRConfigurableInstanceHelper into multiple classes Apr 7, 2026
@toKrause toKrause force-pushed the issues/MCR-3650_Refactor-MCRConfigurableInstanceHelper-into-multiple-classes branch 3 times, most recently from 771e7e0 to c39eb2b Compare April 14, 2026 11:06
@toKrause toKrause force-pushed the issues/MCR-3650_Refactor-MCRConfigurableInstanceHelper-into-multiple-classes branch 2 times, most recently from e3c7c9f to f5789ca Compare April 21, 2026 08:43
@toKrause toKrause force-pushed the issues/MCR-3650_Refactor-MCRConfigurableInstanceHelper-into-multiple-classes branch 2 times, most recently from 021cbcc to 04ac02f Compare May 1, 2026 18:51
@toKrause toKrause force-pushed the issues/MCR-3650_Refactor-MCRConfigurableInstanceHelper-into-multiple-classes branch 2 times, most recently from c23cb3f to 756c532 Compare May 4, 2026 12:49
Copy link
Copy Markdown
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.

Found by AI:

  1. Duplicated word in error message — MCRInstantiator.java:236-237: "...has " + " has no singleton..." produces "...has has no...".
  2. Unbalanced apostrophe — MCRInstantiator.java:107-108 and MCRInstantiatorUtils.java:90-91: "Instance of class " + name + "', configured in ..." has a closing ' without an opening one.
  3. Mutable public sets — Options.NONE / Options.IMPLICIT in MCRInstanceConfiguration and MCRTargetType.ALL are raw EnumSets exposed as public static final. Any caller can mutate them. Wrap with Collections.unmodifiableSet(...) or use
    Set.of(...).
  4. Wrong Javadoc references:
    - MCRFieldInjectable.java:30 says "is a MCRFieldInjectable" (self-referential).
    - MCRMethodInjectable.java:30 says "is a MCRFieldInjectable" (copy-paste error).
    - MCRRawPropertiesSource.java:32 says "interprets a MCRSourceBase" (should be MCRRawProperties).

* in principle, be used for injection. It provides abstractions for obtaining a value ({@link MCRSource})
* based on present annotations, and injecting obtained values ({@link MCRTarget}).
*/
public interface MCRInjectable {
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.

  • Consider merging MCRInjectable and MCRTarget
  • MCRInjectable should be sealed

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.

MCRInjectable represents a component that can, in principle, be used for injection (if it is annotated with one of the supported annotations). MCRTarget represents a component that is, in fact, being used for injection. I've fixed the class comments.

* Common abstraction for components of a class (i.e., a {@link Field} or a {@link Method}) that can,
* in principle, be used for injection. It provides an abstraction for obtaining a value based on present annotations.
*/
public interface MCRSource {
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.

  • MCRSource should be sealed anly only permit the classis in the source package. MCRSourceType enum is a closed list so extension is not intended.

@SuppressWarnings({ "PMD.MCR.Singleton.ClassModifiers", "PMD.MCR.Singleton.PrivateConstructor",
"PMD.MCR.Singleton.NonPrivateConstructors", "PMD.MCR.Singleton.MethodModifiers",
"PMD.MCR.Singleton.MethodReturnType", "PMD.SingletonClassReturningNewInstance" })
abstract class MCRSourceBase implements MCRSource {
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.

  • Consider renaming to MCRInstaceSourceBase, because getInstance is only used in MCRInstanceSource, MCRInstanceMapSource, MCRInstanceListSource
  • allowedTargetTypes() is dead code, because no subclass overrides it

Copy link
Copy Markdown
Contributor Author

@toKrause toKrause May 4, 2026

Choose a reason for hiding this comment

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

MCRPostConstructionSource should indeed have a differing implementation of allowedTargetTypes, since it is only allowed on methods. Small bug that we can fix in this PR.

@toKrause toKrause force-pushed the issues/MCR-3650_Refactor-MCRConfigurableInstanceHelper-into-multiple-classes branch 3 times, most recently from 3b3ad4b to affc313 Compare May 5, 2026 00:42
@toKrause toKrause force-pushed the issues/MCR-3650_Refactor-MCRConfigurableInstanceHelper-into-multiple-classes branch from affc313 to e2b6841 Compare May 5, 2026 00:53
@sebhofmann sebhofmann merged commit 0f524be into main May 5, 2026
3 checks passed
@sebhofmann sebhofmann deleted the issues/MCR-3650_Refactor-MCRConfigurableInstanceHelper-into-multiple-classes branch May 5, 2026 08:14
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