-
Notifications
You must be signed in to change notification settings - Fork 4
feat: Added changes to support crud for graphql-cost-decoration custo… #428
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?
Changes from 7 commits
9b54664
7ce860b
d878b9c
575e162
9b87b55
9707bf5
e1ce1ae
5aa63f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -644,19 +644,22 @@ func getProxyConfiguration(ctx context.Context, group *errgroup.Group, | |
| } | ||
|
|
||
| if !skipCustomEntities && len(config.CustomEntityTypes) > 0 { | ||
| // Get custom entities with types given in config.CustomEntityTypes. | ||
| // Register all entity types first (sequentially) to avoid data race on the registry map. | ||
| // The registry is not thread-safe, so we must complete all registrations before | ||
| // starting concurrent fetch operations that call Lookup(). | ||
| for _, entityType := range config.CustomEntityTypes { | ||
| if err := tryRegisterEntityType(client, custom.Type(entityType)); err != nil { | ||
| group.Go(func() error { | ||
| return fmt.Errorf("custom entity %s: %w", entityType, err) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the purpose of this goroutine?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function |
||
| }) | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| customEntityLock := sync.Mutex{} | ||
| for _, entityType := range config.CustomEntityTypes { | ||
| t := entityType | ||
| group.Go(func() error { | ||
| // Register entity type. | ||
| // Because client writes an unprotected map to register entity types, we need to use mutex to protect it. | ||
| customEntityLock.Lock() | ||
| err := tryRegisterEntityType(client, custom.Type(t)) | ||
| customEntityLock.Unlock() | ||
| if err != nil { | ||
| return fmt.Errorf("custom entity %s: %w", t, err) | ||
| } | ||
| // Fetch all entities with the given type. | ||
| entities, err := GetAllCustomEntitiesWithType(ctx, client, t) | ||
| if err != nil { | ||
|
|
@@ -676,9 +679,19 @@ func tryRegisterEntityType(client *kong.Client, typ custom.Type) error { | |
| if client.Lookup(typ) != nil { | ||
| return nil | ||
| } | ||
|
|
||
| // Determine the CRUD path based on entity type | ||
| crudPath := "/" + string(typ) | ||
|
|
||
| // Special case: Kong exposes this API at /graphql-rate-limiting-advanced/costs, | ||
| // not at /graphql_ratelimiting_cost_decorations (the entity type name). | ||
| if typ == "graphql_ratelimiting_cost_decorations" { | ||
| crudPath = "/graphql-rate-limiting-advanced/costs" | ||
| } | ||
|
|
||
| return client.Register(typ, &custom.EntityCRUDDefinition{ | ||
| Name: typ, | ||
| CRUDPath: "/" + string(typ), | ||
| CRUDPath: crudPath, | ||
| PrimaryKey: "id", | ||
| }) | ||
| } | ||
|
|
||
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.
We were using a mutex for that. What was the issue here?
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.
So basically, mutex only protected the Register() (write) operation, but not the subsequent Lookup() (read) that happens inside GetAllCustomEntitiesWithType.
I have got race conditions on one test where we were adding multiple decorations simultaneously, there i have faced this issue and then added this fix.