[BUG] Updating a template unnecessarily checks for the existence of a template we'll overwrite

[dbwiddis]

What is the bug?

When updating a template, we perform two get requests, one to check for the existence of the template (that we are going to overwrite), and a second one to get the provisioning status:

flow-framework/src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java

Lines 409 to 423 in bae3aa2

	public void updateTemplateInGlobalContext(
	String documentId,
	Template template,
	ActionListener<IndexResponse> listener,
	boolean ignoreNotStartedCheck
	) {
	if (!doesIndexExist(GLOBAL_CONTEXT_INDEX)) {
	String errorMessage = "Failed to update template for workflow_id : " + documentId + ", global_context index does not exist.";
	logger.error(errorMessage);
	listener.onFailure(new FlowFrameworkException(errorMessage, RestStatus.BAD_REQUEST));
	return;
	}
	doesTemplateExist(documentId, templateExists -> {
	if (templateExists) {
	getProvisioningProgress(documentId, progress -> {

What is the expected behavior?

We should query the state index once for the provisioning status, and assume if the state doesn't exist, the template doesn't either. Since we are overwriting the template it doesn't matter whether it exists if the state is other than NOT_STARTED.

[JamieBort]

Hi, @dbwiddis. I haven't come across an any protocol in the documentation on how to assign an Issue to myself. So to prevent anyone else from working on it I am assigning it to myself now. Please correct me if I am mistaken.

[dbwiddis]

Hey @JamieBort welcome! Not sure if you have permission to assign to yourself, but I'm assigning it to you! Let me know if you have any questions.

[JamieBort]

Does it matter what branch name I use?
I was expecting a button somewhere on this issue that generated a branch for me; which in turn created a name based on the title of the Issue. From there I would fork the repo, clone my fork, and then checkout the new branch.
However that's the workflow for GitHub's dependabot.
Maybe this isn't available in this scenario.

EDIT - if the answers to these questions are in the contribution documentation, please let me know. I know you're busy and while I have looked there already, I rather get familiar with the documentation sooner than later.

[dbwiddis]

Does it matter what branch name I use?

Nope, we use a triangle workflow (you can google that) but basically:

1. you will fork this repository to your own GitHub repository (look for a "Fork" button in the upper right corner of the main page)
2. you will clone your personal repo to your local machine (clicking the dropdown on the "code" button on the main page gives you the git clone commands or other ways to do it)
3. you will start with an updated main branch (git pull upstream main)
4. create your own feature branch with whatever name you want (git checkout -b pick-a-name-here)
5. write your code!
6. write tests for your changes!
7. commit as often as you want as you complete portions of the work
8. Eventually when you think you're ready to submit, format the code with ./gradlew spotlessApply
9. Also make sure you've done your javadoces ./gradlew javadoc
10. And make sure tests pass ./gradlew test

If all that works, push (git push) to upload the code to your fork. The response from GitHub will give you a URL you can go to, OR you can go to the main repo and there's usually a pop up showing you recently updated a branch, OR you can go to your fork and look for the "contribute" button. Follow that dialogue to open a PR.

To respond to PR review comments, just edit your code, commit, and push (to your fork) and the PR will automatically get updated.

I was expecting a button somewhere on this issue that generated a branch for me; which in turn created a name based on the title of the Issue.

Not here.

From there I would fork the repo, clone my fork, and then checkout the new branch. However that's the workflow for GitHub's dependabot. Maybe this isn't available in this scenario.

You can fork at any time.

EDIT - if the answers to these questions are in the contribution documentation, please let me know. I know you're busy and while I have looked there already, I rather get familiar with the documentation sooner than later.

A lot of this is at https://github.com/opensearch-project/flow-framework/blob/main/DEVELOPER_GUIDE.md but you can let us know if there's any way we can clarify that!

[JamieBort]

All of this is very helpful. Thank you!

I am familiar with the Developer Guide that you shared. (Thank you for that.)

But neglected to read about ./gradlew spotlessApply. (4. under the Build section.)

And I am sorry to say that this will be my introduction to writing tests as well.

I feel I have a clear understanding for how the code needs to be refactored. (To me that's the easiest part of this task.)

However I am concerned about accomplishing this in a timely manner. Do you have a deadline for this?

[dbwiddis]

No deadline. It's a nice-to-have. Next release code freeze is about 6 weeks away but if you don't make that... not an issue!

[dbwiddis]

As far as testing, the method you're changing is already covered by tests, so either it will fail, or it will pass but include an unneeded mock of an index response. Either way, find the associated test and make sure it accurately reflects what the method does.

[JamieBort]

I've made my changes (Step 5 from #678 (comment)).

However when I run ./gradlew check (Step 1 from ) the Building from the command line section of the Developer Guide, I am receiving a testFailedUpdateTemplateInGlobalContextNotExisting FAILED error.

I am new to Gradle, however so I'm trying to do my due diligence by understand it better. And by attempting to answer my own questions with a little research before I start posting them here.

That said, I'm sharing the complete output in a .txt file for completeness:
flow-framework_test_20240507.txt

To complicate matters the report states 100% successful:

Something I'll be looking into is my JDK version.
The documentation states 11 is sufficient but 14 is required for the full suite of tests:

[To run the full suite of tests, download and install JDK 14 and set JAVA11_HOME, and JAVA14_HOME.](https://github.com/opensearch-project/OpenSearch/blob/main/DEVELOPER_GUIDE.md#jdk-14)

If that is the culprit I'll need up switch to 14 from 13:

OpenSearch Build Hamster says Hello!
  Gradle Version        : 8.7
  OS Info               : Mac OS X 10.16 (x86_64)
  JDK Version           : 13 (Oracle JDK)
  JAVA_HOME             : /Library/Java/JavaVirtualMachines/jdk-13.0.2.jdk/Contents/Home
  Random Testing Seed   : A3CFA76AA32C86BC
  In FIPS 140 mode      : false

[dbwiddis]

Hey Jamie, thanks for the update!

testFailedUpdateTemplateInGlobalContextNotExisting FAILED error.

This is probably a test depending on the old behavior that you just removed as I implied in this comment.

The stack trace after the failed test shows the failed assertion:

org.opensearch.flowframework.indices.FlowFrameworkIndicesHandlerTests.testFailedUpdateTemplateInGlobalContextNotExisting(FlowFrameworkIndicesHandlerTests.java:184)

I'm not sure exactly which line 184 corresponds to yours, but it seems to be the last assertion in this test:

flow-framework/src/test/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandlerTests.java

Lines 170 to 192 in e3a8784

	public void testFailedUpdateTemplateInGlobalContextNotExisting() throws IOException {
	Template template = mock(Template.class);
	@SuppressWarnings("unchecked")
	ActionListener<IndexResponse> listener = mock(ActionListener.class);
	FlowFrameworkIndex index = FlowFrameworkIndex.GLOBAL_CONTEXT;
	ClusterState mockClusterState = mock(ClusterState.class);
	Metadata mockMetadata = mock(Metadata.class);
	when(clusterService.state()).thenReturn(mockClusterState);
	when(mockClusterState.metadata()).thenReturn(mockMetadata);
	when(mockMetadata.hasIndex(index.getIndexName())).thenReturn(true);
	when(flowFrameworkIndicesHandler.doesIndexExist(GLOBAL_CONTEXT_INDEX)).thenReturn(true);
	doAnswer(invocation -> {
	ActionListener<GetResponse> responseListener = invocation.getArgument(1);
	responseListener.onFailure(new Exception("Failed to get template"));
	return null;
	}).when(client).get(any(GetRequest.class), any());
	flowFrameworkIndicesHandler.updateTemplateInGlobalContext("1", template, listener);
	ArgumentCaptor<Exception> exceptionCaptor = ArgumentCaptor.forClass(Exception.class);
	verify(listener, times(1)).onFailure(exceptionCaptor.capture());
	assertTrue(exceptionCaptor.getValue().getMessage().contains("Failed to get template"));
	}

Note that whole test was designed to check the line of code that you're removing (checking whether the template exists before updating it) so it seems you might be able to remove that test entirely.

However, I assume you must have added error handling for the state document (or its index) not existing, so you might want to look at testCanNotDeleteWorkflowStateDocInProgress() and copy that into a similar test method which tests the new behavior you added (except testing for non-existence rather than a value other than not started).

[dbwiddis]

The documentation states 11 is sufficient but 14 is required for the full suite of tests:

Don't worry about any JDK other than 11, 17, or 21. We use JDK17+ on the main branch and 11 on the 2.x branch which this will be backported to, but since you're building against main then 17 or 21 (or any JDK post 14) should work.

[dbwiddis]

copy that into a similar test method which tests the new behavior you added

One other note, we do measure test coverage; codecov will make a comment on a PR (if tests are successful) indicating any uncovered lines, so even if you don't add a new test immediately then after that runs, it can show you uncovered new lines. It's possible to run this code locally with ./gradlew diffCoverage, see https://github.com/opensearch-project/flow-framework/blob/main/CONTRIBUTING.md#code-coverage

[dbwiddis]

testFailedUpdateTemplateInGlobalContextNotExisting FAILED error.

One other helpful hint. You can see the logged error message in your output:

  1> [2024-05-07T19:44:02,236][INFO ][o.o.f.i.FlowFrameworkIndicesHandlerTests] [testFailedUpdateTemplateInGlobalContext] before test
  1> [2024-05-07T19:44:02,605][ERROR][o.o.f.i.FlowFrameworkIndicesHandler] [testFailedUpdateTemplateInGlobalContext] Failed to update template for workflow_id : 1, global_context index does not exist.
  1> [2024-05-07T19:44:02,697][INFO ][o.o.f.i.FlowFrameworkIndicesHandlerTests] [testFailedUpdateTemplateInGlobalContext] after test

So the test failure was looking for an exception message containing "Failed to get template" but I expect the message was something about global context index not existing.

[dbwiddis]

Hey @JamieBort hope you're doing well and making good progress.

@joshpalis is working on implementing #717 which is using the same "PUT" update API that was assumed when writing this issue. In his case, he needs the previous template in order to compare steps. I do think he may be fetching that template earlier in the REST/Transport sequence but since he doesn't have a draft PR out yet I haven't seen his implementation. So I wanted to call his attention to this issue and make sure he's not making any changes to the updateTemplateInGlobalContext() method. @joshpalis can you take a quick look and make sure this issue isn't overlapping any planned code changes you have? If so, let's discuss a path forward.

[JamieBort]

Hi, @dbwiddis. Thank you for letting me know. And yes, I'm still here!
However hit a road block with my task. I sent you an email (on May 17) because I thought that would be more appropriate for my discussion. When I had not heard back from you I assumed you were busy.
That said, I have not worked on this since then. I believe my issue is related to me using VS Code rather than Intellij. And I did not know who else to consult about it.
EDIT: FWIW, that's the second email I have sent you.

[JamieBort]

Hi, @dbwiddis. I just sent you a message via LinkedIn. In short, while I'd very much like to contribute to this repo, I think it is best that I unassign myself from this Issue. At least for the time being.
Thank you very much for all of your guidance and help.