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.

Sign up for GitHub

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

Jump to bottom

Replace String concatenation with Log4j ParameterizedMessage for readability #943

Merged
merged 11 commits into from
Nov 11, 2024
Merged

Replace String concatenation with Log4j ParameterizedMessage for readability #943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
713caa1
Replace strings in GetWorkflowTransportAction.java
KirrTap Oct 24, 2024
c03bae8
Merge branch 'opensearch-project:main' into replace-strings
KirrTap Oct 24, 2024
0c4e600
Replace strings in 5 files
KirrTap Oct 27, 2024
d168f5d
Replace strings in 10 files
KirrTap Oct 28, 2024
7e02230
Merge branch 'opensearch-project:main' into replace-strings
KirrTap Nov 3, 2024
7cd4a0e
Replace strings in 40 files
KirrTap Nov 4, 2024
a82fdb5
Merge branch 'opensearch-project:main' into replace-strings
KirrTap Nov 6, 2024
7419752
Changed back strings, which were not be parametrized
KirrTap Nov 6, 2024
dfffc4e
Merge branch 'opensearch-project:main' into replace-strings
KirrTap Nov 7, 2024
5b84049
Update CHANGELOG.md
KirrTap Nov 7, 2024
91fd142
Add EOF line break
dbwiddis Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,4 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.1.0/)
### Documentation
### Maintenance
### Refactoring
- Replace String concatenation with Log4j ParameterizedMessage for readability ([#943](https://github.com/opensearch-project/flow-framework/pull/943))
104 changes: 81 additions & 23 deletions src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java
View file
Open in desktop

Large diffs are not rendered by default.

6 changes: 5 additions & 1 deletion src/main/java/org/opensearch/flowframework/model/WorkflowNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessageFactory;
import org.opensearch.common.unit.TimeValue;
import org.opensearch.core.rest.RestStatus;
import org.opensearch.core.xcontent.ToXContentObject;
Expand Down Expand Up @@ -171,7 +172,10 @@
String configurationsString = ParseUtils.parseArbitraryStringToObjectMapToString(configurationsMap);
userInputs.put(inputFieldName, configurationsString);
} catch (Exception ex) {
String errorMessage = "Failed to parse" + inputFieldName + "map";
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage(

Check warning on line 175 in src/main/java/org/opensearch/flowframework/model/WorkflowNode.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/model/WorkflowNode.java#L175 

Added line #L175 was not covered by tests
"Failed to parse {} map",
inputFieldName
).getFormattedMessage();

Check warning on line 178 in src/main/java/org/opensearch/flowframework/model/WorkflowNode.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/model/WorkflowNode.java#L178 

Added line #L178 was not covered by tests
logger.error(errorMessage, ex);
throw new FlowFrameworkException(errorMessage, RestStatus.BAD_REQUEST);
}
Expand Down
43 changes: 33 additions & 10 deletions src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessageFactory;
import org.opensearch.ExceptionsHelper;
import org.opensearch.action.get.GetRequest;
import org.opensearch.action.search.SearchRequest;
Expand Down Expand Up @@ -201,7 +202,10 @@
try {
validateWorkflows(templateWithUser);
} catch (Exception e) {
String errorMessage = "Workflow validation failed for template " + templateWithUser.name();
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage(
"Workflow validation failed for template {}",
templateWithUser.name()
).getFormattedMessage();
logger.error(errorMessage, e);
listener.onFailure(
e instanceof FlowFrameworkException ? e : new FlowFrameworkException(errorMessage, ExceptionsHelper.status(e))
Expand All @@ -218,7 +222,10 @@
flowFrameworkSettings.getMaxWorkflows(),
ActionListener.wrap(max -> {
if (FALSE.equals(max)) {
String errorMessage = "Maximum workflows limit reached: " + flowFrameworkSettings.getMaxWorkflows();
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage(
"Maximum workflows limit reached: {}",
flowFrameworkSettings.getMaxWorkflows()
).getFormattedMessage();
logger.error(errorMessage);
FlowFrameworkException ffe = new FlowFrameworkException(errorMessage, RestStatus.BAD_REQUEST);
listener.onFailure(ffe);
Expand Down Expand Up @@ -307,7 +314,10 @@
}));
}
}, exception -> {
String errorMessage = "Failed to update use case template " + request.getWorkflowId();
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage(

Check warning on line 317 in src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java#L317 

Added line #L317 was not covered by tests
"Failed to update use case template {}",
request.getWorkflowId()
).getFormattedMessage();

Check warning on line 320 in src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java#L319-L320 

Added lines #L319 - L320 were not covered by tests
logger.error(errorMessage, exception);
if (exception instanceof FlowFrameworkException) {
listener.onFailure(exception);
Expand Down Expand Up @@ -350,7 +360,10 @@
ActionListener.wrap(reprovisionResponse -> {
listener.onResponse(new WorkflowResponse(reprovisionResponse.getWorkflowId()));
}, exception -> {
String errorMessage = "Reprovisioning failed for workflow " + workflowId;
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage(
"Reprovisioning failed for workflow {}",
workflowId
).getFormattedMessage();
logger.error(errorMessage, exception);
if (exception instanceof FlowFrameworkException) {
listener.onFailure(exception);
Expand Down Expand Up @@ -382,9 +395,10 @@
);
listener.onResponse(new WorkflowResponse(request.getWorkflowId()));
}, exception -> {
String errorMessage = "Failed to update workflow "
+ request.getWorkflowId()
+ " in template index";
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage(

Check warning on line 398 in src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java#L398 

Added line #L398 was not covered by tests
"Failed to update workflow {} in template index",
request.getWorkflowId()
).getFormattedMessage();

Check warning on line 401 in src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java#L400-L401 

Added lines #L400 - L401 were not covered by tests
logger.error(errorMessage, exception);
if (exception instanceof FlowFrameworkException) {
listener.onFailure(exception);
Expand All @@ -399,7 +413,10 @@
listener.onResponse(new WorkflowResponse(request.getWorkflowId()));
}
}, exception -> {
String errorMessage = "Failed to update use case template " + request.getWorkflowId();
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage(
"Failed to update use case template {}",
request.getWorkflowId()
).getFormattedMessage();
logger.error(errorMessage, exception);
if (exception instanceof FlowFrameworkException) {
listener.onFailure(exception);
Expand All @@ -411,12 +428,18 @@
);
}
} else {
String errorMessage = "Failed to retrieve template (" + workflowId + ") from global context.";
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage(

Check warning on line 431 in src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java#L431 

Added line #L431 was not covered by tests
"Failed to retrieve template ({}) from global context.",
workflowId
).getFormattedMessage();

Check warning on line 434 in src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java#L434 

Added line #L434 was not covered by tests
logger.error(errorMessage);
listener.onFailure(new FlowFrameworkException(errorMessage, RestStatus.NOT_FOUND));
}
}, exception -> {
String errorMessage = "Failed to retrieve template (" + workflowId + ") from global context.";
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage(

Check warning on line 439 in src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java#L439 

Added line #L439 was not covered by tests
"Failed to retrieve template ({}) from global context.",
workflowId
).getFormattedMessage();

Check warning on line 442 in src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java#L442 

Added line #L442 was not covered by tests
logger.error(errorMessage, exception);
listener.onFailure(new FlowFrameworkException(errorMessage, ExceptionsHelper.status(exception)));
}));
Expand Down
6 changes: 5 additions & 1 deletion src/main/java/org/opensearch/flowframework/transport/DeprovisionWorkflowTransportAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessageFactory;
import org.opensearch.ExceptionsHelper;
import org.opensearch.OpenSearchStatusException;
import org.opensearch.action.support.ActionFilters;
Expand Down Expand Up @@ -166,7 +167,10 @@
)
);
}, exception -> {
String errorMessage = "Failed to get workflow state for workflow " + workflowId;
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage(

Check warning on line 170 in src/main/java/org/opensearch/flowframework/transport/DeprovisionWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/DeprovisionWorkflowTransportAction.java#L170 

Added line #L170 was not covered by tests
"Failed to get workflow state for workflow {}",
workflowId
).getFormattedMessage();

Check warning on line 173 in src/main/java/org/opensearch/flowframework/transport/DeprovisionWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/DeprovisionWorkflowTransportAction.java#L173 

Added line #L173 was not covered by tests
logger.error(errorMessage, exception);
listener.onFailure(new FlowFrameworkException(errorMessage, ExceptionsHelper.status(exception)));
}));
Expand Down
10 changes: 7 additions & 3 deletions src/main/java/org/opensearch/flowframework/transport/GetWorkflowStateTransportAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessageFactory;
import org.opensearch.ExceptionsHelper;
import org.opensearch.action.get.GetRequest;
import org.opensearch.action.support.ActionFilters;
Expand Down Expand Up @@ -96,7 +97,8 @@
);

} catch (Exception e) {
String errorMessage = "Failed to get workflow: " + workflowId;
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage("Failed to get workflow: {}", workflowId)
.getFormattedMessage();

Check warning on line 101 in src/main/java/org/opensearch/flowframework/transport/GetWorkflowStateTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/GetWorkflowStateTransportAction.java#L100-L101 

Added lines #L100 - L101 were not covered by tests
logger.error(errorMessage, e);
listener.onFailure(new FlowFrameworkException(errorMessage, ExceptionsHelper.status(e)));
}
Expand All @@ -123,7 +125,8 @@
WorkflowState workflowState = WorkflowState.parse(parser);
listener.onResponse(new GetWorkflowStateResponse(workflowState, request.getAll()));
} catch (Exception e) {
String errorMessage = "Failed to parse workflowState: " + r.getId();
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage("Failed to parse workflowState: {}", r.getId())
.getFormattedMessage();
logger.error(errorMessage, e);
listener.onFailure(new FlowFrameworkException(errorMessage, RestStatus.BAD_REQUEST));
}
Expand All @@ -134,7 +137,8 @@
if (e instanceof IndexNotFoundException) {
listener.onFailure(new FlowFrameworkException("Fail to find workflow status of " + workflowId, RestStatus.NOT_FOUND));
} else {
String errorMessage = "Failed to get workflow status of: " + workflowId;
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage("Failed to get workflow status of: {}", workflowId)
.getFormattedMessage();
logger.error(errorMessage, e);
listener.onFailure(new FlowFrameworkException(errorMessage, RestStatus.NOT_FOUND));
}
Expand Down
16 changes: 13 additions & 3 deletions src/main/java/org/opensearch/flowframework/transport/GetWorkflowTransportAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessageFactory;
import org.opensearch.ExceptionsHelper;
import org.opensearch.action.get.GetRequest;
import org.opensearch.action.support.ActionFilters;
Expand Down Expand Up @@ -104,7 +105,10 @@
xContentRegistry
);
} catch (Exception e) {
String errorMessage = "Failed to retrieve template (" + workflowId + ") from global context.";
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage(

Check warning on line 108 in src/main/java/org/opensearch/flowframework/transport/GetWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/GetWorkflowTransportAction.java#L108 

Added line #L108 was not covered by tests
"Failed to retrieve template ({}) from global context.",
workflowId
).getFormattedMessage();

Check warning on line 111 in src/main/java/org/opensearch/flowframework/transport/GetWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/GetWorkflowTransportAction.java#L111 

Added line #L111 was not covered by tests
logger.error(errorMessage, e);
listener.onFailure(new FlowFrameworkException(errorMessage, ExceptionsHelper.status(e)));
}
Expand Down Expand Up @@ -134,7 +138,10 @@
context.restore();

if (!response.isExists()) {
String errorMessage = "Failed to retrieve template (" + workflowId + ") from global context.";
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage(

Check warning on line 141 in src/main/java/org/opensearch/flowframework/transport/GetWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/GetWorkflowTransportAction.java#L141 

Added line #L141 was not covered by tests
"Failed to retrieve template ({}) from global context.",
workflowId
).getFormattedMessage();

Check warning on line 144 in src/main/java/org/opensearch/flowframework/transport/GetWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/GetWorkflowTransportAction.java#L144 

Added line #L144 was not covered by tests
logger.error(errorMessage);
listener.onFailure(new FlowFrameworkException(errorMessage, RestStatus.NOT_FOUND));
} else {
Expand All @@ -144,7 +151,10 @@
listener.onResponse(new GetWorkflowResponse(template));
}
}, exception -> {
String errorMessage = "Failed to retrieve template (" + workflowId + ") from global context.";
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage(
"Failed to retrieve template ({}) from global context.",
workflowId
).getFormattedMessage();
logger.error(errorMessage, exception);
listener.onFailure(new FlowFrameworkException(errorMessage, ExceptionsHelper.status(exception)));
}));
Expand Down
26 changes: 21 additions & 5 deletions src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessageFactory;
import org.opensearch.ExceptionsHelper;
import org.opensearch.action.get.GetRequest;
import org.opensearch.action.support.ActionFilters;
Expand Down Expand Up @@ -138,7 +139,10 @@
xContentRegistry
);
} catch (Exception e) {
String errorMessage = "Failed to retrieve template from global context for workflow " + workflowId;
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage(

Check warning on line 142 in src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java#L142 

Added line #L142 was not covered by tests
"Failed to retrieve template from global context for workflow {}",
workflowId
).getFormattedMessage();

Check warning on line 145 in src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java#L145 

Added line #L145 was not covered by tests
logger.error(errorMessage, e);
listener.onFailure(new FlowFrameworkException(errorMessage, ExceptionsHelper.status(e)));
}
Expand Down Expand Up @@ -169,7 +173,10 @@
context.restore();

if (!response.isExists()) {
String errorMessage = "Failed to retrieve template (" + workflowId + ") from global context.";
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage(

Check warning on line 176 in src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java#L176 

Added line #L176 was not covered by tests
"Failed to retrieve template ({}) from global context.",
workflowId
).getFormattedMessage();

Check warning on line 179 in src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java#L179 

Added line #L179 was not covered by tests
logger.error(errorMessage);
listener.onFailure(new FlowFrameworkException(errorMessage, RestStatus.NOT_FOUND));
return;
Expand Down Expand Up @@ -212,7 +219,10 @@
ActionListener.wrap(templateResponse -> {
listener.onResponse(new WorkflowResponse(request.getWorkflowId()));
}, exception -> {
String errorMessage = "Failed to update use case template " + request.getWorkflowId();
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage(

Check warning on line 222 in src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java#L222 

Added line #L222 was not covered by tests
"Failed to update use case template {}",
request.getWorkflowId()
).getFormattedMessage();

Check warning on line 225 in src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java#L224-L225 

Added lines #L224 - L225 were not covered by tests
logger.error(errorMessage, exception);
if (exception instanceof FlowFrameworkException) {
listener.onFailure(exception);
Expand All @@ -224,7 +234,10 @@
true
);
}, exception -> {
String errorMessage = "Failed to update workflow state: " + workflowId;
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage(

Check warning on line 237 in src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java#L237 

Added line #L237 was not covered by tests
"Failed to update workflow state: {}",
workflowId
).getFormattedMessage();

Check warning on line 240 in src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java#L240 

Added line #L240 was not covered by tests
logger.error(errorMessage, exception);
listener.onFailure(new FlowFrameworkException(errorMessage, ExceptionsHelper.status(exception)));
})
Expand All @@ -244,7 +257,10 @@
logger.error("Workflow validation failed for workflow {}", workflowId);
listener.onFailure(exception);
} else {
String errorMessage = "Failed to retrieve template from global context for workflow " + workflowId;
String errorMessage = ParameterizedMessageFactory.INSTANCE.newMessage(
"Failed to retrieve template from global context for workflow {}",
workflowId
).getFormattedMessage();
logger.error(errorMessage, exception);
listener.onFailure(new FlowFrameworkException(errorMessage, ExceptionsHelper.status(exception)));
}
Expand Down
Loading
Loading