Fix(core): Prevent redundant schema resolution by fixing AnnotatedType equality

[juntae6942]

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

• 🐛 Bug fix
• ✨ New feature
• ♻️ Refactor (non-breaking change)
• 🧪 Tests
• 📝 Documentation
• 🧹 Chore (build or tooling)

Checklist

• I have added/updated tests as needed
• I have added/updated documentation where applicable
• The PR title is descriptive
• The code builds and passes tests locally
• I have linked related issues (if any)

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.

[juntae6942]

Hi ewaostrowska, thank you for your time and effort maintaining this project.
I’d really appreciate it if you could take a look at this PR whenever you have a chance 🙏

[ewaostrowska]

Hi @juntae6942,
thanks a ton for opening the PR and digging into this!
Your change does fix the redundant schema resolutions and there are less random cache misses. Common types (such as Strings) won't be re-resolved over and over again. The provided tests are a great documentation of the problem.

There are however few cases that could be addressed in this area:

1. Order-sensitive annotations
   ctx = {@A, @B} vs {@B, @A} // NOT equal → extra resolution
   equals uses Arrays.equals(ctxAnnotations, …), which is order-sensitive. If callers produce the same effective set of annotations in different orders, there will still be missed cache hits.

2. JDK/internal annotation noise
   If one call site picks up a jdk./sun. annotation and another doesn’t, the arrays differ → not equal (even if hashCode tries to ignore them).
   hashCode tries to ignore sun./jdk., but equals still compares the raw arrays, would be nice to have these two aligned

3. Mutability hazard
   ctxAnnotations is used in equality but is mutable; changing it after insertion into a Set can corrupt lookups.
   ctxAnnotations participates in equality, and it’s an array stored on the object; if it’s mutated after insertion into a Set, lookups can break.

4. getClass() != o.getClass() disallows subclass equality.
   At the moment we don not inherit but it’s a behavioral change vs instanceof and would be good to have a final keyword on the AnnotatedType if we make a change. Currently I think it's better to leave it with instanceOf

5. Adding few tests to the cases above and toggling fields excluded from equality (name, resolveAsRef, etc.)

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?

[juntae6942]

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.

[juntae6942]

Hi ewaostrowska,
I would be grateful if you could review my changes and let me know if the approach I took is appropriate.