[toKrause]

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]

• Adds annotation MCRClassProperty
• Adds support for MCRClassProperty in MCRSource
• Extracts generic base class MCRValueSourceBase from MCRPropertySource
  • MCRValueSourceBase uses MCRValueExtractor to extract generic values
  • MCRPropertySource uses MCRPropertyExtractor to extract simple string values
• Adds MCRClassPropertySource to process MCRClassProperty annotations
  • MCRClassPropertySource uses MCRClassPropertyExtractor to extract class values
• Adds test class MCRInstantiatorClassPropertyTest

Same as above for MCRClassPropertyList and MCRClassPropertyMap

• Adds test class MCRInstantiatorClassPropertyBasicTest to demonstrate basic usages
• Updates MCRSimpleJobSelector and MCRRestV2App to use new annotations

[sebhofmann]

Local code review

Reviewed the full diff of this branch against main. The extraction of MCRValueExtractor and the shared MCRValue*SourceBase classes is a good simplification, and moving class loading out of the individual factories is clearly the right direction. Twelve findings below, one of which I consider a blocker.

Blocker

MCRRestV2App.Binding.Factory silently lost its type check. See the inline comment. The apiClass.isAssignableFrom(implementationClass) validation was deleted and not replaced, and because @MCRClassProperty.valueClass() defaults to Object.class, nothing validates the configured classes any more. A misconfiguration that used to fail at startup with the property name in the message now fails on the first request with an HK2 MultiException.

The root cause is shared with the second point below, so fixing valueClass() first is probably the better order.

Summary of the inline comments

#	Location	Topic
1	MCRRestV2App:154	Lost From/To compatibility check (blocker)
2	MCRClassPropertySource:40	type() returns PROPERTY instead of CLASS_PROPERTY (also list and map)
3	MCRClassPropertyExtractor:39	LinkageError from class initialization escapes unwrapped
4	MCRClassProperty:48	valueClass() defaults to Object.class, inconsistent with MCRInstance*
5	MCRSimpleJobSelector:156	required = true versus blank shipped default (pre existing)
6	MCRValueListSourceBase:64	Hardcoded element description in the shared base
7	MCRValueListSourceBase:78	Short form elements extracted with the parent context
8	MCRValueMapSourceBase:67	Short form entries get a context for a non existent property
9	MCRValueSourceBase:30	Sentinel field duplicated six times
10	MCRSource:77	Nine new classes per value type (design question)
11	MCRInstantiatorClassPropertyBasicTest:44	Untested failure branches for list and map

Two findings outside the diff

MCRInstanceConfiguration.ofClassName (line 91) was not included in the MCRInstantiatorUtils.getClass refactoring. resolveValueClass in the same file was converted, ofClassName was not, and MCRClassPropertyExtractor.toValue adds a third independent copy of the load and wrap logic. The three paths throw three different things:

• resolveValueClass: MCRConfigurationException("Missing class (X) configured in: Y")
• ofClassName: MCRException("Failed to load class X"), with no property name at all
• MCRClassPropertyExtractor: MCRConfigurationException("... has a class (X) that could not be loaded")

Callers that catch MCRConfigurationException to report configuration problems miss the ofClassName path entirely, and the LinkageError gap from finding 3 would have to be fixed in three places.

MCRInstantiator javadoc (lines 150 to 158) still lists the old annotation set. The list claims "Source implementations exist for all supported annotations" but names only MCRProperty, MCRRawProperties, MCRInstance, MCRInstanceMap, MCRInstanceList and MCRPostConstruction. MCRPropertyList and MCRPropertyMap were already missing, and this PR adds three more, so the one place documenting the complete annotation set now omits five of eleven.

_{Review produced locally with Claude Code and verified against the working tree.}

[sebhofmann]

Short form elements lose their position. The long form branch above builds a nested context per element, this one passes the parent context.

For Foo.Values = a.b.Good, a.b.Missing, a.b.AlsoGood the extractor throws "Class list, configured in Foo.Values (and its sub-properties) ... has a class (a.b.Missing) that could not be loaded". The class name only makes it into the message by luck; the index does not, so for a long list the operator has to guess. This did not matter while MCRPropertyListSource could not fail during extraction, but it can now.

[toKrause]

I don't understand what the problem is supposed to be here. If the short form property Foo.Values contains a problematic value, the error message correctly points to that property and names the problematic value.

[sebhofmann]

Expanding on this, since the original wording was too terse.

The two notations

A list property can be written in two ways:

# long form
Foo.Values.10 = a.b.Good
Foo.Values.20 = a.b.Missing
Foo.Values.30 = a.b.AlsoGood

# short form
Foo.Values = a.b.Good, a.b.Missing, a.b.AlsoGood

getResult handles them in two separate loops, and the loops treat the context differently.

Long form (line 60 after 9fc6332):

MCRSourceContext nestedContext = context.nested(key, context.description() + " element");
...
tailPropertyList.add(extractor.toValue(nestedContext, listProperties.get(key)));

Short form (line 74):

shortFormList.add(extractor.toValue(context, shortFormValue));

The first gives every element its own context with property = Foo.Values.20. The second passes the parent context, whose property is just Foo.Values. Since the context is what builds the message in MCRSourceContext.exceptionMessage, the two branches produce different diagnostics for the same defect.

The resulting messages

Long form:

Class list element, configured in Foo.Values.20 (and its sub-properties), for target field 'values' in configured class org.example.Foo has a class (a.b.Missing) that could not be loaded

Short form:

Class list, configured in Foo.Values (and its sub-properties), for target field 'values' in configured class org.example.Foo has a class (a.b.Missing) that could not be loaded

The first points at the offending line, the second at the whole list. With three entries that is fine, with fifteen it is a search.

What "by luck" was meant to say

In the short form case the context contributes no positional information at all. The operator still sees a.b.Missing only because MCRClassPropertyExtractor happens to inline the value into its own message text:

throw context.configurationException("has a class (" + value + ") that could not be loaded", exception);

That is a property of this one extractor, not a contract of MCRValueExtractor. Both of today's failure paths (could not be loaded and incompatibilityException) name the value, which is why the case is still workable right now. A future extractor for Duration, URI or enums that simply reports "is not a valid duration" would, on a short form list, produce a message from which neither the position nor the value can be recovered. Hence a latent trap rather than a bug.

Why it did not matter before

MCRPropertyExtractor.toValue is a plain return value;. Extraction could not fail for string lists, so the context handed to the short form loop had no observable effect. With MCRClassPropertyExtractor extraction can throw, which is what makes the difference between the two branches visible for the first time.

Why nested(index) is not the fix

The obvious repair would be context.nested(String.valueOf(i), ...), but no property Foo.Values.0 exists in the configuration, so the message would point the operator at a key they cannot find in their mycore.properties. That is exactly the defect described in the MCRValueMapSourceBase:67 comment, where shortFormMap is merged into mapProperties before the loop and short form entries therefore get a nested context for a key that does not exist.

The short form needs information that leaves property untouched. MCRSourceContext already carries the machinery for that: the hints field, which description() renders as " (" + join(hints) + ")". It is currently never populated and hints() is never called. A hinted("element " + i) alongside nested(...) would yield:

Class list (element 2), configured in Foo.Values (and its sub-properties), ... has a class (a.b.Missing) that could not be loaded

Correct property, position included, and the same method would cover the map case.

Severity

This is the weakest of the message related findings and not a blocker. Leaving it as is would be defensible. The part I would not leave unmarked is the inconsistency between two branches of the same method, so that the next extractor does not walk into it. The sibling finding is already addressed in 9fc6332: context.description() + " element" on line 61 is precisely the derivation I meant.

[toKrause]

Well, having a good exception message that includes the offending value didn't happen just by luck, but I agree, we should not count on the hope, that future extractors also make an effort to create good exception messages. Also, unaccounted for unchecked exceptions may always be possible (as the LinkageError-example shows), so an adjusted context description is a good idea for short form lists and maps.

I will include this in #3074 as well (where the map source is refactored in a way that makes implementing this easy).

[toKrause]

See 9104ca6

[sebhofmann]

Looked over ace2376 and ce55932. Both do the job, and the type() default method on MCRSource is a better placement than what I suggested, since it also covers MCRPostConstructionSource and MCRRawPropertiesSource. I also checked the one thing that gets structurally weaker, namely that a forgotten Type constant is no longer a compile error: sources are only ever built through MCRInstantiator:327 iterating Type.values(), so an unregistered annotation yields a source that is never constructed and fromAnnotation cannot realistically throw. The trade is clearly worth it.

superClass also reads correctly at the call site in MCRRestV2App: the absence of the attribute now says "no static bound, checked dynamically in get()", where valueClass() default Object.class used to claim "the injected value is an Object".

Three small things from the two commits, none of them blocking. Two are plain javadoc slips, the third is the error message wording that did not follow the rename.

Two optional nits on the lookup table that I did not put inline, since they are cosmetic: TYPES_BY_ANNOTATION could be made unmodifiable after the static block, and the block reads type.mapper.annotationClass directly while Mapper.toSource uses the annotationClass() accessor.

One item from my original review is still open and is not part of these two commits: MCRInstanceConfiguration.ofClassName:90 still throws MCRException("Failed to load class " + className) without the property name, and neither it nor MCRInstantiatorUtils.getClass:42 catches LinkageError. The fix from the extractor thread landed on one of the three class loading paths. It was a remark outside the diff in the first place, so a follow up ticket seems the right home for it.

[sebhofmann]

Javadoc says the wrong supertype. The class implements MCRValueExtractor<Class<?>>, so it is not a MCRPropertyExtractor.

 * A {@link MCRClassPropertyExtractor} is a {@link MCRValueExtractor} that uses
 * {@link MCRClassTools#forName(String)} to extract {@link Class} values.

Both link targets resolve, so javadoc does not fail, the statement is just not true.

[sebhofmann]

Same copy and paste, but here the subject is wrong too: the sentence describes MCRClassPropertyExtractor while sitting on MCRPropertyExtractor.

 * A {@link MCRPropertyExtractor} is a {@link MCRValueExtractor} that extracts
 * {@link String} values.

[sebhofmann]

The wording did not follow the superClass rename in ce55932. The parameter is still called annotationValueClass and the message still reads "incompatible with the annotated value class (X)", but there is no valueClass attribute on the class value annotations any more.

The method has two callers and the text now fits only one of them:

Caller	argument	wording fits
MCRSourceBase:123	configuration.valueClass()	yes
MCRClassPropertyExtractor:39	superClass	no

The error message is where an operator meets the attribute name for the first time, so this gives back part of what the rename bought. Two ways out: a separate method for the bound case, or neutral wording that names no attribute at all, for example "is not assignable to (X)".

Note that MCRInstantiatorClassPropertyBasicTest asserts the current wording in three places (lines 72, 125 and 179), so a change touches those as well.