-
Notifications
You must be signed in to change notification settings - Fork 21
refactor: refactoring for principal cache #253
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
Conversation
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.
Pull request overview
This PR refactors the principal caching system by introducing a new OrganizationPrincipalCache structure to manage user and team principals. The refactoring includes removing the embedded ORM base from OrganizationPrincipal, adding context propagation throughout the entity provider system, and enhancing the elastic search infrastructure with new utility methods.
Key changes:
- Introduced
OrganizationPrincipalCacheas a wrapper aroundOrganizationPrincipalwith versioning support - Updated entity provider interfaces to accept context parameters for better request tracing
- Added utility methods for handling Elasticsearch response totals and field extraction
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
core/security/principal.go |
Refactored OrganizationPrincipal to remove ORM base, added new OrganizationPrincipalCache struct with init-based schema registration |
modules/security/rbac/principal.go |
Updated search function to use new principal structure and NewGeneralTotal utility, reformatted struct initialization |
modules/security/rbac/init.go |
Removed old OrganizationPrincipal schema registration (moved to principal.go) |
modules/security/rbac/entity.go |
Added context parameter to GenEntityInfo and GenEntityLabel methods for request tracing |
core/entity_card/api.go |
Updated EntityProvider interface to propagate context through entity methods |
core/orm/orm.go |
Added system fields constants, enhanced debug mode with panic for field lookup errors |
core/orm/context.go |
Reorganized imports (moved reflect after blank line) |
core/elastic/generic_document.go |
Extracted SearchHits struct, added NewGeneralTotal helper and GetTotal method for parsing various total formats |
core/elastic/generic_document_test.go |
Added comprehensive tests for GetTotal method covering different response formats |
core/elastic/index.go |
Added TryGetStringFieldFromSource method to attempt multiple fields with fallback |
core/elastic/index_test.go |
Added tests for TryGetStringFieldFromSource covering various scenarios |
core/elastic/partition.go |
Changed from fmt.Errorf to errors.New for simple error wrapping |
modules/elastic/schema.go |
Consolidated log messages for mapping override errors, reorganized imports |
core/global/register.go |
Added InitialDelay field to BackgroundTask, improved timing logic with >= comparison |
modules/queue/disk_queue/module.go |
Fixed formatting of else clause |
core/security/service_registry.go |
Removed commented-out code |
Comments suppressed due to low confidence (1)
core/global/register.go:265
- Potential race condition: The fields
InitialDelayandlastRunningof BackgroundTask are being read and written without synchronization. While sync.Map protects map operations, it doesn't protect concurrent access to the values stored in the map. Consider adding a mutex to BackgroundTask or using atomic operations for these fields.
v := value.(*BackgroundTask)
// Default InitialDelay to Interval if not set
if v.InitialDelay == 0 {
v.InitialDelay = v.Interval
}
// If never run before, use InitialDelay
if v.lastRunning.IsZero() {
if time.Since(env.GetStartTime()) < v.InitialDelay {
return true
}
}
if time.Since(v.lastRunning) >= v.Interval {
log.Tracef("start run background job:%v, interval:%v", key, v.Interval)
ctx, cancel := context.WithTimeout(context.Background(), v.Interval)
defer cancel()
err := FuncWithTimeout(ctx, v.Func)
if err != nil {
log.Error(fmt.Sprintf("error on running background job: %v, %v", key, err))
}
v.lastRunning = time.Now()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| provider := mustGetProviders(t) | ||
| info := provider.GenEntityInfo(t, id) | ||
| info := provider.GenEntityInfo(req.Context(),t, id) |
Copilot
AI
Jan 1, 2026
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.
Missing space after comma in function parameter list. The parameter list should have a space between 'context.Context,' and 't string'.
| info := provider.GenEntityInfo(req.Context(),t, id) | |
| info := provider.GenEntityInfo(req.Context(), t, id) |
| Name string `json:"name,omitempty" elastic_mapping:"name:{type:keyword,copy_to:combined_fulltext}"` | ||
| ID string `json:"id,omitempty" elastic_mapping:"id:{type:keyword}"` | ||
| Type string `json:"type,omitempty" elastic_mapping:"type:{type:keyword}"` // "type": "user", // or "team" | ||
| Name string `json:"name,omitempty" elastic_mapping:"name:{type:text,copy_to:combined_fulltext,fields:{keyword: {type: keyword}, pinyin: {type: text, analyzer: pinyin_analyzer}}}"` |
Copilot
AI
Jan 1, 2026
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.
Inconsistent indentation in struct field definition. The Name field has extra spaces before 'string' that don't align with the other fields (ID, Type, Description, Avatar).
| Name string `json:"name,omitempty" elastic_mapping:"name:{type:text,copy_to:combined_fulltext,fields:{keyword: {type: keyword}, pinyin: {type: text, analyzer: pinyin_analyzer}}}"` | |
| Name string `json:"name,omitempty" elastic_mapping:"name:{type:text,copy_to:combined_fulltext,fields:{keyword: {type: keyword}, pinyin: {type: text, analyzer: pinyin_analyzer}}}"` |
| } else { | ||
| log.Warnf("override existing mapping with error: %v, %v", err, string(res)) | ||
| log.Warnf("index pattern: %v*, %v", indexName, json) | ||
| log.Warnf("index pattern: %v*, %v, override existing mapping with error: %v, %v", indexName, json,err, string(res)) |
Copilot
AI
Jan 1, 2026
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.
Missing space after comma in log format arguments. There should be a space between 'json,' and 'err'.
| log.Warnf("index pattern: %v*, %v, override existing mapping with error: %v, %v", indexName, json,err, string(res)) | |
| log.Warnf("index pattern: %v*, %v, override existing mapping with error: %v, %v", indexName, json, err, string(res)) |
| Name: v.Name, | ||
| ID: v.ID, | ||
| Type: security.PrincipalTypeUser, | ||
| Description: v.Email, |
Copilot
AI
Jan 1, 2026
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.
Inconsistent field alignment in struct literal. The fields have different numbers of spaces after the colon, breaking visual alignment. Consider either removing all extra spaces or aligning all values consistently.
| Name: v.Name, | |
| ID: v.ID, | |
| Type: security.PrincipalTypeUser, | |
| Description: v.Email, | |
| Name: v.Name, | |
| ID: v.ID, | |
| Type: security.PrincipalTypeUser, | |
| Description: v.Email, |
| } | ||
|
|
||
| func (this *UserEntityProvider) GenEntityInfo(t string, id string) *entity_card.EntityInfo { | ||
| func (this *UserEntityProvider) GenEntityInfo(ctx context.Context,t string, id string) *entity_card.EntityInfo { |
Copilot
AI
Jan 1, 2026
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.
Missing space after comma in function parameter list. The parameter list should have a space between 'context.Context,' and 't string'.
| } | ||
|
|
||
| func (this *UserEntityProvider) GenEntityLabel(t string, ids []string) []entity_card.EntityLabel { | ||
| func (this *UserEntityProvider) GenEntityLabel(ctx1 context.Context,t string, ids []string) []entity_card.EntityLabel { |
Copilot
AI
Jan 1, 2026
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.
Missing space after comma in function parameter list. The parameter list should have a space between 'context.Context,' and 't string'.
| } | ||
| if f.Kind() != reflect.String { | ||
| if global.Env().IsDebug { | ||
| panic(errors.Errorf("field %s not found in struct", fieldName)) |
Copilot
AI
Jan 1, 2026
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.
Incorrect error message in panic. The panic should indicate that the field is not a string, not that it's not found. The error message should match the log message on line 311: "field %s is not a string in struct".
| panic(errors.Errorf("field %s not found in struct", fieldName)) | |
| panic(errors.Errorf("field %s is not a string in struct", fieldName)) |
| security.RegisterAuthorizationProvider(security.DefaultNativeAuthBackend, &provider) | ||
|
|
||
| orm.MustRegisterSchemaWithIndexName(&security.UserAccount{}, "app-users") | ||
| orm.MustRegisterSchemaWithIndexName(&security.UserRole{}, "app-roles") |
Copilot
AI
Jan 1, 2026
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.
This change removes the schema registration for OrganizationPrincipal with index 'org-principals' and replaces it with OrganizationPrincipalCache with index 'principals-cache'. This is a breaking change that affects data persistence. Consider documenting this breaking change and providing a migration path for any existing data in the 'org-principals' index.
What does this PR do
Rationale for this change
Standards checklist