Skip to content
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

Improve action execution core #6262

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Thisara-Welmilla
Copy link
Contributor

@Thisara-Welmilla Thisara-Welmilla commented Jan 6, 2025

Issue:

With this PR following improvements will be added to the action exection core.

  1. Add INCOMPLETE action invocation status. Eg: In the custom authentication, the external service will be want to redirect user to custom page. There we can utilize this new INCOMPLETE state with REDIRECT operation.

  2. For success action invocation, enable to send custom json object as data.
    Eg: In the cutsom authentication, the external service will send the authenticated user detail as data.

  3. This will bring the Bring following method which execute the action id list to its interface.

    ActionExecutionStatus execute(ActionType actionType, String[] actionIdList,
                                  Map<String, Object> eventContext, String tenantDomain)
            throws ActionExecutionException;

Copy link

codecov bot commented Jan 6, 2025

Codecov Report

Attention: Patch coverage is 65.00000% with 35 lines in your changes missing coverage. Please review.

Project coverage is 45.92%. Comparing base (9148662) to head (66633f9).
Report is 99 commits behind head on master.

Files with missing lines Patch % Lines
...tion/execution/impl/ActionExecutorServiceImpl.java 82.35% 4 Missing and 2 partials ⚠️
...tion/model/ActionInvocationIncompleteResponse.java 66.66% 3 Missing and 3 partials ⚠️
...bon/identity/action/execution/model/UserClaim.java 0.00% 6 Missing ⚠️
...ntity/action/execution/model/IncompleteStatus.java 58.33% 5 Missing ⚠️
...on/identity/action/execution/model/Incomplete.java 0.00% 4 Missing ⚠️
...on/execution/ActionExecutionResponseProcessor.java 0.00% 2 Missing ⚠️
...2/carbon/identity/action/execution/model/User.java 50.00% 2 Missing ⚠️
...ity/action/execution/util/OperationComparator.java 0.00% 1 Missing and 1 partial ⚠️
...tion/execution/model/ActionInvocationResponse.java 50.00% 1 Missing ⚠️
...ecution/model/ActionInvocationSuccessResponse.java 80.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #6262      +/-   ##
============================================
+ Coverage     45.72%   45.92%   +0.20%     
- Complexity    14065    14581     +516     
============================================
  Files          1633     1659      +26     
  Lines        100705   103874    +3169     
  Branches      17706    18241     +535     
============================================
+ Hits          46045    47709    +1664     
- Misses        47959    49318    +1359     
- Partials       6701     6847     +146     
Flag Coverage Δ
unit 29.04% <65.00%> (+0.58%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Thisara-Welmilla Thisara-Welmilla force-pushed the improve-action branch 3 times, most recently from 2cd2670 to 4b0b14d Compare January 9, 2025 13:50
@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/12701805770

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/12701805770
Status: failure

@@ -47,6 +49,23 @@ ActionExecutionStatus<Success> processSuccessResponse(Map<String, Object> eventC
ActionInvocationSuccessResponse successResponse) throws
ActionExecutionResponseProcessorException;

/**
* This method processes the incomplete response received from the action execution.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better mention the default implementation throws an unsupported exception so that actions that requires incomplete state can implement this method

@@ -388,6 +417,27 @@ private void logSuccessResponse(Action action, ActionInvocationSuccessResponse s
}
}

private void logIncompleteResponse(Action action, ActionInvocationIncompleteResponse incompleteResponse) {

DIAGNOSTIC_LOGGER.logSuccessResponse(action);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? If the same functionality can be reused better rename

private final List<PerformableOperation> operations;
private final ExtendedData data;
Copy link
Member

@malithie malithie Jan 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we rename this to some thing like ResponseData, ResponseContext etc.
I prefer ResponseData given the value is data.

Comment on lines +86 to 92
public Builder data(@JsonProperty("data") ExtendedData data) {

this.data = data;
return this;
}

public ActionInvocationSuccessResponse build() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the builder now expecting operations to be not null ?
With this it should be either operations or data that should be present

Comment on lines +30 to +32
public void setUrl(String url) {
this.url = url;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the setter ?

Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants