Skip to content

dont load templates with the same ID #6465

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.

Sign up for GitHub

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

Merged
Mzack9999 merged 3 commits into from
Sep 12, 2025
Merged

dont load templates with the same ID #6465

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4916cf3
dont load templates with the same ID
dogancanbakir Sep 10, 2025
b05359b
using synclockmap
Mzack9999 Sep 10, 2025
608159b
lint
Mzack9999 Sep 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions pkg/catalog/loader/loader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"os"
"sort"
"strings"
"sync"

"github.com/logrusorgru/aurora"
"github.com/pkg/errors"
Expand Down Expand Up @@ -315,6 +316,8 @@ func (store *Store) LoadTemplatesOnlyMetadata() error {
}
templatesCache := parserItem.Cache()

loadedTemplateIDs := make(map[string]bool)

for templatePath := range validPaths {
template, _, _ := templatesCache.Has(templatePath)

Expand All @@ -339,6 +342,12 @@ func (store *Store) LoadTemplatesOnlyMetadata() error {
}

if template != nil {
if loadedTemplateIDs[template.ID] {
store.logger.Debug().Msgf("Skipping duplicate template ID '%s' from path '%s'", template.ID, templatePath)
continue
}

loadedTemplateIDs[template.ID] = true
template.Path = templatePath
Comment on lines +345 to 351

@coderabbitai coderabbitai Bot Sep 10, 2025

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.

⚠️ Potential issue

Fix errcheck failure and handle Set error.

golangci-lint fails because the error return of SyncLockMap.Set is ignored. Set can return ErrReadOnly; handle it and keep behavior unchanged. (pkg.go.dev)

Apply:

-            if loadedTemplateIDs.Has(template.ID) {
+            if loadedTemplateIDs.Has(template.ID) {
                 store.logger.Debug().Msgf("Skipping duplicate template ID '%s' from path '%s'", template.ID, templatePath)
                 continue
             }
-
-            loadedTemplateIDs.Set(template.ID, struct{}{})
+            if err := loadedTemplateIDs.Set(template.ID, struct{}{}); err != nil {
+                store.logger.Debug().Msgf("could not record template ID '%s' for path '%s': %v", template.ID, templatePath, err)
+            }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if loadedTemplateIDs.Has(template.ID) {
store.logger.Debug().Msgf("Skipping duplicate template ID '%s' from path '%s'", template.ID, templatePath)
continue
}
loadedTemplateIDs.Set(template.ID, struct{}{})
template.Path = templatePath
if loadedTemplateIDs.Has(template.ID) {
store.logger.Debug().Msgf("Skipping duplicate template ID '%s' from path '%s'", template.ID, templatePath)
continue
}
if err := loadedTemplateIDs.Set(template.ID, struct{}{}); err != nil {
store.logger.Debug().Msgf("could not record template ID '%s' for path '%s': %v", template.ID, templatePath, err)
}
template.Path = templatePath
🧰 Tools
🪛 GitHub Check: Lint

[failure] 350-350:
Error return value of loadedTemplateIDs.Set is not checked (errcheck)

🪛 GitHub Actions: 🔨 Tests

[error] 350-350: golangci-lint: Error return value of loadedTemplateIDs.Set is not checked (errcheck)

🤖 Prompt for AI Agents 
In pkg/catalog/loader/loader.go around lines 345 to 351, handle the error
returned by loadedTemplateIDs.Set to fix the errcheck failure: capture the
returned error, ignore it when it equals maps.ErrReadOnly (to keep current
behavior), and for any other non-nil error log it
(store.logger.Debug().Err(err).Msgf(...)) and skip/continue as appropriate;
ensure you import the errors package and reference maps.ErrReadOnly when
comparing.

store.templates = append(store.templates, template)
}
Expand Down Expand Up @@ -492,8 +501,20 @@ func (store *Store) LoadTemplatesWithTags(templatesList, tags []string) []*templ
templatePathMap := store.pathFilter.Match(includedTemplates)

loadedTemplates := sliceutil.NewSyncSlice[*templates.Template]()
loadedTemplateIDs := make(map[string]bool)
var loadedTemplateIDsMutex sync.Mutex

Comment thread
Mzack9999 marked this conversation as resolved.
loadTemplate := func(tmpl *templates.Template) {
loadedTemplateIDsMutex.Lock()
if loadedTemplateIDs[tmpl.ID] {
loadedTemplateIDsMutex.Unlock()
store.logger.Debug().Msgf("Skipping duplicate template ID '%s' from path '%s'", tmpl.ID, tmpl.Path)
return
}

loadedTemplateIDs[tmpl.ID] = true
loadedTemplateIDsMutex.Unlock()

Comment on lines +507 to +513

@coderabbitai coderabbitai Bot Sep 10, 2025

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.

🛠️ Refactor suggestion

Make the duplicate-ID guard atomic; remove errcheck issue.

Switch to LoadOrStore so only the first goroutine proceeds; others return early. This also removes the errcheck warning on Set.

Apply:

-        if loadedTemplateIDs.Has(tmpl.ID) {
-            store.logger.Debug().Msgf("Skipping duplicate template ID '%s' from path '%s'", tmpl.ID, tmpl.Path)
-            return
-        }
-
-        loadedTemplateIDs.Set(tmpl.ID, struct{}{})
+        if _, loaded := loadedTemplateIDs.LoadOrStore(tmpl.ID, struct{}{}); loaded {
+            store.logger.Debug().Msgf("Skipping duplicate template ID '%s' from path '%s'", tmpl.ID, tmpl.Path)
+            return
+        }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if loadedTemplateIDs.Has(tmpl.ID) {
store.logger.Debug().Msgf("Skipping duplicate template ID '%s' from path '%s'", tmpl.ID, tmpl.Path)
return
}
loadedTemplateIDs.Set(tmpl.ID, struct{}{})
// Only the first goroutine storing this tmpl.ID will proceed; others skip.
if _, loaded := loadedTemplateIDs.LoadOrStore(tmpl.ID, struct{}{}); loaded {
store.logger.Debug().Msgf("Skipping duplicate template ID '%s' from path '%s'", tmpl.ID, tmpl.Path)
return
}
🧰 Tools
🪛 GitHub Check: Lint

[failure] 512-512:
Error return value of loadedTemplateIDs.Set is not checked (errcheck)

🤖 Prompt for AI Agents 
In pkg/catalog/loader/loader.go around lines 507 to 513, replace the non-atomic
Has/Set pair with a single LoadOrStore call so only the first goroutine for a
given tmpl.ID proceeds and others return early; specifically call
loadedTemplateIDs.LoadOrStore(tmpl.ID, struct{}{}) and if it reports the ID was
already present, log the duplicate and return, otherwise continue (no separate
Set call, which removes the errcheck warning).

loadedTemplates.Append(tmpl)
// increment signed/unsigned counters
if tmpl.Verified {
Expand Down
Loading