fixture-monkey icon indicating copy to clipboard operation
fixture-monkey copied to clipboard

Extend MatcherOperator to manage ArbitraryBuilders with a single variable

Open YongGoose opened this issue 1 year ago • 2 comments

Describe the feature you request

  • Extending the MatcherOperator to remove namedArbitraryBuilder, allowing register operation selection through registeredArbitraryBuilders. https://github.com/naver/fixture-monkey/blob/b8641fcbd18f43648507ca53106c517a16dafcfe/fixture-monkey/src/main/java/com/navercorp/fixturemonkey/resolver/DefaultArbitraryBuilder.java#L190

(Optional): Suggest A Solution

  • Create a registeredName field in the MatcherOperator class.
  • In the initializeNamedArbitraryBuilderMap method while initializing the arbitraryBuilder store the name in the MatcherOperator.
  • In the selectName method identify the MatcherOperator using the name along with property.

following code is a simplified version for better understanding.

MatcherOperator

@API(since = "0.4.0", status = Status.MAINTAINED)
public final class MatcherOperator<T> implements Matcher {
	private final Matcher matcher;
	private final T operator;
	private final String registeredName;
	
	...
		
	@Override
	public boolean match(Property property) {
		return this.matcher.match(property) && registeredName == null;
	}
	public boolean match(Property property, String name) {
		return this.matcher.match(property) && registeredName.equals(name);
	}
	

        ...

FixtureMonkey - initializeNamedArbitraryBuilderMap

private void initializeNamedArbitraryBuilderMap(
	Map<String, MatcherOperator<Function<FixtureMonkey, ? extends ArbitraryBuilder<?>>>> mapsByRegisteredName
) {
	mapsByRegisteredName.forEach((name, matcherOperator) -> {
		registeredArbitraryBuilders.add(
			new MatcherOperator<>(matcherOperator.getMatcher(), matcherOperator.getOperator().apply(this), name)
		);
	});
}

DefaultArbitraryBuilder - selectName

@Override
public ArbitraryBuilder<T> selectName(String... names) {
	ArbitraryBuilderContext builderContext = new ArbitraryBuilderContext();

	for (String name : names) {
		builderContext = registeredArbitraryBuilders.stream()
			.filter(it -> it.match(rootProperty, name))
			.map(MatcherOperator::getOperator)
			.findAny()
			.map(DefaultArbitraryBuilder.class::cast)
			.map(DefaultArbitraryBuilder::getContext)
			.orElse(new ArbitraryBuilderContext());
	}

	return new DefaultArbitraryBuilder<>(
		this.fixtureMonkeyOptions,
		this.rootProperty,
		new ArbitraryResolver(
			this.traverser,
			this.manipulatorOptimizer,
			this.monkeyManipulatorFactory,
			this.fixtureMonkeyOptions,
			this.monkeyContext,
			this.registeredArbitraryBuilders
		),
		this.traverser,
		this.monkeyManipulatorFactory,
		builderContext.copy(),
		this.registeredArbitraryBuilders,
		this.monkeyContext,
		this.manipulatorOptimizer,
		this.fixtureMonkeyOptions.getInstantiatorProcessor()
	);
}

If the feature request is approved, would you be willing to submit a PR?

Yes

YongGoose avatar Oct 06 '24 15:10 YongGoose

@seongahjo

위 코드에서 Wrapper Class는 잘 생성이 되나, SimpleObject와 같이 값을 조합해서 만드는 객체 생성에서 문제가 생겼습니다. Basecase에서 문제가 생긴 것으로 추측이 되나, 아직 자세한 원인을 찾아 보지 않아 조금 더 찾아볼 계획입니다!

그 외, 한 가지 질문이 있습니다. 성아님께서 값을 property로만 식별하는 것이 아니라, propertyname으로도 식별하라고 하셨습니다. MatcherOperator 객체를 생성할 때, 위 코드와 같이 이름을 함께 등록하고 seleteName API에서 전달받은 이름과 property를 함께 사용해 값을 식별하는 방향이 맞는지 궁금합니다!

  • new MatcherOperator<>(matcherOperator.getMatcher(), matcherOperator.getOperator().apply(this), name)
  • .filter(it -> it.match(rootProperty, name))

YongGoose avatar Oct 06 '24 15:10 YongGoose

@YongGoose

객체를 생성할 때, 위 코드와 같이 이름을 함께 등록하고 seleteName API에서 전달받은 이름과 property를 함께 사용해 값을 식별하는 방향이 맞는지 궁금합니다!

new MatcherOperator<>(matcherOperator.getMatcher(), matcherOperator.getOperator().apply(this), name) .filter(it -> it.match(rootProperty, name))

MatcherOperator는 건들이지 않고, MatcherOperator를 확장해서 처리하는 방향으로 생각했습니다. 아직 일반화할 수 있을만한 경우의 수가 부족하기 때문에 MatcherOperator와 비슷한 역할을 하는 새로운 클래스를 만들어서 registerName 케이스만 처리해보시면 어떨까 싶습니다. registerMatcherOperator를 그대로 사용하게 두고 중간에 새로운 클래스로 변환해주는 어댑터를 둬야할 것 같습니다.

seongahjo avatar Oct 10 '24 11:10 seongahjo