fix(#3265781): add resource type route default for single-bundle views - #14
fix(#3265781): add resource type route default for single-bundle views#14Decipher wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughGenerated JSON:API view routes now resolve bundle resource types. Single-bundle views receive a singular ChangesView route resource type
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ViewRouteBuilder
participant ResourceTypeRepository
participant RouteMetadata
ViewRouteBuilder->>ResourceTypeRepository: Resolve bundle ResourceType objects
ResourceTypeRepository-->>ViewRouteBuilder: Return resolved types or NULL
ViewRouteBuilder->>RouteMetadata: Set plural types and conditional singular default
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.x-1.x #14 +/- ##
===========================================
+ Coverage 94.16% 94.32% +0.16%
===========================================
Files 3 3
Lines 137 141 +4
===========================================
+ Hits 129 133 +4
Misses 8 8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
18d585a to
7eb9fda
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Decipher
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Routing/Routes.php`:
- Around line 80-85: Update the route-building logic around the
bundle_resource_types mapping to remove unresolved NULL ResourceType results
before constructing resource_types and route defaults. Skip the route when no
bundle resource types resolve, and ensure single-bundle resource_type handling
never assigns NULL; add a Kernel test covering a view whose bundle resource type
is missing.
In `@tests/src/Kernel/JsonapiViewsResourceKernelTest.php`:
- Line 315: Update the assertion for JsonApiRoutes::RESOURCE_TYPE_KEY in the
multi-bundle route test to use hasDefault() and assert the default is absent,
rather than relying on getDefault() returning NULL. Preserve the existing route
setup and other assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 55d5c820-ad18-4c4c-8d89-fd0cce38ca93
📒 Files selected for processing (3)
AGENTS.mdsrc/Routing/Routes.phptests/src/Kernel/JsonapiViewsResourceKernelTest.php
9bd7f78 to
d01e7e6
Compare
d01e7e6 to
8b8a2b7
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/src/Kernel/JsonapiViewsResourceKernelTest.php (1)
292-294: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the cache assertion non-vacuous.
The loop passes when
callsByBundleis empty. That case does not prove that route rebuilding usedCountingResourceTypeRepository. Assert that the spy recorded calls before checking each count.Proposed fix
+ $this->assertNotEmpty($counting_repository->callsByBundle); foreach ($counting_repository->callsByBundle as $key => $count) { $this->assertSame(1, $count, "$key resolved more than once across the two views."); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/Kernel/JsonapiViewsResourceKernelTest.php` around lines 292 - 294, Make the cache assertion in the test method containing the callsByBundle loop non-vacuous by first asserting that CountingResourceTypeRepository recorded at least one call, then retain the per-bundle assertSame(1, $count) checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/src/Kernel/JsonapiViewsResourceKernelTest.php`:
- Around line 292-294: Make the cache assertion in the test method containing
the callsByBundle loop non-vacuous by first asserting that
CountingResourceTypeRepository recorded at least one call, then retain the
per-bundle assertSame(1, $count) checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e9ab77e5-78ca-4aca-b543-636b297141ef
📒 Files selected for processing (4)
CHANGELOG.mdsrc/Routing/Routes.phptests/src/Kernel/JsonapiViewsResourceKernelTest.phptests/src/Kernel/NullingResourceTypeRepository.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Routing/Routes.php
|
Superseded — merged upstream on Drupal.org as 4b17574 (squash of this branch's commits) and fast-forwarded here. |
1 participant
Fixes #3265781.
jsonapi_views-generated routes never carried the
resource_typeroute default that route/resource introspection code (for example the OpenAPI module's JSON:API discovery) reads to resolve a route to a singleResourceType.This does not use the community patch's/MR !18's approach. Both the original 2022 patch and the 2026 MR add
new ResourceType('view', 'view', ViewsResource::class)- a fake type unrelated to the view's real entity type/bundle(s). I installeddrupal/openapi_jsonapiand called its generator directly against a real site: that fake type produces zero-schema, generically-tagged ("View") OpenAPI entries for every exposed view, regardless of its real bundle(s). It avoids a crash but replaces it with silently wrong documentation. I also found the originally reported crash no longer reproduces against currentopenapi/openapi_jsonapireleases at all - their own null-guard (added 2020, ingetPaths()) already excludes routes with noresource_typedefault rather than erroring.Instead: a view can span multiple bundles (
_jsonapi_resource_types, plural, already reflects this), so a singleresource_typedefault only has one unambiguous answer when the view's entity type has exactly one bundle. For that case, this sets the realResourceTypealready returned byresourceTypeRepository->get()- no fabrication needed. For a view spanning several bundles, the default is left unset rather than reporting a made-up type.Verified manually against the real
openapi/openapi_jsonapimodules (not just the Kernel test assertions): a single-bundle diagnostic view resolves a real schema ($ref: #/definitions/user--user); a multi-bundle diagnostic view is correctly excluded.Adds Kernel test coverage for both the single-bundle and multi-bundle cases.
Verified on Drupal 10 and 11 locally via
make test-kernelandmake lint.Summary by CodeRabbit
Bug Fixes
Tests
Documentation