Skip to content

Commit

Permalink
Refactor field update method to Template class
Browse files Browse the repository at this point in the history 
Signed-off-by: Daniel Widdis <[email protected]>
  • Loading branch information
@dbwiddis
dbwiddis committed Jul 10, 2024
1 parent a1d51b6 commit d6510ee
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 30 deletions.
33 changes: 32 additions & 1 deletion src/main/java/org/opensearch/flowframework/model/Template.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/
package org.opensearch.flowframework.model;

import org.apache.logging.log4j.util.Strings;
import org.opensearch.Version;
import org.opensearch.common.xcontent.LoggingDeprecationHandler;
import org.opensearch.common.xcontent.json.JsonXContent;
Expand All @@ -19,6 +20,7 @@
import org.opensearch.core.xcontent.XContentBuilder;
import org.opensearch.core.xcontent.XContentParser;
import org.opensearch.flowframework.exception.FlowFrameworkException;
import org.opensearch.flowframework.model.Template.Builder;
import org.opensearch.flowframework.util.ParseUtils;

import java.io.IOException;
Expand Down Expand Up @@ -55,7 +57,7 @@ public class Template implements ToXContentObject {
/** The template field name for template use case */
public static final String USE_CASE_FIELD = "use_case";
/** Fields which may be updated in the template even if provisioned */
private static final Set<String> UPDATE_FIELD_ALLOWLIST = Set.of(
public static final Set<String> UPDATE_FIELD_ALLOWLIST = Set.of(
NAME_FIELD,
DESCRIPTION_FIELD,
USE_CASE_FIELD,
Expand Down Expand Up @@ -343,6 +345,35 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
return xContentBuilder.endObject();
}

/**
* Merges two templates by updating the fields from an existing template with the (non-null) fields of another one.
* @param existingTemplate An existing complete template.
* @param templateWithNewFields A template containing only fields to update. The fields must be included in {@link #UPDATE_FIELD_ALLOWLIST}.
* @return the updated template.
*/
public static Template updateExistingTemplate(Template existingTemplate, Template templateWithNewFields) {
Builder builder = new Template.Builder(existingTemplate).lastUpdatedTime(Instant.now());
if (templateWithNewFields.name() != null) {
builder.name(templateWithNewFields.name());
}
if (!Strings.isBlank(templateWithNewFields.description())) {
builder.description(templateWithNewFields.description());
}
if (!Strings.isBlank(templateWithNewFields.useCase())) {
builder.useCase(templateWithNewFields.useCase());
}
if (templateWithNewFields.templateVersion() != null) {
builder.templateVersion(templateWithNewFields.templateVersion());
}
if (!templateWithNewFields.compatibilityVersion().isEmpty()) {
builder.compatibilityVersion(templateWithNewFields.compatibilityVersion());
}
if (templateWithNewFields.getUiMetadata() != null) {
builder.uiMetadata(templateWithNewFields.getUiMetadata());
}
return builder.build();
}

/**
* Parse raw xContent into a Template instance.
*
Expand Down
32 changes: 3 additions & 29 deletions src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.util.Strings;
import org.opensearch.ExceptionsHelper;
import org.opensearch.action.get.GetRequest;
import org.opensearch.action.search.SearchRequest;
Expand All @@ -30,7 +29,6 @@
import org.opensearch.flowframework.model.ProvisioningProgress;
import org.opensearch.flowframework.model.State;
import org.opensearch.flowframework.model.Template;
import org.opensearch.flowframework.model.Template.Builder;
import org.opensearch.flowframework.model.Workflow;
import org.opensearch.flowframework.workflow.ProcessNode;
import org.opensearch.flowframework.workflow.WorkflowProcessSorter;
Expand Down Expand Up @@ -243,36 +241,12 @@ protected void doExecute(Task task, WorkflowRequest request, ActionListener<Work
context.restore();
if (getResponse.isExists()) {
Template existingTemplate = Template.parse(getResponse.getSourceAsString());
Template template;
if (isFieldUpdate) {
// Update only specified fields in existing
Builder builder = new Template.Builder(existingTemplate).lastUpdatedTime(Instant.now());
if (templateWithUser.name() != null) {
builder.name(templateWithUser.name());
}
if (!Strings.isBlank(templateWithUser.description())) {
builder.description(templateWithUser.description());
}
if (!Strings.isBlank(templateWithUser.useCase())) {
builder.useCase(templateWithUser.useCase());
}
if (templateWithUser.templateVersion() != null) {
builder.templateVersion(templateWithUser.templateVersion());
}
if (!templateWithUser.compatibilityVersion().isEmpty()) {
builder.compatibilityVersion(templateWithUser.compatibilityVersion());
}
if (templateWithUser.getUiMetadata() != null) {
builder.uiMetadata(templateWithUser.getUiMetadata());
}
template = builder.build();
} else {
// Update existing entry, full document replacement
template = new Template.Builder(templateWithUser).createdTime(existingTemplate.createdTime())
Template template = isFieldUpdate
? Template.updateExistingTemplate(existingTemplate, templateWithUser)
: new Template.Builder(templateWithUser).createdTime(existingTemplate.createdTime())
.lastUpdatedTime(Instant.now())
.lastProvisionedTime(existingTemplate.lastProvisionedTime())
.build();
}
flowFrameworkIndicesHandler.updateTemplateInGlobalContext(
request.getWorkflowId(),
template,
Expand Down
42 changes: 42 additions & 0 deletions src/test/java/org/opensearch/flowframework/model/TemplateTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,48 @@ public void testTemplate() throws IOException {
assertThrows(FlowFrameworkException.class, () -> Template.parse(parser, true));
}

public void testUpdateExistingTemplate() {
Instant now = Instant.now();

Template original = new Template(
"name one",
"description one",
"use case one",
Version.fromString("1.1.1"),
List.of(Version.fromString("1.1.1"), Version.fromString("1.1.1")),
Collections.emptyMap(),
Map.of("uiMetadata", "one"),
null,
now,
now,
null
);
Template updated = new Template.Builder().name("name two").description("description two").useCase("use case two").build();
Template merged = Template.updateExistingTemplate(original, updated);
assertEquals("name two", merged.name());
assertEquals("description two", merged.description());
assertEquals("use case two", merged.useCase());
assertEquals("1.1.1", merged.templateVersion().toString());
assertEquals("1.1.1", merged.compatibilityVersion().get(0).toString());
assertEquals("1.1.1", merged.compatibilityVersion().get(1).toString());
assertEquals("one", merged.getUiMetadata().get("uiMetadata"));

updated = new Template.Builder().templateVersion(Version.fromString("2.2.2"))
.compatibilityVersion(List.of(Version.fromString("2.2.2"), Version.fromString("2.2.2")))
.uiMetadata(Map.of("uiMetadata", "two"))
.build();
merged = Template.updateExistingTemplate(original, updated);
assertEquals("name one", merged.name());
assertEquals("description one", merged.description());
assertEquals("use case one", merged.useCase());
assertEquals("2.2.2", merged.templateVersion().toString());
assertEquals("2.2.2", merged.compatibilityVersion().get(0).toString());
assertEquals("2.2.2", merged.compatibilityVersion().get(1).toString());
assertEquals("two", merged.getUiMetadata().get("uiMetadata"));

assertTrue(merged.lastUpdatedTime().isAfter(now));
}

public void testExceptions() throws IOException {
FlowFrameworkException e;

Expand Down

0 comments on commit d6510ee

Please sign in to comment.