fix: replace panic with error handling in template loader#6823
fix: replace panic with error handling in template loader#6823areebahmeddd wants to merge 1 commit intoprojectdiscovery:devfrom
Conversation
WalkthroughThe changes add proper error handling to template loading functions in the catalog loader package. LoadTemplates and LoadTemplatesWithTags now return errors instead of panicking when dialers are not found. Callers have been updated throughout the codebase to handle the new error returns. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/catalog/loader/loader_bench_test.go`:
- Around line 73-75: The benchmark loop is ignoring errors returned by
store.LoadTemplates which can hide failures; update each b.Loop() block (where
store.LoadTemplates([]string{config.DefaultConfig.TemplatesDirectory}) is
called) to capture the returned error and fail the benchmark on error (e.g., if
err := store.LoadTemplates(...); err != nil { b.Fatalf("LoadTemplates failed:
%v", err) }) so the benchmark stops and surfaces the real failure instead of
measuring an error path.
In `@pkg/catalog/loader/loader.go`:
- Around line 334-339: The Load method currently ignores the error from
LoadTemplates which can hide failures; update Store.Load (the method receiver
Store.Load) to return an error, call store.LoadTemplates(store.finalTemplates)
and capture its error, set store.templates only on success, call
store.LoadWorkflows(store.finalWorkflows) afterwards, and propagate any error
returned (i.e., return the error instead of discarding it) so callers can handle
failures instead of getting an empty store.templates; adjust callers of
Store.Load accordingly to handle the new error return.
|
gentle ping! @dwisiswant0 @dogancanbakir @Mzack9999 |
|
gentle ping.. @DhiyaneshGeek |
|
Kindly refrain from tagging everyone in the comments , the team will review short Thanks |
|
gentle ping! it’s been about a month. i’ve noticed a lot of prs coming in for this issue. any chance this could get a review soon? (p.s the failing tests are flaky in nature) |
|
i noticed that a pr (#6825) raised after mine, with the same changes, was reviewed and merged first. this reflects poorly on the open-source spirit at projectdiscovery and feels borderline biased scam. while many use ai (including myself), i took the time to understand the context, run tests, and follow pr standards. yet, the team showed significant negligence. |
Proposed changes
Fixes #6674
Replaced a panic with proper error handling in the template loader when
GetDialersWithId()returns nil, since this can occur in non-scanning paths where dialers aren’t initialized.Proof
make test- all unit tests passmake vet- passes with no issuesmake build- compiles successfullyChecklist
/claim #6674
Summary by CodeRabbit