feat: add NullOnCircularReference strategy to NullableStrategy#38
Conversation
- Added NullOnCircularReference to NullableStrategy - Updated ResolverChain to break cycles for nullable fields when using this strategy - Set NullOnCircularReference as the default strategy in SomeConfig - Updated NullableResolver and Some.kt to support the new strategy - Added CircularReferenceIntegrationTest and updated existing tests Co-authored-by: MessiasLima <[email protected]>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the NullOnCircularReference strategy as the new default for handling nullable types in circular dependencies, enabling recursive data structures to resolve to null rather than throwing an exception. The implementation involves updates to ResolverChain, SomeConfig, and NullableResolver. Review feedback identifies a logic error where the circularity check should precede the exception-throwing match, suggests a performance optimization for type comparisons in isCircularIgnoringNullability, and points out potential consistency risks due to redundant strategy state and the mutability of the global configuration.
| */ | ||
| class NullableResolver( | ||
| private val nullableStrategy: NullableStrategy = NullableStrategy.Random(), | ||
| private val nullableStrategy: NullableStrategy = NullableStrategy.NullOnCircularReference, |
There was a problem hiding this comment.
There is now a redundancy between the nullableStrategy stored in this resolver and the one passed to ResolverChain. If the global defaultConfig is mutated after defaultResolvers has been initialized, these two values will diverge, leading to inconsistent behavior. To ensure consistency, this resolver should ideally retrieve the strategy from the chain parameter during the resolve call.
|
|
||
| val defaultResolvers: List<TypeResolver> by lazy { SomeConfig().buildResolvers() } | ||
| val defaultConfig: SomeConfig by lazy { SomeConfig() } | ||
| val defaultResolvers: List<TypeResolver> by lazy { defaultConfig.buildResolvers() } |
There was a problem hiding this comment.
Note that defaultResolvers is lazily initialized using the initial state of defaultConfig. Since SomeConfig properties (like nullableStrategy) are mutable, any changes made to defaultConfig after defaultResolvers has been accessed will not be reflected in the existing resolvers. This creates a mismatch where the ResolverChain uses the updated strategy but the resolvers continue using the old one. Consider making SomeConfig immutable or refactoring resolvers to access strategies dynamically from the session context.
- Added NullOnCircularReference to NullableStrategy - Updated ResolverChain to break cycles for nullable fields when using this strategy - Set NullOnCircularReference as the default strategy in SomeConfig - Updated NullableResolver and Some.kt to support the new strategy - Added CircularReferenceIntegrationTest with proper import ordering - Updated existing tests for ResolverChain changes Co-authored-by: MessiasLima <[email protected]>
MessiasLima
changed the title
MessiasLima
deleted the
feat/null-on-circular-reference-5388625712608883014
branch
1 participant
Implemented the NullOnCircularReference strategy to gracefully handle recursive data structures by resolving circular nullable fields to null. This is now the default behavior, replacing the previous Random strategy. Non-nullable circular references continue to throw SomeCircularReferenceException. Existing strategies' behavior regarding circular references remains unchanged.
Fixes #34
PR created automatically by Jules for task 5388625712608883014 started by @MessiasLima