Skip to content

Fix validate container expression for size in strict mode#1223

Merged
seongahjo merged 10 commits intonaver:mainfrom
Chanwon-Seo:fix-strict-mode-size-expression
Aug 29, 2025
Merged

Fix validate container expression for size in strict mode#1223
seongahjo merged 10 commits intonaver:mainfrom
Chanwon-Seo:fix-strict-mode-size-expression

Conversation

@Chanwon-Seo
Copy link
Contributor

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

  • Previously, strict mode only validated set/setNull expressions, but did not apply the same validation to size/container expressions.
  • This PR ensures that size, minSize, maxSize, etc. also throw exceptions for invalid container expressions in strict mode, making the behavior consistent.
  • Added and improved tests to verify that strict mode throws the correct exceptions for invalid container expressions.
  • Ensured that exception messages are clear and consistent.

How Has This Been Tested?

  • Added property-based and unit tests to verify that:
    • Invalid container expressions (e.g., size on non-existent fields) in strict mode throw the expected exceptions.
    • set/setNull and size operations now behave consistently in strict mode.
    • Non-strict mode does not throw exceptions for invalid expressions.

Is the Document updated?

N/A

@seongahjo
Copy link
Contributor

Sorry for the delay. I will check it as soon as possible.

@Chanwon-Seo Chanwon-Seo force-pushed the fix-strict-mode-size-expression branch from 8aa1100 to 9450013 Compare August 21, 2025 08:48
return delegate.size();
}

private boolean isValidFieldPath(
Copy link
Contributor

@seongahjo seongahjo Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@seongahjo seongahjo Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the late-night review!

I've addressed it in commit

@seongahjo seongahjo added this to the 1.1.15 milestone Aug 22, 2025
Copy link
Contributor

@seongahjo seongahjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@seongahjo seongahjo merged commit e02b882 into naver:main Aug 29, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing Validation for size() on Container Element Index Expressions

2 participants