Refactor: Improve robustness in CosmosDBFilterExpressionConverter#5338
Open
ryujungkyun wants to merge 1 commit intospring-projects:mainfrom
Open
Refactor: Improve robustness in CosmosDBFilterExpressionConverter#5338ryujungkyun wants to merge 1 commit intospring-projects:mainfrom
ryujungkyun wants to merge 1 commit intospring-projects:mainfrom
Conversation
Resolves spring-projects#5337 Signed-off-by: 류정균 <prank01@naver.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #5337
This PR improves the robustness and readability of CosmosDBFilterExpressionConverter.
The changes are intentionally small and focused, addressing edge cases in the constructor and simplifying the doKey method to use more idiomatic Optional patterns.
All changes were made following a TDD-based approach, ensuring existing behavior is preserved while improving code quality.
before

The constructor previously collected the provided columns directly into a Map using Collectors.toMap.
Problems addressed:
Duplicate column names would cause IllegalStateException
No explicit validation for null input
Improvements:

Added Assert.notNull(columns, ...) to fail fast
Applied distinct() to safely handle duplicate column names
The previous implementation relied on Optional.isPresent() followed by Optional.get(), which is verbose and less expressive.
Using Optional.get() without proper guarding can result in a NoSuchElementException and offers little advantage over traditional null checks. In such cases, it effectively behaves the same as accessing a potentially null value directly.
Improvement:

Replaced with orElseThrow to clearly express that missing metadata configuration is an exceptional case
Improved readability and control flow
Testing (TDD)
The changes were implemented following a Test-Driven Development (TDD) approach.
Initially, I verified that an exception was thrown due to duplicate columns.

Verified that the constructor throws an IllegalArgumentException when the input is null
After modifying the application, I verified via unit tests that duplicate columns are handled safely without throwing exceptions
Verified that null input is explicitly rejected with an IllegalArgumentException
This test ensures the robustness of the doKey method. It initializes the converter with a specific field (e.g., "country") and attempts to create a filter expression using an unconfigured field (e.g., "city"). The test asserts that this invalid operation triggers an IllegalArgumentException with the message "No metadata field city has been configured", confirming that the orElseThrow logic works as expected.
I executed the full unit test suite to verify test isolation.
