Skip to content

fix(apt): detect cycles through createDefaultVariable=false entities - #1951

Open
o54711254 wants to merge 1 commit into
OpenFeign:masterfrom
o54711254:fix/apt-cycle-detection-through-disabled-default-variable
Open

o54711254 wants to merge 1 commit into
OpenFeign:masterfrom
o54711254:fix/apt-cycle-detection-through-disabled-default-variable

Conversation

@o54711254

Copy link
Copy Markdown
Contributor

Background

Follow-up to @velo's review on #1905.

Closes #1949.

AbstractQuerydslProcessor.detectCircularQClassReferences previously filtered context.entityTypes by createDefaultVariable() before passing the map to QClassCycleDetector.

Because the detector resolves neighbors through that map, removing an entity also removes the reference paths through it. However, createDefaultVariable controls static default instance generation; it does not remove constructor references to other Q-classes.

As a result, a cycle passing through a createDefaultVariable=false entity could go undetected even when the remaining static default instances could cause a class initialization deadlock.

Changes

  • Traverse the full context.entityTypes map without filtering out intermediate entities.
  • Return List<List<EntityType>> from QClassCycleDetector.detect so the processor can check each entity's configuration after detection.
  • Report a detected cycle only when at least two distinct entities have createDefaultVariable() == true.
  • Use .distinct() to avoid counting the repeated endpoint twice.
  • Keep the existing DFS traversal and warning format.

The two-entity threshold follows the reasoning in the #1905 review: with only one static default instance, the circular initialization re-entry occurs on the same thread, which JVM class initialization permits.

Tests

  • Adapt QClassCycleDetectorTest to the new return type while preserving existing graph assertions.
  • Verify that a three-entity cycle produces a warning when the middle entity has @Config(createDefaultVariable = false).
  • Verify that a bidirectional pair produces no warning when static default instances are disabled globally.
  • Verify that a bidirectional pair produces no warning when only one entity generates a static default instance.
  • Assert that the warning includes -Aquerydsl.createDefaultVariable=false.
  • Retain the existing self-reference suppression tests.

The circular Q-class detector was pre-filtering entities by
createDefaultVariable() before walking the graph. Because neighbors are
resolved through that same map, dropping a node also removed its edges
— so a cycle passing through a createDefaultVariable=false entity was
silently missed, even though the remaining entities still emit static
default instances and can deadlock at class initialization.

Walk the full context.entityTypes so every edge is traversed, and
filter the emitted cycles instead: report one only when at least two
of its nodes have createDefaultVariable() == true. Two rather than one
because a single static instance re-enters on the same thread, and JVM
class initialization is re-entrant.

Also pins the -Aquerydsl.createDefaultVariable=false suggestion string
via hadWarningContaining, so the flag name added in OpenFeign#1905 is covered.

Follow-up to OpenFeign#1905 (see approval comment). Closes OpenFeign#1949.
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.

Circular Q-class detection misses cycles passing through a createDefaultVariable=false entity

1 participant