Provide default values in DMN deployment records

[tmetzke]

Description

DMN resources that pass model validation but fail to deploy due to missing data, e.g. an empty decision name, can lead to unwritable data on the stream. A rejection for the deployment CREATE command can thus not be written due to MsgPack exceptions. This can run the message writer into an infinie loop of trying to write.

Using proper defaults in the DMN deployment records lets the writer successfully write the deployment CREATE command rejection.

Related issues

closes #16104

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

• The changes are backwards compatibility with previous versions
• If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/1.3) to the PR, in case that fails you need to create backports manually.

Testing:

• There are unit/integration tests that verify all acceptance criterias of the issue
• New tests are written to ensure backwards compatibility with further versions
• The behavior is tested manually
• The change has been verified by a QA run
• The impact of the changes is verified by a benchmark

Documentation:

• The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
• If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

Other teams:
If the change impacts another team an issue has been created for this team, explaining what they need to do to support this change.

• Operate
• Tasklist
• Web Modeler
• Desktop Modeler
• Optimize

Please refer to our review guidelines.

[nicpuppa]

I might be wrong, but new StringProperty("decisionId", ""); should be equal to new StringProperty("decisionId");. If you look at this lines for example, should be the same for the other property types.

Wdyt ? @tmetzke

[tmetzke]

I might be wrong, but new StringProperty("decisionId", ""); should be equal to new StringProperty("decisionId");. If you look at this lines for example, should be the same for the other property types.

Wdyt ? @tmetzke

No, it's not, I think. It will initialize the BaseProperty with a null default value. But we want a default value in case we cannot set anything while deploying (because we fail before we can).

[nicpuppa]

No, it's not, I think. It will initialize the BaseProperty with a null default value. But we want a default value in case we cannot set anything while deploying (because we fail before we can).

Thanks for the clarification ❤️

[tmetzke]

@nicpuppa, do you know if I have to consider any backward compatibility issues? I am introducing new values to the attributes in case they are not set. I am wondering if this needs to be considered anywhere 🤔

[nicpuppa]

I don't see any backward compatibility issues here 👀 🤔

[abbasadel]

Adding backporting lables to 8.3 and 8.4

[npepinpe]

👍

I can't speak for the backwards compatibility. If you add default values, it will work with older records which have none, so that's fine. However, it could be that there was a reason there was no default values - you'd have to ask whoever implemented this initially =/ Maybe Nico or Philipp know.

As for the endless loop, I tested it with the following QA test to make sure the endless loop is not triggered. While the unit test is probably sufficient, it relies on a test stream platform and not on the "production" one, so I felt safer with an integration test. You don't have to add it as I will probably add something anyway for #16107, but you can as well and I'll adapt it later.

Note

This test is not back port-able to 8.3 since we don't have the infrastructure.

/*
 * Copyright Camunda Services GmbH and/or licensed to Camunda Services GmbH under
 * one or more contributor license agreements. See the NOTICE file distributed
 * with this work for additional information regarding copyright ownership.
 * Licensed under the Zeebe Community License 1.1. You may not use this file
 * except in compliance with the Zeebe Community License 1.1.
 */
package io.camunda.zeebe.it.client.command;

import static org.assertj.core.api.Assertions.assertThat;

import io.camunda.zeebe.qa.util.cluster.TestStandaloneBroker;
import io.camunda.zeebe.qa.util.junit.ZeebeIntegration;
import io.camunda.zeebe.qa.util.junit.ZeebeIntegration.TestZeebe;
import io.camunda.zeebe.test.util.junit.RegressionTest;
import io.grpc.Status;
import io.grpc.Status.Code;
import io.grpc.StatusRuntimeException;
import java.nio.charset.StandardCharsets;
import java.time.Duration;
import java.util.concurrent.Future;
import org.assertj.core.api.InstanceOfAssertFactories;
import org.junit.jupiter.api.Test;

@ZeebeIntegration
public class EndlessLoopTest {
  @TestZeebe private final TestStandaloneBroker broker = new TestStandaloneBroker();

  private final String dmn =
      """
<?xml version="1.0" encoding="UTF-8"?>
<definitions xmlns="https://www.omg.org/spec/DMN/20191111/MODEL/" id="_6f87b3aa-d613-4c41-93ad-832f021c8318" name="definitions" namespace="http://camunda.org/schema/1.0/dmn">
  <decision id="_51711_44808585-3491-42a8-87ea-3c4fcda46eff" name="">
    <decisionTable id="_07c6fdc3-7bdf-459a-a016-2ae12e474bd2">
      <input id="_f00d0717-d08a-4918-ad51-568788e92038" label="Percent Responsible (Loan Roles)">
        <inputExpression id="_0b922041-84c3-47bf-b94c-1dd423c394ae" typeRef="number">
          <text>proposedLoan.loanRoles.percentResponsible</text>
        </inputExpression>
      </input>
      <output id="_b304292d-98e5-4dde-8fbd-0de1981c97ea" label="" name="out" typeRef="string">
        <outputValues id="UnaryTests_1yu7moy">
          <text>"Approve","Decline","Review"</text>
        </outputValues>
      </output>
      <rule id="DecisionRule_1j6jzzn">
        <inputEntry id="UnaryTests_105fhm8">
          <text></text>
        </inputEntry>
        <outputEntry id="LiteralExpression_1ucr6zl">
          <text>"Approve"</text>
        </outputEntry>
      </rule>
    </decisionTable>
  </decision>
</definitions>
""";

  @RegressionTest("https://github.com/camunda/zeebe/issues/16104")
  void shouldNotLoopEndlessly() {
    // given
    try (final var client = broker.newClientBuilder().build()) {
      // when
      final Future<?> result =
          client
              .newDeployResourceCommand()
              .addResourceBytes(dmn.getBytes(StandardCharsets.UTF_8), "bork.dmn")
              .requestTimeout(Duration.ofSeconds(30))
              .send();

      // then
      assertThat(result)
          .failsWithin(Duration.ofSeconds(10))
          .withThrowableThat()
          .havingCause()
          .isInstanceOf(StatusRuntimeException.class)
          .asInstanceOf(InstanceOfAssertFactories.throwable(StatusRuntimeException.class))
          .extracting(StatusRuntimeException::getStatus)
          .returns(Code.INVALID_ARGUMENT, Status::getCode);
    }
  }
}

[nicpuppa]

The PR looks good from my prespective 👍

[korthout]

I don't know of any issues with introducing defaults for the properties at a later time like you're doing now.

I do wonder why they weren't set in the first place. I tried to find it out in the original pull request introducing these properties, but I was unable to figure out why.

@saig0 Do you remember (or know) whether there was/is a specific reason to not have defaults for these properties?

[abbasadel]

Thanks everyone for your approval. I will start merging and backporting the PR

[saig0]

@korthout we expected that these properties should always be set. So, no defaults are required.

However, we can use default, similar to other records. 👍

[backport-action]

Successfully created backport PR for stable/8.3:

• [Backport stable/8.3] Provide default values in DMN deployment records #16108

[backport-action]

Successfully created backport PR for stable/8.4:

• [Backport stable/8.4] Provide default values in DMN deployment records #16109

[Zelldon]

@korthout we expected that these properties should always be set. So, no defaults are required.

But it was not enforced right?

[korthout]

@tmetzke Should this backported to 8.2 and 8.1 as well? DMN decisions have been around since 8.0 I believe.

[nicpuppa]

I see only pros in backporting this fix to other supported stables @korthout

[korthout]

💭 If you happen to want to backport to those versions later. Please ensure to unlabel the already successful backport labels. Otherwise, the action will attempt to backport to those again. It's not smart enough 😄

[tmetzke]

/backport

[backport-action]

Backport failed for stable/8.1, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin stable/8.1
git worktree add -d .worktree/backport-16103-to-stable/8.1 origin/stable/8.1
cd .worktree/backport-16103-to-stable/8.1
git switch --create backport-16103-to-stable/8.1
git cherry-pick -x 05fe07a3798012e6cdbf398669fdb38167471deb

[backport-action]

Backport failed for stable/8.2, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin stable/8.2
git worktree add -d .worktree/backport-16103-to-stable/8.2 origin/stable/8.2
cd .worktree/backport-16103-to-stable/8.2
git switch --create backport-16103-to-stable/8.2
git cherry-pick -x 05fe07a3798012e6cdbf398669fdb38167471deb

[tmetzke]

Backports are failing due to the tenantId property not being present in the DecisionRequirementsRecord in versions ≤8.2. The adjustments are thus not directly mergeable but easy to merge manually.

[tmetzke]

Successfully created backport PRs for stable/8.2 and stable/8.1:

• [Backport stable/8.2] Provide default values in DMN deployment records #16123
• [Backport stable/8.1] Provide default values in DMN deployment records #16124