Moving oauth & flow consumable interfaces to pkg#2906
Draft
anushasunkada wants to merge 4 commits into
Draft
Conversation
anushasunkada
requested review from
ThaminduDilshan,
ThumulaPerera,
darshanasbg and
thiva-k
as code owners
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a public embedding API for OAuth/flow runtime, provider bridges, host-backed stores, executor init refactor, OAuth engine initializer, server runtime wiring via enginebridge, tests validating route registration and stores, and docs. ChangesEmbedded Engine and Runtime Wiring
Sequence Diagram(s)sequenceDiagram
participant HostApp
participant ThunderEngine
participant EngineBridge
participant FlowExec
participant OAuthEngine
HostApp->>ThunderEngine: Initialize(mux)
ThunderEngine->>EngineBridge: Initialize(config, mux)
EngineBridge->>FlowExec: Initialize(opts)
EngineBridge->>OAuthEngine: InitializeEngine(deps)
OAuthEngine-->>HostApp: /.well-known, /oauth2, PAR, authz
FlowExec-->>HostApp: /flow/meta, /flow/execute
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (3)
backend/internal/enginebridge/bridge_client.go (1)
101-105: ⚡ Quick winReturn a populated certificate error instead of an empty one.
At Line 104, returning
&inboundclient.CertOperationError{}hides the actual failure reason. Please return a populated not-implemented error payload so callers can log/branch correctly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/enginebridge/bridge_client.go` around lines 101 - 105, The GetCertificate method currently returns an empty &inboundclient.CertOperationError{} which hides the failure reason; update clientBridge.GetCertificate to return a populated CertOperationError indicating "not implemented" (set available fields such as Message or Error and a Code or Type field to "NotImplemented" or similar) so callers can log/branch correctly and see the intent that GetCertificate is unimplemented.backend/pkg/thunderidengine/engine.go (1)
30-32: ⚡ Quick winMake bootstrap registration single-assignment.
RegisterBootstrapcan currently be called multiple times (or withnil), which can silently replace/disable engine wiring process-wide. Consider guarding registration to the first non-nil value.Proposed change
package thunderidengine -import "net/http" +import ( + "net/http" + "sync" +) @@ var registeredBootstrap bootstrapFunc +var registerBootstrapOnce sync.Once @@ func RegisterBootstrap(fn bootstrapFunc) { - registeredBootstrap = fn + if fn == nil { + return + } + registerBootstrapOnce.Do(func() { + registeredBootstrap = fn + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/pkg/thunderidengine/engine.go` around lines 30 - 32, RegisterBootstrap currently overwrites registeredBootstrap on every call; change it to perform a single-assignment of the first non-nil bootstrapFunc and ignore subsequent calls (and nil args) to avoid accidental replacement. Implement this by guarding the assignment with a check that registeredBootstrap is nil and fn != nil (or use a sync.Once named bootstrapOnce and call bootstrapOnce.Do to set registeredBootstrap) so the first non-nil fn is preserved and further RegisterBootstrap calls are no-ops; keep the symbol names RegisterBootstrap, registeredBootstrap, bootstrapFunc and make the behavior concurrency-safe.backend/tests/integration/providers.go (1)
131-145: ⚡ Quick winUse unique PAR keys in the in-memory runtime store.
Storecurrently keys PAR entries asurn:par:<clientID>, so subsequent requests from the same client overwrite prior ones. Generating unique keys better matches real PAR behavior and improves integration-test fidelity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/integration/providers.go` around lines 131 - 145, The Store method in memoryRuntimeStore currently uses a non-unique key "urn:par:"+parRequest.ClientID which causes overwrites; update memoryRuntimeStore.Store to generate a unique key (e.g., UUID or timestamp+random) such as "urn:par:<unique-id>", store the parRequest in m.par under that generated key, and return that key; keep newMemoryRuntimeStore and the m.par map unchanged but ensure the generated key format is used wherever callers of Store expect the returned key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ARCHITECTURE.md`:
- Line 31: Rename the markdown heading text "Embeddable engine
(`pkg/thunderidengine`)" to a Vale-friendly variant (for example "Embeddable
Engine (`pkg/thunderidengine`)" or "Embeddable runtime (`pkg/thunderidengine`)")
to resolve the lint spelling warning; update the exact heading string in
ARCHITECTURE.md so the new title replaces the original "Embeddable engine
(`pkg/thunderidengine`)" occurrence and ensure the package reference
`pkg/thunderidengine` remains intact.
In `@backend/internal/enginebridge/bootstrap_integration_test.go`:
- Around line 41-50: The test currently calls
Initialize(thunderidengine.EngineConfig{ConfigPath: serverHome}, mux) and skips
on any error which hides real regressions; change the test to require no error
from Initialize (use require.NoError or equivalent on the Initialize call) so
initialization failures make the test fail, then perform the HTTP request
against mux and assert a successful metadata response (assert rec.Code ==
http.StatusOK and check content-type is application/json or that the response
body contains expected OpenID metadata keys like "issuer" or
"authorization_endpoint") instead of only asserting non-404; update assertions
around Initialize, mux.ServeHTTP, and the recorder to enforce success.
In `@backend/internal/enginebridge/bootstrap.go`:
- Around line 64-67: The code creates a private http.NewServeMux when the
incoming mux is nil, which hides any registered routes; change Initialize to
fail fast instead: check the incoming mux parameter and if it's nil return an
error (or the existing error-handling path) instead of creating a private
routeMux, so callers know they passed nil; update logic referencing routeMux and
mux in Initialize to remove the private ServeMux fallback and surface an
explicit error when mux == nil.
In `@backend/internal/flow/flowexec/host_runtime_store.go`:
- Around line 32-60: NewContextStoreFromRuntime currently returns a
hostContextStore that dereferences s.host in methods (StoreFlowContext,
GetFlowContext, UpdateFlowContext, DeleteFlowContext) and will panic if the
provided RuntimeStore is nil; change the constructor to validate the host
argument and return an error or a no-op implementation when host is nil, or
store an explicit nil-check flag, and update each method to check s.host != nil
and return a clear error (e.g., fmt.Errorf("nil runtime store") or a
package-level sentinel error) instead of panicking; ensure GetFlowContext still
returns (nil, nil) when appropriate and keep conversions
(toPublicFlowContext/fromPublicFlowContext) unchanged.
In `@backend/internal/flow/flowexec/init.go`:
- Around line 75-86: The custom ContextStore path in resolveFlowContextStore
currently still initializes a runtime DB transactioner for non-Redis runtimes;
change resolveFlowContextStore so that whenever opts != nil && opts.ContextStore
!= nil it immediately returns opts.ContextStore with
transaction.NewNoOpTransactioner() (do not call dbprovider.GetDBProvider() or
GetRuntimeDBTransactioner()), so a provided ContextStore does not force
initialization of the runtime DB transactioner.
In `@backend/internal/flow/mgt/runtime_flow_service.go`:
- Around line 170-186: hostFlowToComplete should validate def.FlowType before
constructing CompleteFlowDefinition: convert the raw value with
common.FlowType(def.FlowType) and reject unknown/unsupported values (e.g.,
zero/unknown enum) rather than accepting them silently. Update
hostFlowToComplete to check the mapped flowType against the known/allowed values
(use a small switch, a common.IsValidFlowType helper, or explicit list of
common.FlowType constants) and return an appropriate service error (e.g.,
serviceerror.InvalidArgument or a new ErrorInvalidFlowType) when invalid; only
then populate CompleteFlowDefinition.FlowType with the validated
common.FlowType.
In `@backend/internal/oauth/engine_init.go`:
- Around line 84-87: The local creation of a new http.ServeMux when deps.Mux is
nil causes an orphaned mux; instead make deps.Mux required by replacing the nil
branch with an explicit error return (do not call http.NewServeMux()), e.g.
check deps.Mux and if nil return an error from the enclosing init function (the
function that contains "mux := deps.Mux"), update callers to handle the error,
and remove any code that registers routes on the locally-created mux; reference
the symbols deps.Mux and mux to locate and change the behavior.
In `@backend/internal/oauth/oauth2/authz/host_runtime_store.go`:
- Around line 176-180: The json.Unmarshal error for pub.ClaimsRequestJSON in
fromPublicAuthorizationCode is being discarded; capture the error (err :=
json.Unmarshal(pub.ClaimsRequestJSON, &claims)) and handle it: if
pub.ClaimsRequestJSON is non-empty and err != nil, record/log the error (e.g.,
using the package/logger available in this component) or set a clear sentinel on
the returned AuthorizationCode indicating invalid claims instead of silently
leaving claims nil; ensure you reference fromPublicAuthorizationCode,
pub.ClaimsRequestJSON, claims and the unmarshalling call so reviewers can find
and validate the change.
- Around line 154-156: The toPublicAuthorizationCode function currently discards
the error from json.Marshal(code.ClaimsRequest) which can drop ClaimsRequest
silently; update to handle the marshal error consistently with
marshalOAuthParameters by either (a) changing toPublicAuthorizationCode's
signature to return (thunderidengine.AuthorizationCode, error) and propagate the
json.Marshal error, or (b) keep the signature and log a warning via the same
logger used elsewhere and set ClaimsRequest to nil/empty on marshal failure;
locate toPublicAuthorizationCode and the json.Marshal call and implement one of
these fixes so the error is not silently ignored and behavior matches
marshalOAuthParameters.
In `@backend/internal/oauth/oauth2/authz/init.go`:
- Around line 70-85: Defer calling initializeAuthorizationStores until defaults
are actually needed: if opts is nil, call initializeAuthorizationStores() and
return defaultCode, defaultRequest, transactioner; if opts is non-nil and both
opts.CodeStore and opts.RequestStore are non-nil, return opts.CodeStore,
opts.RequestStore, nil (no transactioner initialization); otherwise call
initializeAuthorizationStores(), then substitute only the missing stores into
defaultCode/defaultRequest and return the resolved codeStore, requestStore and
transactioner. Use the existing identifiers initializeAuthorizationStores,
defaultCode, defaultRequest, transactioner, opts, CodeStore and RequestStore to
locate and update the logic.
In `@backend/internal/oauth/oauth2/par/init.go`:
- Around line 53-56: The code currently calls initializePARStore()
unconditionally which initializes the default PAR store even when an override is
provided; change the logic so initializePARStore() is only invoked when no
override is supplied: check opts and opts.Store first and use opts.Store if
present, otherwise call initializePARStore() to assign store (referencing the
initializePARStore function, the opts variable and opts.Store field and the
store local variable).
In `@backend/tests/integration/engine_test.go`:
- Around line 55-57: The test is skipping on any non-nil err after the
deployment file check which hides Initialize failures; replace the t.Skip call
(where err is checked) with a hard failure such as t.Fatalf (or t.Fatalf-like
usage) so the test fails and prints the actual error (e.g., use t.Fatalf("engine
bootstrap requires provisioned server home: %v", err)). Locate the err check
where t.Skip is used and change it to fail the test and include err in the
message.
In `@docs/content/guides/deployment-patterns/embed-thunderidengine.mdx`:
- Line 5: Replace all hardcoded occurrences of "Thunder" and "ThunderID" in this
document: in the YAML frontmatter swap product name values to use
{{ProductName}}; in prose, headings and any JSX use the component form
<ProductName />; and inside fenced code blocks or inline code (non-symbol text)
use {{ProductName}}. Search for the literal strings "Thunder" and "ThunderID"
(including in frontmatter, paragraph text, headings, JSX fragments and code
blocks) and update each instance accordingly so frontmatter uses
{{ProductName}}, prose/JSX uses <ProductName />, and code blocks/inline code use
{{ProductName}}.
- Line 33: Replace the incorrect Vale term "UserInfo" in the table row string
"`GET` / `POST /oauth2/userinfo` | UserInfo" with the correct "Userinfo"; locate
the table entry in embed-thunderidengine.mdx (search for the exact text "`GET` /
`POST /oauth2/userinfo`") and update the right-hand cell value to "Userinfo" so
the Vale linting passes.
In `@docs/sidebars.ts`:
- Around line 406-410: The new sidebar entry with id
'guides/deployment-patterns/embed-thunderidengine' and label 'Embed OAuth and
Flow Engine' is placed as a top-level sibling; move it into the nested array for
the 'Deployment Paths' section so it appears under 'Deployment Paths' ->
'Deployment Patterns'. Locate the object that defines the 'Deployment Paths'
section and insert the { type: 'doc', id:
'guides/deployment-patterns/embed-thunderidengine', label: 'Embed OAuth and Flow
Engine' } entry inside its 'items' (or the 'Deployment Patterns' subsection's
items) instead of at the top level.
---
Nitpick comments:
In `@backend/internal/enginebridge/bridge_client.go`:
- Around line 101-105: The GetCertificate method currently returns an empty
&inboundclient.CertOperationError{} which hides the failure reason; update
clientBridge.GetCertificate to return a populated CertOperationError indicating
"not implemented" (set available fields such as Message or Error and a Code or
Type field to "NotImplemented" or similar) so callers can log/branch correctly
and see the intent that GetCertificate is unimplemented.
In `@backend/pkg/thunderidengine/engine.go`:
- Around line 30-32: RegisterBootstrap currently overwrites registeredBootstrap
on every call; change it to perform a single-assignment of the first non-nil
bootstrapFunc and ignore subsequent calls (and nil args) to avoid accidental
replacement. Implement this by guarding the assignment with a check that
registeredBootstrap is nil and fn != nil (or use a sync.Once named bootstrapOnce
and call bootstrapOnce.Do to set registeredBootstrap) so the first non-nil fn is
preserved and further RegisterBootstrap calls are no-ops; keep the symbol names
RegisterBootstrap, registeredBootstrap, bootstrapFunc and make the behavior
concurrency-safe.
In `@backend/tests/integration/providers.go`:
- Around line 131-145: The Store method in memoryRuntimeStore currently uses a
non-unique key "urn:par:"+parRequest.ClientID which causes overwrites; update
memoryRuntimeStore.Store to generate a unique key (e.g., UUID or
timestamp+random) such as "urn:par:<unique-id>", store the parRequest in m.par
under that generated key, and return that key; keep newMemoryRuntimeStore and
the m.par map unchanged but ensure the generated key format is used wherever
callers of Store expect the returned key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f648e151-24de-4f9d-9a63-8ee52db3f666
📒 Files selected for processing (72)
ARCHITECTURE.mdbackend/cmd/server/servicemanager.gobackend/internal/authnprovider/manager/model.gobackend/internal/enginebridge/bootstrap.gobackend/internal/enginebridge/bootstrap_config.gobackend/internal/enginebridge/bootstrap_integration_test.gobackend/internal/enginebridge/bootstrap_internal.gobackend/internal/enginebridge/bootstrap_providers.gobackend/internal/enginebridge/bridge.gobackend/internal/enginebridge/bridge_authn.gobackend/internal/enginebridge/bridge_authz.gobackend/internal/enginebridge/bridge_client.gobackend/internal/enginebridge/bridge_entity.gobackend/internal/enginebridge/bridge_executor_registry.gobackend/internal/enginebridge/bridge_idp.gobackend/internal/enginebridge/bridge_observability.gobackend/internal/enginebridge/bridge_ou.gobackend/internal/enginebridge/bridge_resource.gobackend/internal/enginebridge/bridges_compile_test.gobackend/internal/enginebridge/bridges_provider_test.gobackend/internal/enginebridge/bridges_test.gobackend/internal/enginebridge/convert.gobackend/internal/enginebridge/default_runtime_store.gobackend/internal/enginebridge/flow_definition_bridge.gobackend/internal/enginebridge/init.gobackend/internal/enginebridge/register.gobackend/internal/enginebridge/runtime_store_test.gobackend/internal/enginebridge/runtime_stores.gobackend/internal/enginebridge/server_runtime.gobackend/internal/flow/executor/deps.gobackend/internal/flow/executor/init.gobackend/internal/flow/executor/register.gobackend/internal/flow/executor/register_test.gobackend/internal/flow/flowexec/host_runtime_store.gobackend/internal/flow/flowexec/init.gobackend/internal/flow/flowexec/redis_store.gobackend/internal/flow/flowexec/service.gobackend/internal/flow/flowexec/store.gobackend/internal/flow/mgt/host_flow_provider.gobackend/internal/flow/mgt/runtime_flow_service.gobackend/internal/flow/mgt/runtime_init.gobackend/internal/oauth/engine_init.gobackend/internal/oauth/init.gobackend/internal/oauth/oauth2/authz/auth_code_store.gobackend/internal/oauth/oauth2/authz/auth_req_redis_store.gobackend/internal/oauth/oauth2/authz/auth_req_store.gobackend/internal/oauth/oauth2/authz/auth_req_store_test.gobackend/internal/oauth/oauth2/authz/host_runtime_store.gobackend/internal/oauth/oauth2/authz/init.gobackend/internal/oauth/oauth2/authz/init_stores_test.gobackend/internal/oauth/oauth2/authz/init_test.gobackend/internal/oauth/oauth2/authz/service.gobackend/internal/oauth/oauth2/granthandlers/init.gobackend/internal/oauth/oauth2/par/host_runtime_store.gobackend/internal/oauth/oauth2/par/init.gobackend/internal/oauth/oauth2/par/init_stores_test.gobackend/internal/oauth/oauth2/par/redis_store.gobackend/internal/oauth/oauth2/par/service.gobackend/internal/oauth/oauth2/par/store.gobackend/pkg/thunderidengine/config.gobackend/pkg/thunderidengine/doc.gobackend/pkg/thunderidengine/engine.gobackend/pkg/thunderidengine/engine_bootstrap_test.gobackend/pkg/thunderidengine/errors.gobackend/pkg/thunderidengine/executor.gobackend/pkg/thunderidengine/model.gobackend/pkg/thunderidengine/providers.gobackend/pkg/thunderidengine/providers_test.gobackend/tests/integration/engine_test.gobackend/tests/integration/providers.godocs/content/guides/deployment-patterns/embed-thunderidengine.mdxdocs/sidebars.ts
Comment on lines
+32
to
+60
| func NewContextStoreFromRuntime(host thunderidengine.RuntimeStore) ContextStoreInterface { | ||
| return &hostContextStore{host: host} | ||
| } | ||
|
|
||
| func (s *hostContextStore) StoreFlowContext( | ||
| ctx context.Context, dbModel FlowContextDB, expirySeconds int64, | ||
| ) error { | ||
| return s.host.StoreFlowContext(ctx, toPublicFlowContext(dbModel), expirySeconds) | ||
| } | ||
|
|
||
| func (s *hostContextStore) GetFlowContext(ctx context.Context, executionID string) (*FlowContextDB, error) { | ||
| pub, err := s.host.GetFlowContext(ctx, executionID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if pub == nil { | ||
| return nil, nil | ||
| } | ||
| internal := fromPublicFlowContext(*pub) | ||
| return &internal, nil | ||
| } | ||
|
|
||
| func (s *hostContextStore) UpdateFlowContext(ctx context.Context, dbModel FlowContextDB) error { | ||
| return s.host.UpdateFlowContext(ctx, toPublicFlowContext(dbModel)) | ||
| } | ||
|
|
||
| func (s *hostContextStore) DeleteFlowContext(ctx context.Context, executionID string) error { | ||
| return s.host.DeleteFlowContext(ctx, executionID) | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against nil host runtime store to prevent panics.
The adapter methods dereference s.host without validation. A nil RuntimeStore will panic on first call instead of returning a controlled error.
Proposed fix
import (
"context"
+ "fmt"
"github.com/thunder-id/thunderid/pkg/thunderidengine"
)
@@
type hostContextStore struct {
host thunderidengine.RuntimeStore
}
+
+func (s *hostContextStore) validateHost() error {
+ if s.host == nil {
+ return fmt.Errorf("runtime store is not configured")
+ }
+ return nil
+}
@@
func (s *hostContextStore) StoreFlowContext(
ctx context.Context, dbModel FlowContextDB, expirySeconds int64,
) error {
+ if err := s.validateHost(); err != nil {
+ return err
+ }
return s.host.StoreFlowContext(ctx, toPublicFlowContext(dbModel), expirySeconds)
}
@@
func (s *hostContextStore) GetFlowContext(ctx context.Context, executionID string) (*FlowContextDB, error) {
+ if err := s.validateHost(); err != nil {
+ return nil, err
+ }
pub, err := s.host.GetFlowContext(ctx, executionID)
@@
func (s *hostContextStore) UpdateFlowContext(ctx context.Context, dbModel FlowContextDB) error {
+ if err := s.validateHost(); err != nil {
+ return err
+ }
return s.host.UpdateFlowContext(ctx, toPublicFlowContext(dbModel))
}
@@
func (s *hostContextStore) DeleteFlowContext(ctx context.Context, executionID string) error {
+ if err := s.validateHost(); err != nil {
+ return err
+ }
return s.host.DeleteFlowContext(ctx, executionID)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/internal/flow/flowexec/host_runtime_store.go` around lines 32 - 60,
NewContextStoreFromRuntime currently returns a hostContextStore that
dereferences s.host in methods (StoreFlowContext, GetFlowContext,
UpdateFlowContext, DeleteFlowContext) and will panic if the provided
RuntimeStore is nil; change the constructor to validate the host argument and
return an error or a no-op implementation when host is nil, or store an explicit
nil-check flag, and update each method to check s.host != nil and return a clear
error (e.g., fmt.Errorf("nil runtime store") or a package-level sentinel error)
instead of panicking; ensure GetFlowContext still returns (nil, nil) when
appropriate and keep conversions (toPublicFlowContext/fromPublicFlowContext)
unchanged.
Comment on lines
+75
to
+86
| func resolveFlowContextStore(opts *InitOptions) (ContextStoreInterface, transaction.Transactioner, error) { | ||
| if opts != nil && opts.ContextStore != nil { | ||
| if config.GetServerRuntime().Config.Database.Runtime.Type == dbprovider.DataSourceTypeRedis { | ||
| return opts.ContextStore, transaction.NewNoOpTransactioner(), nil | ||
| } | ||
| dbProvider := dbprovider.GetDBProvider() | ||
| transactioner, err := dbProvider.GetRuntimeDBTransactioner() | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| return opts.ContextStore, transactioner, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Custom ContextStore path should not require runtime DB transactioner.
When opts.ContextStore is provided, this code still initializes a DB transactioner for non-Redis runtime types. That can fail startup even though persistence is delegated to the custom store.
Proposed fix
func resolveFlowContextStore(opts *InitOptions) (ContextStoreInterface, transaction.Transactioner, error) {
if opts != nil && opts.ContextStore != nil {
- if config.GetServerRuntime().Config.Database.Runtime.Type == dbprovider.DataSourceTypeRedis {
- return opts.ContextStore, transaction.NewNoOpTransactioner(), nil
- }
- dbProvider := dbprovider.GetDBProvider()
- transactioner, err := dbProvider.GetRuntimeDBTransactioner()
- if err != nil {
- return nil, nil, err
- }
- return opts.ContextStore, transactioner, nil
+ return opts.ContextStore, transaction.NewNoOpTransactioner(), nil
}📝 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
| func resolveFlowContextStore(opts *InitOptions) (ContextStoreInterface, transaction.Transactioner, error) { | |
| if opts != nil && opts.ContextStore != nil { | |
| if config.GetServerRuntime().Config.Database.Runtime.Type == dbprovider.DataSourceTypeRedis { | |
| return opts.ContextStore, transaction.NewNoOpTransactioner(), nil | |
| } | |
| dbProvider := dbprovider.GetDBProvider() | |
| transactioner, err := dbProvider.GetRuntimeDBTransactioner() | |
| if err != nil { | |
| return nil, nil, err | |
| } | |
| return opts.ContextStore, transactioner, nil | |
| } | |
| func resolveFlowContextStore(opts *InitOptions) (ContextStoreInterface, transaction.Transactioner, error) { | |
| if opts != nil && opts.ContextStore != nil { | |
| return opts.ContextStore, transaction.NewNoOpTransactioner(), nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/internal/flow/flowexec/init.go` around lines 75 - 86, The custom
ContextStore path in resolveFlowContextStore currently still initializes a
runtime DB transactioner for non-Redis runtimes; change resolveFlowContextStore
so that whenever opts != nil && opts.ContextStore != nil it immediately returns
opts.ContextStore with transaction.NewNoOpTransactioner() (do not call
dbprovider.GetDBProvider() or GetRuntimeDBTransactioner()), so a provided
ContextStore does not force initialization of the runtime DB transactioner.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
anushasunkada
force-pushed
the
local_main
branch
2 times, most recently
from
8720e7e to
78dca70
Compare
senthalan
reviewed
May 22, 2026
| // Import this package for side effects when embedding the engine from another module: | ||
| // | ||
| // import _ "github.com/thunder-id/thunderid/pkg/thunderidengine/embed" | ||
| package embed |
Member
There was a problem hiding this comment.
What is the need of this package
| * under the License. | ||
| */ | ||
|
|
||
| package thunderidengine |
Member
There was a problem hiding this comment.
Shall we use the gateway, to align with the ThunderID architecture.
Suggested change
| package thunderidengine | |
| package gateway |
| IDP IDPProvider | ||
| FlowDefinition FlowDefinitionProvider | ||
| Observability ObservabilityProvider | ||
| RuntimeStore RuntimeStore |
Member
There was a problem hiding this comment.
Shall we take this out from providers and keep it out
|
|
||
| // Package thunderidengine is the public embed surface for Thunder's OAuth authorization | ||
| // server and flow execution runtime. | ||
| package thunderidengine |
Member
There was a problem hiding this comment.
We don't use the doc.go to maintain the package level comments. We usually add it in the package service or main file
|
|
||
| // RegisterBootstrap registers the module-internal bootstrap implementation. | ||
| // It is called from internal/enginebridge and must not be used by embedders. | ||
| func RegisterBootstrap(fn bootstrapFunc) { |
Member
There was a problem hiding this comment.
Can't we make the internal implementation also to follow the same path? Having two entry points for the library one for internal and external might cause issues in long run. We should keep only one entry point and used internally too
Comment on lines
+21
to
+22
| // NewExecutorRegistry creates an empty executor registry. | ||
| func NewExecutorRegistry() ExecutorRegistryInterface { |
Member
There was a problem hiding this comment.
Why we have to make the NewExecutorRegistry public function? In ThunderID, we have not made the constructor public.
All the constructors will be private and each package will have the Initialize function which will internally create the needed instances and return the the instances needed for other packages to consume. So that the usage of those instances are controlled.
Comment on lines
+26
to
+27
| // DefaultExecutorNames returns the full built-in executor name list. | ||
| func DefaultExecutorNames() []string { |
Member
There was a problem hiding this comment.
Should be private
Suggested change
| // DefaultExecutorNames returns the full built-in executor name list. | |
| func DefaultExecutorNames() []string { | |
| // defaultExecutorNames returns the full built-in executor name list. | |
| func defaultExecutorNames() []string { |
Comment on lines
+69
to
+70
| // RegisterDefaultExecutors registers built-in executors on reg. When names is empty, all defaults are registered. | ||
| func RegisterDefaultExecutors(reg ExecutorRegistryInterface, deps ExecutorDeps, names []string) { |
Member
There was a problem hiding this comment.
Suggested change
| // RegisterDefaultExecutors registers built-in executors on reg. When names is empty, all defaults are registered. | |
| func RegisterDefaultExecutors(reg ExecutorRegistryInterface, deps ExecutorDeps, names []string) { | |
| // registerDefaultExecutors registers built-in executors on reg. When names is empty, all defaults are registered. | |
| func registerDefaultExecutors(reg ExecutorRegistryInterface, deps ExecutorDeps, names []string) { |
| flowExecService, observabilitySvc, runtimeCryptoSvc, ouService, attributeCacheService, authZService, | ||
| entityProvider, resourceService, i18nService, idpService) | ||
| // Runtime routes (OAuth AS, flow execute, flow meta) via thunderidengine server wiring. | ||
| err = enginebridge.RegisterServerRuntime(mux, enginebridge.ServerRuntimeDeps{ |
Member
There was a problem hiding this comment.
I feel like we should remove the bridge and refactor our internal implementations to use the interfaces defined in the pkg.
So the servermanager will become another embedder implementation. So there is no backdoor for our implementation to bypass the pkg layer.
|
|
||
| // InitializeEngine initializes OAuth runtime services for the embeddable engine. | ||
| // DCR is omitted unless EnableDCR is true and ApplicationService is set. | ||
| func InitializeEngine(deps EngineDeps) error { |
Member
There was a problem hiding this comment.
Let's not have two paths. As pointed out in earlier, let's have as single entry point
Signed-off-by: anushasunkada <anushasunkada@gmail.com>
Signed-off-by: anushasunkada <anushasunkada@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: anushasunkada <anushasunkada@gmail.com>
Signed-off-by: anushasunkada <anushasunkada@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Approach
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
New Features
Documentation