Fix validate container expression for size in strict mode#1223
Fix validate container expression for size in strict mode#1223seongahjo merged 10 commits intonaver:mainfrom
Conversation
fixture-monkey/src/test/java/com/navercorp/fixturemonkey/test/FixtureMonkeyOptionsTest.java
Show resolved
Hide resolved
...ure-monkey/src/main/java/com/navercorp/fixturemonkey/expression/ExpressionPathValidator.java
Outdated
Show resolved
Hide resolved
...ure-monkey/src/main/java/com/navercorp/fixturemonkey/expression/ExpressionPathValidator.java
Outdated
Show resolved
Hide resolved
...c/main/java/com/navercorp/fixturemonkey/expression/StrictModeNextNodePredicateContainer.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/navercorp/fixturemonkey/expression/StrictModeMonkeyExpressionFactory.java
Outdated
Show resolved
Hide resolved
fixture-monkey/src/main/java/com/navercorp/fixturemonkey/FixtureMonkeyBuilder.java
Outdated
Show resolved
Hide resolved
|
Sorry for the delay. I will check it as soon as possible. |
...c/main/java/com/navercorp/fixturemonkey/expression/StrictModeNextNodePredicateContainer.java
Outdated
Show resolved
Hide resolved
fixture-monkey/src/main/java/com/navercorp/fixturemonkey/FixtureMonkeyBuilder.java
Outdated
Show resolved
Hide resolved
fixture-monkey/src/main/java/com/navercorp/fixturemonkey/FixtureMonkeyBuilder.java
Outdated
Show resolved
Hide resolved
8aa1100 to
9450013
Compare
| return delegate.size(); | ||
| } | ||
|
|
||
| private boolean isValidFieldPath( |
There was a problem hiding this comment.
I think there is a limitation in that it cannot check the element.
For example, it would not throw an exception in the following case:
String actual = sut.giveMeBuilder(new TypeReference<List<List<SimpleObject>>>() {
})
.size("$", 1)
.size("$[0]", 1)
.set("$[0][1].str", "expected")
.sample()
.get(0)
.get(0)
.getStr();
Nevertheless, it is a useful feature, so adding a comment to describe the limitation would be better.
There was a problem hiding this comment.
For example, it would not throw an exception in the following case:
here is a passing test case.
thenThrownBy(
() -> sut.giveMeBuilder(new TypeReference<List<List<SimpleObject>>>() {
})
.size("$", 1)
.size("$[0]", 1)
.set("$[0][1].str", "expected")
.sample()
).isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("No matching results for given NodeResolvers.");As for the other test case you suggested, I felt it was already covered by the existing tests, so I didn't add it separately.
However, if you still think a more explicit test would be valuable, I'm happy to add it.
On a related note, I'm not sure about the best way to proceed with the isValidFieldPath method.
Should Iadd a comment to explain its current limitations, or would adding a test case that demonstrates the issue be sufficient?
If you think a comment is the right approach, could you please recommend what it should say?
There was a problem hiding this comment.
Sorry, I've misunderstood. The above case was worked on using the existing MonkeyExpressionFactory.
I mean, in the case below, it does not throw an error even if the first element does not exist.
FixtureMonkey sut = FixtureMonkey.builder().useExpressionStrictMode().build();
List<SimpleObject> actual = sut.giveMeBuilder(new TypeReference<List<List<SimpleObject>>>() {
})
.size("$", 1)
.size("$[1]", 1)
.sample()
.get(0);
I think it's an edge case. It would be better to write a comment about the limitations.
There was a problem hiding this comment.
Thanks for the late-night review!
I've addressed it in commit
Summary
Fix strict mode so that container expression validation (e.g., size) throws exceptions for invalid paths, and add corresponding tests.
Related issues:
Fixes #1219
(Optional): Description
How Has This Been Tested?
Is the Document updated?
N/A