-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Auto-add new apps during sync #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
fix: Support new Dify API cookie-based authentication
Previously, GetAppList only fetched the first page of results (default 20 apps). When has_more was true, subsequent pages were not fetched, causing apps beyond the first page to be incorrectly identified as "deleted remotely" and triggering unwanted deletion of local DSL files. This fix: - Loops through all pages when has_more is true - Adds page parameter to API requests - Accumulates results from all pages before returning - Adds comprehensive tests for pagination scenarios Fixes the issue where all DSL files were deleted when syncing with Dify instances containing more than 20 apps. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
fix: GetAppList APIのページネーション未対応によるDSLファイル全削除問題を修正
SyncAll() 実行時に、Dify から取得したアプリ一覧と app_map.json を比較し、 新規アプリを自動的に検出して追加するようにした。 これにより、Dify に新しいアプリが追加された場合でも difync init を 再実行する必要がなくなり、通常の sync で新規アプリが取り込まれる。 変更内容: - SyncStats に Added フィールドを追加 - generateUniqueFilename() ヘルパー関数を追加(重複回避ロジック共通化) - SyncAll() に新規アプリ自動追加ロジックを実装 - TestSyncAllWithNewApps テストを追加 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
yoshikouki
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
レビュー結果
新規アプリの自動追加機能は非常に有用です。generateUniqueFilename()の共通化も良いリファクタリングです。ただし、エラーハンドリングに重大な問題があるため修正が必要です。
🔴 重大な問題
1. DSL取得失敗時のapp_map.json不整合
問題箇所: internal/syncer/syncer.go:380-420
現在の実装では、DSL取得/書き込みに失敗してもnewAppsへの追加が既に済んでいるため、app_map.jsonにファイルが存在しないアプリが記録されます。
// ❌ 現在の実装
newApp := AppMapping{
Filename: filename,
AppID: appID,
}
newApps = append(newApps, newApp) // ダウンロード前に追加
// Download DSL for new app
if !s.config.DryRun {
dsl, err := s.client.GetDSL(appID)
if err != nil {
stats.Errors++
continue // ここでスキップされるが、newAppsには既に追加済み
}
}影響:
- 次回のsync時に
os.Statエラーが発生し続ける - ユーザーは手動で
app_map.jsonを編集する必要がある
修正案:
// ✅ 修正後
newApp := AppMapping{
Filename: filename,
AppID: appID,
}
// Download DSL for new app
localPath := filepath.Join(s.config.DSLDirectory, filename)
if !s.config.DryRun {
dsl, err := s.client.GetDSL(appID)
if err != nil {
fmt.Printf("Warning: Failed to download DSL for new app %s: %v\n", appInfo.Name, err)
stats.Errors++
continue // 成功前にスキップ
}
if err := os.WriteFile(localPath, dsl, 0644); err != nil {
fmt.Printf("Warning: Failed to write DSL file for new app %s: %v\n", appInfo.Name, err)
stats.Errors++
continue // 成功前にスキップ
}
if s.config.Verbose {
fmt.Printf("Downloaded DSL for new app %s to %s\n", appInfo.Name, localPath)
}
}
// ✅ 成功した場合のみ追加
newApps = append(newApps, newApp)
stats.Added++
stats.Total++🟡 軽微な改善提案
2. Total apps統計の意味が不明確
stats.Totalは削除分を差し引かず、新規追加分のみ加算されるため、削除と追加が同時に発生するケースでリモートの総数と乖離します。
該当箇所: internal/syncer/syncer.go:418, cmd/difync/main.go:124
提案:
- 表示文言を「Processing apps」などに変更する
- または、削除分を差し引いて実際のリモート総数と一致させる
3. テストカバレッジの追加
TestSyncAllWithNewAppsは正常系のみです。以下のエラーケースも追加すると安心です:
- 新規アプリのDSL取得が失敗するケース
- ファイル書き込みが失敗するケース
- 複数の新規アプリで一部が失敗するケース
✅ 良い点
- ファイル名生成ロジックの共通化(
generateUniqueFilename()) Addedカウントで新規追加数が可視化される- 詳細なログ出力(Verboseモード)
- 新機能に対するテストの追加
質問
- 新規DSL取得に失敗した場合でもアプリマップに残す意図はありますか?
- 意図がない場合は、上記の修正案のように失敗時はマップ更新対象から外す実装が必要です
Move newApps append to after DSL download and file write succeed, preventing app_map.json inconsistency when errors occur. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
確認ありがとうございます、修正しました! 🔴 重大な問題についてご指摘の通り、DSL取得/書き込み失敗時に 修正内容:
🟡 軽微な改善提案について
こちらは別PRで対応を検討します。
以下のエラーケーステストを追加しました:
✅ 質問への回答
意図はありません。今回の修正で、失敗時はマップ更新対象から外すようにしました。 |
- TestSyncAllWithNewAppsDSLFetchFails: DSL fetch failure case - TestSyncAllWithNewAppsPartialFailure: Multiple new apps with partial failure Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
d59e0a7 to
551de8a
Compare
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
574efd0 to
47007be
Compare
yoshikouki
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
レビュー結果
sync時に新規アプリを自動追加する機能、よく実装されています!
✅ 良い点
- 堅牢なエラーハンドリング: DSL取得失敗時に
app_map.jsonの整合性を保護 - テストが充実: 正常系・異常系・部分的失敗のテストを網羅
- リファクタリング:
generateUniqueFilename()の共通化でコード重複を削減 - PR説明が明確: 背景・課題・期待動作が分かりやすい
📝 軽微な指摘
インラインコメントで4点指摘しました。いずれも軽微なもので、マージ後のフォローアップで対応可能です。
- ページネーションの無限ループリスク (Medium)
- mapの走査順序が非決定的 (Low)
- DryRunモードでの新規アプリ検出 (Low)
- Debug出力の制御 (Nit)
LGTM 👍
| page := 1 | ||
|
|
||
| fmt.Printf("Debug - Using app list URL: %s\n", url) | ||
| for { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Medium] ページネーションの無限ループリスク
APIが常に has_more: true を返すバグがある場合、無限ループになる可能性があります。
// 提案: 最大ページ数の制限を追加
const maxPages = 1000
for page := 1; page <= maxPages; page++ {
// ...
if !hasMore || page >= maxPages {
break
}
}現時点では軽微なリスクですが、将来的に対応を検討することを推奨します。
| } | ||
|
|
||
| newApps := []AppMapping{} | ||
| for appID, appInfo := range remoteApps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Low] mapの走査順序が非決定的
Goのmapは走査順序が保証されないため、新規アプリの追加順序が実行ごとに変わる可能性があります。app_map.json の差分が毎回変わったり、テストの安定性に影響する可能性があります。
// 提案: ソートしてから処理
var newAppIDs []string
for appID := range remoteApps {
if !existingAppIDs[appID] {
newAppIDs = append(newAppIDs, appID)
}
}
sort.Strings(newAppIDs)
for _, appID := range newAppIDs {
appInfo := remoteApps[appID]
// ...
}|
|
||
| // Download DSL for new app | ||
| localPath := filepath.Join(s.config.DSLDirectory, filename) | ||
| if !s.config.DryRun { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Low] DryRunモードで新規アプリの検出が表示されない
現在の実装では、DryRunモードの場合、新規アプリの検出・追加処理がスキップされます。ユーザーが「何が追加されるか」を事前確認したい場合に不便です。
// 提案: DryRunでもログ出力と統計カウントは行う
if s.config.DryRun {
fmt.Printf("Dry run: Would download DSL for new app %s to %s\n", appInfo.Name, localPath)
newApps = append(newApps, newApp)
stats.Added++
stats.Total++
continue
}| // generateUniqueFilename generates a unique filename for an app, avoiding duplicates | ||
| func (s *DefaultSyncer) generateUniqueFilename(appName string, usedFilenames map[string]bool) string { | ||
| safeName := s.sanitizeFilename(appName) | ||
| fmt.Printf("Debug - sanitizeFilename(%q) = %q\n", appName, safeName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Nit] Debug出力が常に出力される
Verbose フラグに関係なく常にDebug出力されます。本番環境でログが冗長になる可能性があります。
// 提案: Verboseフラグで制御
if s.config.Verbose {
fmt.Printf("Debug - sanitizeFilename(%q) = %q\n", appName, safeName)
}または、別途 Debug フラグを追加する方法もあります。
Summary
difync initを再実行せずに、通常の sync で新規アプリが取り込まれるようになるgenerateUniqueFilename()ヘルパー関数)背景・課題
現在の difync(sync 動作)は app_map.json に登録済みのアプリのみを同期する。Dify に新しいアプリが追加されても app_map.json
には追加されないため、新規アプリを取り込むには
difync initの再実行が必要だった。しかし
difync initを複数回実行すると、既存ファイルとの重複回避でサフィックス(_1, _2)が付き、ファイルが増殖する問題があった。期待する動作
$ difync
Starting sync...
Sync Summary:
Total apps: 155
Downloads: 2
Added: 2 ← 新規追加
No action (in sync): 151
Errors: 0
Test plan
TestSyncAllWithNewAppsテストを追加して新機能を検証🤖 Generated with Claude Code