Skip to content

include_file DSL が使えなくなっていたので修正(再) - #74

Merged
haruyan-hopemucci merged 4 commits into
masterfrom
fix/degrated-include-file-dsl-and-eror-exit-if-raises
Jul 9, 2026
Merged

haruyan-hopemucci merged 4 commits into
masterfrom
fix/degrated-include-file-dsl-and-eror-exit-if-raises

Conversation

@lobin-z0x50

Copy link
Copy Markdown
Member

#73 が #72 マージ後にそっちに入ってしまったので、master 向けに出し直します。

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the DSL file inclusion mechanism by moving the include_file logic from Loader to Evaluator, enabling recursive evaluation of nested configuration files with relative paths. It also updates the Nginx main renderer initializer to accept an optional configuration, adds set -eo pipefail to reload_config.sh, and introduces corresponding tests. Feedback suggests securing the directory resolution in reload_config.sh against spaces in paths, and improving include_file in Evaluator by adding input validation and circular dependency detection to prevent stack overflow.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/reload_config.sh
@@ -1,4 +1,5 @@
#!/bin/bash
set -eo pipefail
SCRIPT_DIR=$(cd $(dirname $0) && pwd)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

スクリプトが配置されているディレクトリのパスにスペースが含まれている場合、cd $(dirname $0) が正しく動作せずエラーになる可能性があります。変数の展開結果をダブルクォーテーションで囲むことで、スペースを含むパスに対しても安全に動作するように改善することをおすすめします。

Suggested change
SCRIPT_DIR=$(cd $(dirname $0) && pwd)
SCRIPT_DIR=$(cd "$(dirname "$0")" && pwd)

Comment on lines +79 to +95
def include_file(file_path)
file_path = if @current_file
File.expand_path file_path, File.dirname(@current_file)
else
File.expand_path file_path
end
raise "File not found. '#{file_path}'" unless File.file? file_path

bkup = @current_file
@current_file = file_path
begin
log "DSL: include_file(#{file_path})"
instance_eval File.read(file_path), file_path
ensure
@current_file = bkup
end
end

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

この実装には、以下の2つの改善の余地があります:\n\n1. 引数のバリデーション(防御的プログラミング):\n file_path が nil または空文字の場合、File.expand_path で TypeError が発生します。事前に分かりやすいエラーメッセージを伴うガード節を追加することをおすすめします。\n\n2. 循環インクルードの検知:\n 設定ファイル間で相互に include_file を呼び出し合う(循環参照)と、無限再帰により SystemStackError(スタックオーバーフロー)が発生します。現在ロード中のファイルを追跡し、循環参照を検知した時点で明示的な例外を発生させることで、デバッグが容易になり堅牢性が向上します。

    def include_file(file_path)\n      raise "DSL: file_path is required for include_file" if file_path.nil? || file_path.empty?\n\n      file_path = if @current_file\n                    File.expand_path file_path, File.dirname(@current_file)\n                  else\n                    File.expand_path file_path\n                  end\n      raise "File not found. '#{file_path}'" unless File.file? file_path\n\n      @loading_files ||= []\n      if @loading_files.include?(file_path)\n        raise "Circular dependency detected: #{@loading_files.join(' -> ')} -> #{file_path}"\n      end\n\n      @loading_files << file_path\n      bkup = @current_file\n      @current_file = file_path\n      begin\n        log "DSL: include_file(#{file_path})"\n        instance_eval File.read(file_path), file_path\n      ensure\n        @current_file = bkup\n        @loading_files.delete(file_path)\n      end\n    end

@haruyan-hopemucci haruyan-hopemucci left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

masterへのPR出し直しありがとうございました!
レビューOKです!

@haruyan-hopemucci
haruyan-hopemucci merged commit 5446744 into master Jul 9, 2026
1 check passed
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.

3 participants