-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement a Matcher interface to manage ArbitraryBuilders with a single variable #1062
Changes from 6 commits
001f026
e18495e
c7dc4fb
73e1116
85eb885
5b35d26
fa4a08c
a5136b3
e29ce3a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
/* | ||
* 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. | ||
*/ | ||
|
||
package com.navercorp.fixturemonkey.api.matcher; | ||
|
||
public interface MatcherMetadata<T> { | ||
T getMetadataInfo(); | ||
} | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
package com.navercorp.fixturemonkey.api.matcher; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 라이센스를 추가하면 좋을 것 같습니다.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 넵! 수정하겠습니다. |
||
|
||
import com.navercorp.fixturemonkey.api.property.Property; | ||
|
||
public final class NamedMatcher implements Matcher { | ||
private final Matcher matcher; | ||
private final String registeredName; | ||
|
||
public NamedMatcher(Matcher matcher, String registeredName) { | ||
this.matcher = matcher; | ||
this.registeredName = registeredName; | ||
} | ||
|
||
public boolean matchRegisteredName(String registeredName) { | ||
return this.registeredName.equals(registeredName); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이건 없어져도 될 것 같습니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 넵! 삭제하도록 하겠습니다. |
||
|
||
@Override | ||
public boolean match(Property property) { | ||
return this.matcher.match(property); | ||
} | ||
|
||
@Override | ||
public boolean match(Property property, MatcherMetadata<?> matcherMetadata) { | ||
return this.matcher.match(property) && registeredName.equals(matcherMetadata.getMetadataInfo()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
/* | ||
* 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. | ||
*/ | ||
|
||
package com.navercorp.fixturemonkey.api.matcher; | ||
|
||
import javax.annotation.Nullable; | ||
|
||
public class NamedMatcherMetadata<T> implements MatcherMetadata<T> { | ||
private final T medataInfo; | ||
|
||
public NamedMatcherMetadata(@Nullable T medataInfo) { | ||
this.medataInfo = medataInfo; | ||
} | ||
|
||
public static <T> NamedMatcherMetadata<T> empty() { | ||
return new NamedMatcherMetadata<>(null); | ||
} | ||
|
||
@Override | ||
public T getMetadataInfo() { | ||
return medataInfo; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,7 @@ | |
import com.navercorp.fixturemonkey.api.generator.ArbitraryContainerInfo; | ||
import com.navercorp.fixturemonkey.api.lazy.LazyArbitrary; | ||
import com.navercorp.fixturemonkey.api.matcher.MatcherOperator; | ||
import com.navercorp.fixturemonkey.api.matcher.NamedMatcherMetadata; | ||
import com.navercorp.fixturemonkey.api.property.Property; | ||
import com.navercorp.fixturemonkey.customizer.InnerSpecState.ManipulatorHolderSet; | ||
import com.navercorp.fixturemonkey.customizer.Values.Just; | ||
|
@@ -142,7 +143,8 @@ public ContainerInfoManipulator newContainerInfoManipulator( | |
|
||
public List<ArbitraryManipulator> newRegisteredArbitraryManipulators( | ||
List<MatcherOperator<? extends ArbitraryBuilder<?>>> registeredArbitraryBuilders, | ||
Map<Property, List<ObjectNode>> nodesByType | ||
Map<Property, List<ObjectNode>> nodesByType, | ||
ArbitraryBuilderContext builderContext | ||
) { | ||
List<ArbitraryManipulator> manipulators = new ArrayList<>(); | ||
|
||
|
@@ -152,7 +154,14 @@ public List<ArbitraryManipulator> newRegisteredArbitraryManipulators( | |
|
||
DefaultArbitraryBuilder<?> registeredArbitraryBuilder = | ||
(DefaultArbitraryBuilder<?>)registeredArbitraryBuilders.stream() | ||
.filter(it -> it.match(property)) | ||
.filter(it -> { | ||
if (builderContext.getSelectedNames().isEmpty()) { | ||
return it.match(property, NamedMatcherMetadata::empty); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
이렇게 진행을 하면 registeredName API를 통해 register 연산을 등록한 후, selectName을 호출하지 않았는데도 적용이 되는 사이드 이펙트가 발생할 것 같습니다. 실제로 실행을 시켜본 결과 하기와 같은 테스트 코드가 성공함을 확인했습니다. registeredName API를 초기 설계할 때 선택된 이름만 적용이 되도록 설계를 해서 위와 같은 경우는 적용이 되면 안 되는 경우라고 생각합니다.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
registeredName도 register인 만큼 랜덤하게 선택된다고 생각했는데 아니었군요. 이해하기 쉬운 주문 도메인을 구현한다고 가정했을 때, 제가 생각하는 시나리오는 아래와 같습니다.
주문을 생성할 때마다 위에 두 register로 등록한 연산 셋 중 하나를 선택해서 생성합니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 고민해보니, 성아님의 말씀대로 현재 설계가 InnerSpec과 크게 다르지 않은 것 같습니다. 성아님의 의견에 전적으로 동의를 하는데 한 가지 궁금한 점이 있습니다.
현재 registeredName은 selectName이 없는 경우 가장 먼저 호출된 registeredName이 적용되는 구조입니다.
그리고 제 개인적인 생각인데 JUnit의
코드와 함께 추가 설명을 하자면, 하기의 코드와 같이 String이라는 클래스에 대해 register 연산이 여러 개 등록된 경우 가장 먼저 등록된 배송이 있는 주문이 선택 됩니다.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
`첫 번째로 등록한 연산을 적용하는 것보다 적용할 regiserGroup을 임의로 (arbitrary) 정하는 방향을 생각했습니다. 만약 통과하지 못한다면 다음 두 가지 경우일 것이라고 생각합니다.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
이번 PR의 목적은 ArbitraryBuilder를 관리하는 부분을 리팩토링하는 것이므로, 이번 PR과는 내용이 어울리지 않다고 생각합니다!
넵! 성아님께서 괜찮으시다면 연산을 임의로 선택하는 것과 함께 고민해보겠습니다 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 넵 다음 PR에서 진행하시죠 👍 |
||
return builderContext.getSelectedNames().stream().anyMatch( | ||
name -> it.match(property, new NamedMatcherMetadata<>(name)) | ||
); | ||
}) | ||
.findFirst() | ||
.map(MatcherOperator::getOperator) | ||
.filter(it -> it instanceof DefaultArbitraryBuilder<?>) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,7 @@ | |
public final class ArbitraryBuilderContext { | ||
private final List<ArbitraryManipulator> manipulators; | ||
private final List<ContainerInfoManipulator> containerInfoManipulators; | ||
private final List<String> selectNames; | ||
private final Map<Class<?>, List<Property>> propertyConfigurers; | ||
private final Map<Class<?>, ArbitraryIntrospector> arbitraryIntrospectorsByType; | ||
private boolean validOnly; | ||
|
@@ -54,6 +55,7 @@ public final class ArbitraryBuilderContext { | |
public ArbitraryBuilderContext( | ||
List<ArbitraryManipulator> manipulators, | ||
List<ContainerInfoManipulator> containerInfoManipulators, | ||
List<String> selectNames, | ||
Map<Class<?>, List<Property>> propertyConfigurers, | ||
Map<Class<?>, ArbitraryIntrospector> arbitraryIntrospectorsByType, | ||
boolean validOnly, | ||
|
@@ -62,6 +64,7 @@ public ArbitraryBuilderContext( | |
) { | ||
this.manipulators = manipulators; | ||
this.containerInfoManipulators = containerInfoManipulators; | ||
this.selectNames = selectNames; | ||
this.propertyConfigurers = propertyConfigurers; | ||
this.arbitraryIntrospectorsByType = arbitraryIntrospectorsByType; | ||
this.validOnly = validOnly; | ||
|
@@ -71,7 +74,8 @@ public ArbitraryBuilderContext( | |
|
||
public ArbitraryBuilderContext() { | ||
this( | ||
new ArrayList<>(), new ArrayList<>(), new HashMap<>(), new HashMap<>(), true, null, null); | ||
new ArrayList<>(), new ArrayList<>(), new ArrayList<>(), new HashMap<>(), new HashMap<>(), true, null, null | ||
); | ||
} | ||
|
||
public ArbitraryBuilderContext copy() { | ||
|
@@ -82,6 +86,7 @@ public ArbitraryBuilderContext copy() { | |
return new ArbitraryBuilderContext( | ||
new ArrayList<>(this.manipulators), | ||
copiedContainerInfoManipulators, | ||
this.selectNames, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
new HashMap<>(propertyConfigurers), | ||
new HashMap<>(arbitraryIntrospectorsByType), | ||
this.validOnly, | ||
|
@@ -114,6 +119,15 @@ public List<ContainerInfoManipulator> getContainerInfoManipulators() { | |
return Collections.unmodifiableList(containerInfoManipulators); | ||
} | ||
|
||
public void addSelectedNames(List<String> selectNames) { | ||
this.selectNames.addAll(selectNames); | ||
} | ||
|
||
public List<String> getSelectedNames() { | ||
return this.selectNames; | ||
} | ||
|
||
|
||
public void putPropertyConfigurer(Class<?> type, List<Property> propertyConfigurer) { | ||
this.propertyConfigurers.put(type, propertyConfigurer); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제네릭을 사용하는 것보다 스프링에 있는
AnnotatedTypeMetadata
처럼 구현하는 방향이 어떨까 싶습니다.당장 필요한 정보는 name이므로
String getName()
메서드를 추가하는 건 어떨까요?https://github.com/spring-projects/spring-framework/blob/a3b979c5ecb7eda96ebf264672ce522177c6fc77/spring-core/src/main/java/org/springframework/core/type/AnnotatedTypeMetadata.java#L54
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵!
추후에 필요한 정보가 더 생기면 확장성을 고려해서 리팩토링 하겠습니다 :)