-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix(core): Prevent redundant schema resolution by fixing AnnotatedType equality #4975
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
base: master
Are you sure you want to change the base?
Fix(core): Prevent redundant schema resolution by fixing AnnotatedType equality #4975
Conversation
Hi ewaostrowska, thank you for your time and effort maintaining this project. |
Hi @juntae6942, There are however few cases that could be addressed in this area:
We really appreciate your contribution, and we’d be happy to see an updated PR with these extras. Are you interested in taking it further? |
ewaostrowska Thanks for the detailed feedback! Yes, I'm happy to take this further. I'll look into the points you raised and update the PR. |
Hi ewaostrowska, |
…e equality Signed-off-by: potato <[email protected]>
8da3b8a
to
dfb9076
Compare
hi @juntae6942, thanks for taking it further |
Pull Request
Description
This pull request resolves a performance issue where the ModelConverterContext fails to cache AnnotatedType instances, leading to redundant schema resolutions.
The root cause was a faulty equals() and hashCode() implementation in the AnnotatedType class. These methods incorrectly included contextual fields (e.g., propertyName), causing logically identical types to be treated as different objects by the cache (processedTypes Set). This bug resulted in the same types being processed multiple times during schema generation, causing significant performance degradation.
This commit corrects the equals() and hashCode() implementations to only consider fields that define a type's intrinsic identity, ensuring the cache functions as expected.
Closes: #4965
Type of Change
Checklist
Screenshots / Additional Context
Test Case Explanation:
A new test, AnnotatedTypeCachingTest.java, has been added to reproduce the bug and verify the fix.
Why a DummyModelConverter is used: The standard ModelResolver has internal optimizations for simple types like String, which prevents it from recursively calling the ModelConverterContext. This interferes with testing the context's caching behavior. To solve this, the test uses a DummyModelConverter that simulates the recursive resolution of a model in a controlled environment, allowing us to test the caching logic in isolation.
How it Works:
Before the fix: The test fails because the faulty equals() implementation causes multiple AnnotatedType instances for the String type to be added to the cache. The assertion assertEquals(stringTypeCount, 1) fails because the actual count is greater than 1.
After the fix: The test passes because the corrected equals() implementation ensures that only one AnnotatedType instance for the String type is stored in the cache. The assertion assertEquals(stringTypeCount, 1) succeeds.
This test provides clear, automated proof that the caching issue is resolved.