Skip to content

Reach the index fallbacks when a resource is not on its URI path - #49

Open
koriym wants to merge 1 commit into
masterfrom
fix/resource-index-fallback-reachability
Open

Reach the index fallbacks when a resource is not on its URI path#49
koriym wants to merge 1 commit into
masterfrom
fix/resource-index-fallback-reachability

Conversation

@koriym

@koriym koriym commented Aug 21, 2026

Copy link
Copy Markdown
Member

背景

#46 のレビュー中に、ResourceMethodTypeProvider の URI 解決に到達不能なコードがあることが分かりました。

resolveResourceClass は4段の解決戦略を並べていますが、3段目と4段目に到達しません:

VirtualFile targetFile = baseDir.findFileByRelativePath(relPath);
if (targetFile == null) {
    return Optional.empty();        // ← ここで打ち切られる
}
...
resolveResourceClassFromIndex(...)          // 到達しない
resolveResourceClassFromFilenameIndex(...)  // 到達しない

リソースが URI の示すパスに無いとき、findFileByRelativePath が null を返して打ち切られます。インデックス経路が動くのは「そのパスにファイルは存在するが PHP クラスを含まない」という狭い場合だけでした。

これは戦略の意図と逆です。クラス名やファイル名で引くのは、リソースがプロジェクトルート外にある構成を解決するためのもので、早期 return が弾いていたのはまさにその構成でした。

cf79ae2(2026-06-10)由来で、#46 とは独立した既存の問題です。#46 が置換した getVirtualFilesByName はこの到達不能メソッドの中にあり、テストが無かったのもこれが理由です。

変更

  1. 各戦略を素通りさせる — 見つからなければ次の戦略へ。resolveResourceClassFromBaseDir として抽出

  2. インデックス経路に vendor ガードを追加 — 到達可能になったことで顕在化するリスクへの対処です。composer で入れた BEAR アプリは src/Resource/App/*.php\Resource\App\Foo で終わる名前空間で持つため、FQN 末尾一致にもパス末尾一致にも形だけで合致します。依存パッケージのリソースに解決してしまうと、ナビゲーションも型推論もそちらへ飛びます

    • FilenameIndex: allScopeprojectScope、加えて /vendor/ を除外
    • PhpIndex: isInVendor() で除外

テスト

ResourceIndexResolutionFixtureTest を新設しました。インデックス経路の検証にはフィクスチャがコンテントルートを持つ必要があり、既存の ResourceMethodTypeProviderFixtureTest でそれをやると ProjectUtil.guessProjectDir の返す値が変わって page context 判定が壊れるため、別クラスにしています(理由は Javadoc に記載)。

3件とも、対応する修正を外すと落ちることを確認済みです:

テスト 外すと落ちるもの
resolvesResourceOutsideUriPathThroughClassNameIndex 制御フロー修正
resolvesResourceOutsideUriPathThroughFilenameIndex 制御フロー修正
ignoresVendoredResourceSharingTheUriShape vendor ガード

2番目は、#46 が置換した getVirtualFilesByName を実際に通す唯一の経路です。

./gradlew test 155件 green。

🤖 Generated with Claude Code

Summary by CodeRabbit

  • 改善

    • リソースURIの解決精度を向上しました。
    • 複数の解決方法を順番に試行し、該当するリソースを見つけやすくしました。
    • プロジェクト内のリソースのみを対象とし、vendor/ 配下のリソースを除外します。
  • テスト

    • クラス名・ファイル名によるリソース解決と、ベンダーリソース除外の動作を追加検証しました。

resolveResourceClass listed four resolution strategies, but the third and
fourth were unreachable: when the resource was absent from the path its URI
names, findFileByRelativePath returned null and the method returned empty
instead of falling through. The index strategies only ran for the narrow case
of a file that exists at the expected path yet declares no PHP class.

That inverted their purpose. Looking the resource up by class name or by
filename is what resolves layouts where resources live outside the project
root, and those are exactly the layouts the early return rejected.

Let each strategy fall through, and guard the two index strategies now that
they run: an installed BEAR application ships src/Resource/App/*.php under a
namespace ending in \Resource\App\Foo, so it matches both the FQN suffix and
the path suffix on shape alone, and resolving to it would send navigation and
type inference into a dependency.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 39b418a7-4344-4c0d-86fd-64159ff274a3

📥 Commits

Reviewing files that changed from the base of the PR and between 84de243 and 15597be.

📒 Files selected for processing (2)
  • src/main/java/idea/bear/sunday/resource/ResourceMethodTypeProvider.java
  • src/test/java/idea/bear/sunday/resource/ResourceIndexResolutionFixtureTest.java

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

変更概要

リソースクラス解決のフォールバック順序を整理しました。PHPインデックスとファイル名インデックスでは、vendor/ 配下を除外します。各解決方式を検証するフィクスチャテストを追加しました。

Changes

リソース解決

Layer / File(s) Summary
解決戦略とベンダー除外
src/main/java/idea/bear/sunday/resource/ResourceMethodTypeProvider.java
NIOパス、プロジェクト相対パス、PHPインデックス、ファイル名インデックスの順で解決します。ファイル名インデックスをプロジェクトスコープに限定し、両インデックスで vendor/ 配下を除外します。
インデックス解決のフィクスチャテスト
src/test/java/idea/bear/sunday/resource/ResourceIndexResolutionFixtureTest.java
一時ソースルートを登録し、クラス名インデックス、ファイル名インデックス、ベンダーリソース除外を検証します。

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 15597

The change enables intended fallback resolution and adds vendor filtering; no actionable merge-blocking risk remains beyond normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant 呼び出し元PHP
  participant ResourceMethodTypeProvider
  participant PHPインデックス
  participant ファイル名インデックス
  呼び出し元PHP->>ResourceMethodTypeProvider: リソースURIを解決
  ResourceMethodTypeProvider->>PHPインデックス: クラス候補を検索
  PHPインデックス-->>ResourceMethodTypeProvider: vendor除外済み候補
  ResourceMethodTypeProvider->>ファイル名インデックス: 未解決時にファイル名を検索
  ファイル名インデックス-->>ResourceMethodTypeProvider: プロジェクト内候補
  ResourceMethodTypeProvider-->>呼び出し元PHP: 型またはnullを返却
Loading

Suggested reviewers: suzumaze

Poem

ぴょんと跳ねて、道をたどる
NIOの先も、次へ進む
vendorの森は通り抜け
インデックスでクラスを見つけ
うさぎも安心、解決完了 🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.88% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed リソースがURIパスにない場合にインデックス解決へフォールバックする主な変更を、明確かつ簡潔に示しています。
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/resource-index-fallback-reachability

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

suzumaze added a commit to suzumaze/idea-php-bearsunday-plugin that referenced this pull request Aug 21, 2026
Mirrors the vendor guard of bearsunday#49 (bearsunday#49)
into the resolver this branch extracted the fallbacks into. An installed
BEAR application carries src/Resource/App/*.php of its own, matching both
the path suffix and the FQN tail by shape alone, so the index stages
could answer a URI with a dependency's class -- and every MCP resource
tool reads through this resolver, not only the type provider.

The control flow bearsunday#49 also repairs is already correct here: each stage is
a private helper and resolveCached() falls through to the next one.
No test is added: bearsunday#49 ships ResourceIndexResolutionFixtureTest, which
drives ResourceMethodTypeProvider.complete() and therefore this resolver,
and it arrives on this branch when master is merged after bearsunday#49 lands.
@suzumaze

Copy link
Copy Markdown
Contributor

(このコメントはsuzumazeの環境で動作しているClaude Codeが、suzumazeの指示と確認のもとで書いています。)

到達不能の指摘、ありがとうございます。#46で置き換えたgetVirtualFilesByNameがそのメソッドの中にあった、というのはこちらでも確認しました。テストが無かった理由の説明としても腑に落ちます。

ひとつ、#47との相互作用をお知らせします。#47はresolveResourceClassまわりをResourceClassResolverへ抽出していて、同じ箇所を削除します。そのため、

これを避けるため、#47側にvendorガードを先に移植しました(4995f76、本PRのミラーである旨をコミットメッセージに明記)。#49がどの順で入っても、ガードが失われないようにしてあります。

なお制御フローの方は、#47のResourceClassResolver独立に同じ結論に到達していました。各段をprivateヘルパーにしてresolveCached()が順に落ちる形で、本PRのresolveResourceClassFromBaseDir抽出と同じ構造です。別の経路から同じ設計になったので、修正の裏付けにはなるかと思います。

vendorガードのテストは#47側には追加していません。本PRのResourceIndexResolutionFixtureTestResourceMethodTypeProvider.complete()を駆動する作りなので、#49マージ後に#47がmasterを取り込めば、そのまま抽出先の検証になります。重複を避けてそちらに委ねています。

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.

2 participants