refactor: MatcherOperatorRegistry and default impl #1237

rlaxowns7916 · 2025-09-05T15:33:32Z

Summary

Abstract List<MatcherOperator> behind a small wrapper to make the matching layer extensible.
Introduces MatcherOperatorRegistry as the abstraction that manages registration and lookup of MatcherOperators.

(Related issue: N/A)

⸻

(Optional): Description
This PR refactors the internal storage and lookup of matchers:

New interface: MatcherOperatorRegistry
Default Implementation: DefaultMatcherOperatorRegistry
- ExactTypeMatcher operators indexed by exact Class<?> in a Map.
- AssignableTypeMatcher operators also stored in a Map, keyed by their anchor type (Class<?> used on the left side of isAssignableFrom).
  ⸻

How Has This Been Tested?

No new tests were required because this is an internal refactor.

⸻

Is the Document updated?

x

rlaxowns7916 · 2025-09-06T03:03:04Z

...-monkey-api/src/main/java/com/navercorp/fixturemonkey/api/matcher/AssignableTypeMatcher.java

+
+	public Class<?> getAnchorType() {
+		return this.type;
+	}


Exposed a public getter for anchorType in AssignableTypeMatcher to make it accessible from DefaultImpl.

rlaxowns7916 · 2025-09-06T03:04:18Z

...onkey-api/src/main/java/com/navercorp/fixturemonkey/api/matcher/PriorityMatcherOperator.java

-	private void checkPriority(int priority) {
-		if (priority < 0) {
-			throw new IllegalArgumentException("Priority must be greater than or equal to 0");
-		}
-	}


In DefaultImpl, I used PriorityMatcherOperator to preserve the order. If negative conditions should not be removed, should I create an additional implementation for that?

Yes, it would be helpful if you mentioned that the priority could be less than zero.

It would be helpful to add the {@link DefaultMatcherOperatorRegistry} for an example.

The change was applied, but it does not appear in the git diff.

rlaxowns7916 · 2025-09-06T03:05:32Z

...ure-monkey-kotlin/src/main/kotlin/com/navercorp/fixturemonkey/kotlin/ExpressionGenerators.kt

        property.javaField?.annotations?.toList() ?: listOf()

-    override fun getType(): Class<*> = property.javaField!!.type
+    override fun getType(): Class<*> = property.javaField?.type ?: Object::class.java


To avoid NPEs when javaField is missing, I added a fallback to Object::class.java. Since this method returns Class<*> (Java reflection), Object is more explicit than Any even though both map to java.lang.Object at runtime.

If null is a valid return value, consider changing the return type to Class<*>? instead of falling back to Object::class.java.

Could you please provide some examples of situations in which the javaField is missing?

i commented it below

seongahjo · 2025-09-19T14:04:33Z

...i/src/main/java/com/navercorp/fixturemonkey/api/matcher/DefaultMatcherOperatorContainer.java

@@ -0,0 +1,84 @@
+package com.navercorp.fixturemonkey.api.matcher;


/* * Fixture Monkey * * Copyright (c) 2021-present NAVER Corp. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. */

Please add the license

The change was applied, but it does not appear in the git diff.

seongahjo · 2025-09-19T14:05:07Z

...pi/src/main/java/com/navercorp/fixturemonkey/api/matcher/DefaultMatcherOperatorRegistry.java

+import com.navercorp.fixturemonkey.api.property.Property;
+import com.navercorp.fixturemonkey.api.type.Types;
+
+public class DefaultMatcherOperatorRegistry<T> implements MatcherOperatorRegistry<T> {


There is no need to inherit right now; please add final.

I think the MatcherOperatorRegistry splits two concerns: the registry and the retriever.

The MatcherOperatorRegistry should have the methods addFirst and addLast.

A new interface is needed to get the List.

The implementations of two interface should be DefaultMatcherOperatorRegistry

I agree with your idea of separating concerns into a Retriever and a Registry.
The original DefaultMatcherOperatorRegistry was responsible for both concerns,
so I thought it would be clearer to rename it to Container.

seongahjo · 2025-09-19T14:06:28Z

...pi/src/main/java/com/navercorp/fixturemonkey/api/matcher/DefaultMatcherOperatorRegistry.java

+	public void addFirst(MatcherOperator<T> matcherOperator) {
+		addInternal(matcherOperator, true);
+	}
+
+	public void addLast(MatcherOperator<T> matcherOperator) {
+		addInternal(matcherOperator, false);
+	}
+
+	private void addInternal(MatcherOperator<T> matcherOperator, boolean prepend) {
+		int seq = sequenceIssuer.incrementAndGet();
+		int sequence = prepend ? -seq : seq;
+
+		Matcher matcher = matcherOperator.getMatcher();
+		T operator = matcherOperator.getOperator();
+
+		PriorityMatcherOperator<T> priorityMatcherOperator = new PriorityMatcherOperator<>(
+			matcher,
+			operator,
+			sequence
+		);
+
+		if (matcher instanceof ExactTypeMatcher) {
+			Class<?> type = ((ExactTypeMatcher)matcher).getType();
+			typeAwareIntrospectors.computeIfAbsent(type, k -> new ArrayList<>()).add(priorityMatcherOperator);
+		} else if (matcher instanceof AssignableTypeMatcher) {
+			Class<?> anchorType = ((AssignableTypeMatcher)matcher).getAnchorType();
+			typeAssignableIntrospectors.computeIfAbsent(anchorType, k -> new ArrayList<>())
+				.add(priorityMatcherOperator);
+		} else {
+			typeUnknownIntrospectors.add(priorityMatcherOperator);
+		}
+	}


Better readiness would be achieved if the methods were placed behind the overridden methods.

seongahjo · 2025-09-19T14:07:53Z

...onkey-api/src/main/java/com/navercorp/fixturemonkey/api/matcher/MatcherOperatorRegistry.java

@@ -0,0 +1,11 @@
+package com.navercorp.fixturemonkey.api.matcher;


/* * Fixture Monkey * * Copyright (c) 2021-present NAVER Corp. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. */

Please add the license

The change was applied, but it does not appear in the git diff.

seongahjo · 2025-09-19T14:12:55Z

...onkey-api/src/main/java/com/navercorp/fixturemonkey/api/matcher/MatcherOperatorRegistry.java

+public interface MatcherOperatorRegistry<T> {
+	List<MatcherOperator<T>> values();
+
+	List<MatcherOperator<T>> get(Property property);


'get' is ambiguous; it would be better to use getListByProperty.

More getXXX methods may be needed in the future.

The change was applied, but it does not appear in the git diff.

seongahjo · 2025-09-19T14:15:41Z

...onkey-api/src/main/java/com/navercorp/fixturemonkey/api/matcher/MatcherOperatorRegistry.java

+
+import com.navercorp.fixturemonkey.api.property.Property;
+
+public interface MatcherOperatorRegistry<T> {


Please add @API(since = "1.1.16", status = EXPERIMENTAL)

The change was applied, but it does not appear in the git diff.

seongahjo · 2025-09-21T12:42:48Z

...re-monkey-api/src/main/java/com/navercorp/fixturemonkey/api/option/FixtureMonkeyOptions.java

+		MatcherOperatorRetriever<PropertyGenerator> propertyGenerators,
 		PropertyGenerator defaultPropertyGenerator,
-		List<MatcherOperator<ObjectPropertyGenerator>> objectPropertyGenerators,
+		MatcherOperatorRetriever<ObjectPropertyGenerator> objectPropertyGenerators,


rlaxowns7916 · 2025-09-21T16:02:34Z

...i/src/main/java/com/navercorp/fixturemonkey/api/matcher/DefaultMatcherOperatorContainer.java

+		if (propertyType != UnidentifiableType.class) {
+			for (Map.Entry<Class<?>, List<PriorityMatcherOperator<T>>> e : typeAssignableIntrospectors.entrySet()) {


If the type cannot be identified, it is skipped.

rlaxowns7916 · 2025-09-21T16:03:07Z

...onkey-api/src/main/java/com/navercorp/fixturemonkey/api/matcher/PriorityMatcherOperator.java

-	private void checkPriority(int priority) {
-		if (priority < 0) {
-			throw new IllegalArgumentException("Priority must be greater than or equal to 0");
-		}
-	}


The change was applied, but it does not appear in the git diff.

rlaxowns7916 · 2025-09-21T16:04:29Z

...ure-monkey-kotlin/src/main/kotlin/com/navercorp/fixturemonkey/kotlin/ExpressionGenerators.kt

+    /**
+     * If backed only by a getter, {@code javaProperty} or {@code javaField} may be null and cause NPE.
+     * To avoid this, returns {@link Types.UnidentifiableType} when the type cannot be identified.
+     *
+     * @see com.navercorp.fixturemonkey.kotlin.expression.root
+     */
+    override fun getType(): Class<*> = property.javaField?.type ?: Types.UnidentifiableType::class.java
+
+    /**
+     * If backed only by a getter, {@code javaProperty} or {@code javaField} may be null and cause NPE.
+     * To avoid this, returns {@link Types.UnidentifiableType} when the type cannot be identified.
+     *
+     * @see com.navercorp.fixturemonkey.kotlin.expression.root
+     */
    override fun getAnnotatedType(): AnnotatedType =
-        property.javaField!!.annotatedType
+        property.javaField?.annotatedType ?: object : AnnotatedType {
+            override fun getType(): Type = Types.UnidentifiableType::class.java
+
+            override fun getAnnotations(): Array<Annotation> = emptyArray()
+
+            override fun <T : Annotation?> getAnnotation(annotationClass: Class<T>): T? = null
+
+            override fun getDeclaredAnnotations(): Array<Annotation> = emptyArray()
+        }


When extracting a JavaClass from a KProperty, the type can only be resolved if it is field-backed.
If the property is backed by a getter, the actual Java type cannot be determined, which may lead to a NullPointerException.

Previously, a generic Object type was returned in such cases, but this was ambiguous.

To make the intent explicit and allow the caller to handle branching logic more clearly, the return value was changed to the existing UnidentifiableType.

This triggered an NPE

rlaxowns7916 added 2 commits August 31, 2025 23:45

feat: MatcherOperator enhancement

e1f9fee

feat: MatcherOperator to interface and AssignableTypeMatcher as Map

2728b1d

github-actions bot requested review from SooKim1110, acktsap, benelog, jwChung, mhyeon-lee and seongahjo September 5, 2025 15:33

github-actions bot assigned rlaxowns7916 Sep 5, 2025

fix: lint

c98239a

rlaxowns7916 commented Sep 6, 2025

View reviewed changes

seongahjo reviewed Sep 19, 2025

View reviewed changes

seongahjo added this to the 1.1.16 milestone Sep 19, 2025

rlaxowns7916 added 6 commits September 20, 2025 00:02

chore: add license

d4eb972

chore: add experimental annotation

4362e61

fix: rename MatcherOperatorRegistry getListByProperty Method

7ebd8b2

fix: separate DefaultMatcherOperatorRegistry to two interfaces

84ca292

fix: change method declaration sequence for readability

ab1d773

chore: add PriorityMatcherOperator comment

53d1fd7

seongahjo reviewed Sep 21, 2025

View reviewed changes

fix: return UnidentifiableType when KProperty can't extract javaType

851d1be

rlaxowns7916 commented Sep 21, 2025

View reviewed changes

fix: ktlint

ef4d899

seongahjo approved these changes Oct 10, 2025

View reviewed changes

seongahjo merged commit bb81f25 into naver:main Oct 10, 2025
10 checks passed

		@@ -0,0 +1,84 @@
		package com.navercorp.fixturemonkey.api.matcher;

		@@ -0,0 +1,11 @@
		package com.navercorp.fixturemonkey.api.matcher;


		import com.navercorp.fixturemonkey.api.property.Property;

		public interface MatcherOperatorRegistry<T> {

		if (propertyType != UnidentifiableType.class) {
		for (Map.Entry<Class<?>, List<PriorityMatcherOperator<T>>> e : typeAssignableIntrospectors.entrySet()) {

refactor: MatcherOperatorRegistry and default impl #1237

refactor: MatcherOperatorRegistry and default impl #1237

Conversation

rlaxowns7916 commented Sep 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seongahjo Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seongahjo Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

seongahjo Sep 19, 2025 •

edited

Loading

seongahjo Sep 19, 2025 •

edited

Loading