Add dynamic executor loading to /flow/meta api - #1156
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary
WalkthroughThe Flow Management v1 module resolves legacy and dynamically registered flow executors through Sequence Diagram(s)sequenceDiagram
participant Client
participant FlowMetaHandler
participant ExecutorMetadataResolver
participant FlowExecutorRegistry
participant FlowMetaResponse
Client->>FlowMetaHandler: request flow metadata
FlowMetaHandler->>ExecutorMetadataResolver: resolve executors
FlowMetaHandler->>ExecutorMetadataResolver: resolve connections and required groups
ExecutorMetadataResolver->>FlowExecutorRegistry: discover registered executors
FlowExecutorRegistry-->>ExecutorMetadataResolver: return executor metadata
ExecutorMetadataResolver-->>FlowMetaHandler: return supported names and extensions
FlowMetaHandler->>FlowMetaResponse: create response with extensionExecutors
FlowMetaResponse-->>Client: return flow metadata
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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: 1
🧹 Nitpick comments (2)
components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/response/handlers/RegistrationFlowMetaHandler.java (1)
78-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCopy the superclass baseline before mutating it.
Line 81 mutates the list returned by
super.getLegacyExecutorBaseline()directly.AskPasswordFlowMetaHandlerandPasswordRecoveryFlowMetaHandlerwrap the result in a newArrayList. The current code works because the base method returns a fresh mutable list, but the copy keeps the three handlers consistent and protects against a future base change that returns an immutable list.♻️ Proposed refactor
- List<String> supportedExecutors = super.getLegacyExecutorBaseline(); + List<String> supportedExecutors = new ArrayList<>(super.getLegacyExecutorBaseline());🤖 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 `@components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/response/handlers/RegistrationFlowMetaHandler.java` around lines 78 - 90, Update RegistrationFlowMetaHandler.getLegacyExecutorBaseline to wrap the list returned by super.getLegacyExecutorBaseline() in a new mutable ArrayList before adding the executor constants, matching AskPasswordFlowMetaHandler and PasswordRecoveryFlowMetaHandler.components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/utils/ExecutorMetadataResolver.java (1)
152-182: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider memoizing
declaredExecutors()per instance.
declaredExecutors()calls the engine on every invocation.resolve(),getConnectionExecutorMap(), andgetExecutorsWithTag()each trigger a separate call within one request. A single memoized field would keep the resolved view consistent and reduce repeated service lookups.Also note that Line 176 adds extension names without a duplicate check, while
metadatade-duplicates by name. If the engine can return two entries with the same name,supportedExecutorNamescontains a duplicate.♻️ Proposed refactor
private final String flowType; private final List<String> legacyBaseline; + private List<FlowExecutorInfo> declared; private List<String> supportedExecutorNames;private List<FlowExecutorInfo> declaredExecutors() { + if (declared != null) { + return declared; + } try { - return FlowExecutorMetadataService.getInstance().getComposerExecutors(flowType); + declared = FlowExecutorMetadataService.getInstance().getComposerExecutors(flowType); } catch (Throwable e) { LOG.warn("Failed to resolve dynamically registered executors for flow type: " + flowType + ". Falling back to the built in executor list.", e); - return Collections.emptyList(); + declared = Collections.emptyList(); } + return declared; }LinkedHashMap<String, ExecutorMetadata> metadata = new LinkedHashMap<>(); for (FlowExecutorInfo extension : extensions) { - names.add(extension.getName()); - metadata.put(extension.getName(), toModel(extension)); + if (metadata.containsKey(extension.getName()) || names.contains(extension.getName())) { + continue; + } + names.add(extension.getName()); + metadata.put(extension.getName(), toModel(extension)); }Also applies to: 211-226
🤖 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 `@components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/utils/ExecutorMetadataResolver.java` around lines 152 - 182, Memoize the result of declaredExecutors() once per ExecutorMetadataResolver instance and reuse that field in resolve(), getConnectionExecutorMap(), and getExecutorsWithTag() so all lookups share one consistent view. While building supportedExecutorNames in resolve(), only add an extension name when it is not already present, matching metadata’s name de-duplication behavior.
🤖 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
`@components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/utils/ExecutorMetadataResolver.java`:
- Around line 74-78: Resolve the Javadoc reference in the parameter
documentation for the resolver method by replacing the unresolved {`@link`
FlowTypes} reference with a fully qualified type name or plain text. Keep the
description of the flow type unchanged.
---
Nitpick comments:
In
`@components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/response/handlers/RegistrationFlowMetaHandler.java`:
- Around line 78-90: Update
RegistrationFlowMetaHandler.getLegacyExecutorBaseline to wrap the list returned
by super.getLegacyExecutorBaseline() in a new mutable ArrayList before adding
the executor constants, matching AskPasswordFlowMetaHandler and
PasswordRecoveryFlowMetaHandler.
In
`@components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/utils/ExecutorMetadataResolver.java`:
- Around line 152-182: Memoize the result of declaredExecutors() once per
ExecutorMetadataResolver instance and reuse that field in resolve(),
getConnectionExecutorMap(), and getExecutorsWithTag() so all lookups share one
consistent view. While building supportedExecutorNames in resolve(), only add an
extension name when it is not already present, matching metadata’s name
de-duplication behavior.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dc4f220b-b77d-4faa-a795-5e9d9423313c
⛔ Files ignored due to path filters (2)
components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/gen/java/org/wso2/carbon/identity/api/server/flow/management/v1/ExecutorMetadata.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/gen/java/org/wso2/carbon/identity/api/server/flow/management/v1/FlowMetaResponse.javais excluded by!**/gen/**
📒 Files selected for processing (9)
components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/pom.xmlcomponents/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/constants/FlowEndpointConstants.javacomponents/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/response/handlers/AbstractMetaResponseHandler.javacomponents/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/response/handlers/AskPasswordFlowMetaHandler.javacomponents/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/response/handlers/PasswordRecoveryFlowMetaHandler.javacomponents/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/response/handlers/RegistrationFlowMetaHandler.javacomponents/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/utils/ExecutorMetadataResolver.javacomponents/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/utils/Utils.javacomponents/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/resources/flow.yaml
7597d1e to
6a340d7
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/utils/ExecutorMetadataResolver.java`:
- Around line 218-226: Cache the result of the executor lookup shared by
declaredExecutors(), resolve(), getConnectionExecutorMap(), and
getExecutorsWithTag() so each resolver output in a request uses the same
executor set. Ensure the cache also preserves the fallback empty list when
lookup fails, and reuse the cached value instead of invoking
FlowExecutorMetadataService.getInstance().getComposerExecutors(flowType)
multiple times.
- Around line 175-179: Update the extension loop in resolve() to de-duplicate
executor names before adding them to names, while still updating metadata using
the same accepted name and corresponding FlowExecutorInfo. Ensure duplicate
registry entries do not produce repeated names and preserve the existing
last-entry metadata behavior.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1087e3d5-285e-4350-a5b6-a928b2b5cc1c
⛔ Files ignored due to path filters (2)
components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/gen/java/org/wso2/carbon/identity/api/server/flow/management/v1/ExecutorMetadata.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/gen/java/org/wso2/carbon/identity/api/server/flow/management/v1/FlowMetaResponse.javais excluded by!**/gen/**
📒 Files selected for processing (9)
components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/pom.xmlcomponents/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/constants/FlowEndpointConstants.javacomponents/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/response/handlers/AbstractMetaResponseHandler.javacomponents/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/response/handlers/AskPasswordFlowMetaHandler.javacomponents/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/response/handlers/PasswordRecoveryFlowMetaHandler.javacomponents/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/response/handlers/RegistrationFlowMetaHandler.javacomponents/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/utils/ExecutorMetadataResolver.javacomponents/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/utils/Utils.javacomponents/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/resources/flow.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/resources/flow.yaml
- components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/utils/Utils.java
- components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/response/handlers/AskPasswordFlowMetaHandler.java
- components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/constants/FlowEndpointConstants.java
- components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/response/handlers/AbstractMetaResponseHandler.java
- components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/response/handlers/RegistrationFlowMetaHandler.java
- components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/response/handlers/PasswordRecoveryFlowMetaHandler.java
- components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/pom.xml
6a340d7 to
2c1fe13
Compare
| * | ||
| * @return Mutable list of executor names, in the order they should be presented. | ||
| */ | ||
| protected List<String> getLegacyExecutorBaseline() { |
There was a problem hiding this comment.
getSupportedExecutors
There was a problem hiding this comment.
No need of 2 methods to get common and another to extensions
There was a problem hiding this comment.
This will require all additional executors added for each flow to be defined in FlowEndpointConstants. Current approach is cleaner.
| protected List<String> getLegacyExecutorBaseline() { | ||
|
|
||
| List<String> supportedExecutors = super.getSupportedExecutors(); | ||
| List<String> supportedExecutors = super.getLegacyExecutorBaseline(); |
There was a problem hiding this comment.
get the union to here
There was a problem hiding this comment.
This requires all additional executors beyond common ones to be added to Constants. Current approach is cleaner
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/utils/ExecutorMetadataResolver.java (1)
222-227: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNarrow the registry failure boundary. An
ErrorfromFlowExecutorMetadataServiceis converted into incomplete metadata. Catch the registry exception type, orExceptionif a broad fallback is required.🤖 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 `@components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/utils/ExecutorMetadataResolver.java` around lines 222 - 227, Update the exception handler around FlowExecutorMetadataService.getInstance().getComposerExecutors(flowType) to catch the registry’s specific exception type, or Exception if a broad fallback is required, instead of Throwable; preserve the existing warning and empty-list fallback for caught failures.Source: MCP tools
🤖 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.
Outside diff comments:
In
`@components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/utils/ExecutorMetadataResolver.java`:
- Around line 222-227: Update the exception handler around
FlowExecutorMetadataService.getInstance().getComposerExecutors(flowType) to
catch the registry’s specific exception type, or Exception if a broad fallback
is required, instead of Throwable; preserve the existing warning and empty-list
fallback for caught failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 788d1ef4-9f55-4207-b3af-50842f21deaf
⛔ Files ignored due to path filters (1)
components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/gen/java/org/wso2/carbon/identity/api/server/flow/management/v1/ExecutorMetadata.javais excluded by!**/gen/**
📒 Files selected for processing (3)
components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/response/handlers/PasswordRecoveryFlowMetaHandler.javacomponents/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/utils/ExecutorMetadataResolver.javacomponents/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/resources/flow.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/java/org/wso2/carbon/identity/api/server/flow/management/v1/response/handlers/PasswordRecoveryFlowMetaHandler.java
- components/org.wso2.carbon.identity.api.server.flow.management/org.wso2.carbon.identity.api.server.flow.management.v1/src/main/resources/flow.yaml
9fc4169 to
1656837
Compare
1656837 to
8369c4b
Compare
7776ab1 to
a0d58a7
Compare
| description: Short explanation of what this step does | ||
| example: Verifies the user's identity with an external verification provider. | ||
| behaviorFlags: | ||
| type: array |
There was a problem hiding this comment.
Make this a first class variable rather than an array
Purpose
This pull request introduces support for extension executors in the flow management API, allowing the system to dynamically include metadata for executors contributed by deployed extensions. It also refactors how legacy executors are handled for backward compatibility and sets up infrastructure for resolving executor metadata.
Related Issues
Related PRs
Approach
Extension Executor Metadata Support:
ExecutorMetadatamodel class to represent metadata about extension executors, including fields such asname,displayName,description,tags,icon,requiresConnection, andassociatedAuthenticator. (ExecutorMetadata.java)FlowMetaResponsemodel to include a newextensionExecutorsfield, along with its getter, setter, and helper methods, so that API responses can return metadata for extension executors.Legacy Executors Refactoring:
LegacyExecutorsinner class inFlowEndpointConstantsto encapsulate hardcoded executor names, recovery factors, and authenticator-to-executor mappings for backward compatibility. This will allow the system to union dynamic executors on top of these legacy definitions. (FlowEndpointConstants.java)Infrastructure and Dependency Updates:
org.wso2.carbon.identity.flow.execution.enginemodule, which is likely required for the new executor metadata resolution logic. (pom.xml)ExecutorMetadataResolverutility import and an instance field inAbstractMetaResponseHandler, preparing the handler for resolving executor metadata from extensions.AbstractMetaResponseHandler.java)