Replace protocol-util module with explicit mapping via forward-declarations

[npepinpe]

Description

This PR drops the protocol-util module entirely, instead opting for a more explicit mapping done via a forward-declaration in the ImmutableProtocol annotation. The annotation now has a mandatory member, builder, which should be assigned to the builder class generated by the immutable protocol. This will allow classes to later find the correct builder class, enabling deserialization use cases. It could be further extended to also enable mapping by declaring the immutable class as well. Doing this means the type itself carries all the information it needs to be deserialized by protocol-jackson, which makes the shading use case much, much simpler. Operate now only has to shade the protocol itself, nothing else needs to be shaded (though they may still want to shade the deserialization module to be safe, as shading is there for major versions).

This PR also marks the generated types with two marker annotations: ImmutableProtocol.Type and ImmutableProtocol.Builder. The generated types (ImmutableRecord, ImmutableVariableRecordValue, etc.) will be marked with ImmutableProtocol.Type, and their builders with ImmutableProtocol.Builder. Doing so allows us to more easily detect whether a type needs special deserialization configuration (e.g. @JsonIgnore(unknownProperties = true) for the types marked), and also lets the ProtocolFactory easily find the concrete, immutable types for the abstract protocol types (i.e. look for ImmutableProtocol annotated types, then look for their implementing classes annotated with ImmutableProtocol.Type).

We can then move the ValueTypeMapping class into the protocol since it has no dependency other than the protocol. This allows us to get rid entirely of the protocol-util module, which was admittedly a very broad module by definition, and most likely would have become a shallow module over time.

A few tests were added to compensate, mostly ArchUnit tests to ensure that the forward-declared builder builds the correct type, and again in the factory to ensure we can generate objects for all protocol types.

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.)
• New content is added to the release announcement
• 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.

[korthout]

👍 Nice work @npepinpe.

❓ Have you tested this already with shading like Operate does it?

It feels slightly strange to annotate a new type with @ImmutableProtocol(immutable=ImmutableNewClassName.class, builder=ImmutableNewClassName.Builder.class) while the ImmutableNewClassName and the ImmutableNewClassName.Builder haven't been generated yet. Especially, because they are only generated because of the @ImmutableProtocol annotation. To annotate with @ImmutableProtocol both the immutable and the builder parameters are required but need to be filled with a not yet existing class. It feels a bit like a chicken/egg problem. Then again, once it's declared it will compile just fine. 🤔 Perhaps we just need to provide a clear direction for developers on how to add new types. Because that is the confusing part.

I don't necessarily mind having to add the parameters to the annotation for new types. It's not much more work or so. Developers already need to add the annotation when creating new protocol types and we have a test case for this to guide them. 🔧 Perhaps the test case needs to provide additional directions on how to annotate the interface.

[npepinpe]

I didn't test it with Operate, mostly because running their tests takes forever 😄 I did test a small shading example however, and it works nicely because the type you wish to deserialize carries all the information it needs.

So if you shade two versions of the protocol, relocating io.camunda.zeebe.protocol once to io.camunda.zeebe.protocol.v14 and once to io.camunda.zeebe.protocol.v13, then when the Jackson module encounters this, it will get, say, io.camunda.zeebe.protocol.v14.Record. This will be annotated with (expanding the annotations for the purpose of the example):

@io.camunda.zeebe.protocol.v14.ImmutableProtocol(
  immutable = io.camunda.zeebe.protocol.v14.ImmutableRecord.class,
  builder = io.camunda.zeebe.protocol.v14.ImmutableRecord.Builder.class
)
public interface io.camunda.zeebe.protocol.v14.Record<T extends io.camunda.zeebe.protocol.v14.RecordValue> {
   // ...
}

So when Jackson wants to, say, find the builder, then the annotation returns the shaded version as well. There is no need anymore for the zeebe-protocol-jackson and zeebe-protocol-util (which doesn't exist anymore anyway) to also be shaded.

I get that it looks a little weird initially, since the annotation which generates the types (e.g. ImmutableRecord) makes use of them. Keep in mind that the generation occurs during process-sources, which is before compilation - but I totally get that it's not ideal. I also don't like it, but I found it to be the "simplest" way of keeping everything self-contained. The other options are:

1. Classpath scanning (sort of what we do already). The disadvantage here is the need for an additional utility which is a little to purpose specific for my tastes, and you have to remember to shade the module doing the scanning as well so it also gets relocated.
2. Class lookup by convention - you could fix a convention, saying, if I see a type annotated with ImmutableProtocol, then I should find an equivalent, immutable class, in the same package, with the same name but prefixed Immutable. Additionally, it will have an inner builder class called Builder. So you can look them all up via reflection by convention. The advantage here over classpath scanning is obviously that you don't need any utility, and it is self-contained. The downside is that you might encounter more errors at runtime that could be picked up at compile time. It also means external users of this module (e.g. zeebe-protocol-jackson) need to be aware of these conventions, which isn't ideal. Note that you can test for all these cases, which would mitigate those disadvantages, obviously.

I think the second option is also something we can explore, but I'm wary of having conventions that aren't made explicit in the code, and having the annotation values works around that.

As the protocol is very much something the engine team will maintain (meaning not me 😄), I'm also not super keen on pushing something that you will not want to maintain, so I want to be sure that, well, you're sure of this 😄

[korthout]

@io.camunda.zeebe.protocol.v14.ImmutableProtocol(
  immutable = io.camunda.zeebe.protocol.v14.ImmutableRecord.class,
  builder = io.camunda.zeebe.protocol.v14.ImmutableRecord.Builder.class
)
public interface io.camunda.zeebe.protocol.v14.Record<T extends io.camunda.zeebe.protocol.v14.RecordValue> {
   // ...
}

Nice example. Thanks for sharing.

Keep in mind that the generation occurs during process-sources, which is before compilation

That's a good point.

The other options are:

1. Classpath scanning (sort of what we do already) ...
2. Class lookup by convention ...

I agree that the current proposal is better.

As the protocol is very much something the engine team will maintain (meaning not me 😄), I'm also not super keen on pushing something that you will not want to maintain, so I want to be sure that, well, you're sure of this

That makes sense. I was wondering about polling the team, but I think that's not necessary. I'm happy to accept a PR with the proposed solution.

[npepinpe]

The latest version shows an example of what it would look like without the forward declarations in the annotations, and instead rely on looking up the builder by name. Let me know what you think. I thought it might be good to see both and check which you prefer. I personally prefer the forward declarations as I'll know quicker if anything goes wrong, but the arch unit tests were more complicated to write (though that's probably just because I'm not super familiar with ArchUnit yet).

[korthout]

The latest version shows an example of what it would look like without the forward declarations in the annotations

It feels more brittle due to the fqdn construction and class loading. I guess it works with shading because the package is taken from the annotated type at runtime instead of being hardcoded. Nevertheless, this setup seems more complicated because you need to be aware of the naming convention.

My preference is still on the first option with forward declarations.

[npepinpe]

Here's one final option 😄 We only really need the forward declaration of the builder. For the immutable one, as we have the marker annotation, the factory can find it using classgraph (as it's already finding the protocol types anyway). So for now we don't need this.

I also added a test for the builder declaration. So if you have, say, Process, annotated with @ImmutableProtocol(builder = ImmutableTimerRecordValue.Builder.class), then you get this error:

java.lang.AssertionError: Architecture Violation [Priority: MEDIUM] - Rule 'classes that are interfaces and reside in any package ['io.camunda.zeebe.protocol.record.value..'] or equivalent to io.camunda.zeebe.protocol.record.Record and not equivalent to io.camunda.zeebe.protocol.record.value.ProcessInstanceRelated should be annotated with @Immutable and should be annotated with @ImmutableProtocol and should declare a builder class as ImmutableProtocol#builder= which builds instances of itself' was violated (1 times):
[io.camunda.zeebe.protocol.record.value.deployment.Process] defines builder class ImmutableProtocol(builder=[io.camunda.zeebe.protocol.record.value.ImmutableTimerRecordValue$Builder]) which does not build an object of type assignable to ([io.camunda.zeebe.protocol.record.value.deployment.Process]) but instead builds [io.camunda.zeebe.protocol.record.value.ImmutableTimerRecordValue]

If you were to annotate it with @ImmutableProtocol(builder = String.class), you would get:

java.lang.AssertionError: Architecture Violation [Priority: MEDIUM] - Rule 'classes that are interfaces and reside in any package ['io.camunda.zeebe.protocol.record.value..'] or equivalent to io.camunda.zeebe.protocol.record.Record and not equivalent to io.camunda.zeebe.protocol.record.value.ProcessInstanceRelated should be annotated with @Immutable and should be annotated with @ImmutableProtocol and should declare a builder class as ImmutableProtocol#builder= which builds instances of itself' was violated (1 times):
[io.camunda.zeebe.protocol.record.value.deployment.Process] defines builder class ImmutableProtocol(builder=[java.lang.String]) which does not have a build method

[korthout]

Here's one final option 😄 We only really need the forward declaration of the builder.

Now that's fancy.

So, let me get this straight. The immutable type doesn't have to be directly declared because it was only needed for the ProtocolFactory. The builder is needed because it is used by protocol-jackson, but it does not need the direct type. Right?

java.lang.AssertionError: Architecture Violation [Priority: MEDIUM] - Rule 'classes that are interfaces and reside in any package ['io.camunda.zeebe.protocol.record.value..'] or equivalent to io.camunda.zeebe.protocol.record.Record and not equivalent to io.camunda.zeebe.protocol.record.value.ProcessInstanceRelated should be annotated with @Immutable and should be annotated with @ImmutableProtocol and should declare a builder class as ImmutableProtocol#builder= which builds instances of itself' was violated (1 times):
[io.camunda.zeebe.protocol.record.value.deployment.Process] defines builder class ImmutableProtocol(builder=[io.camunda.zeebe.protocol.record.value.ImmutableTimerRecordValue$Builder]) which does not build an object of type assignable to ([io.camunda.zeebe.protocol.record.value.deployment.Process]) but instead builds [io.camunda.zeebe.protocol.record.value.ImmutableTimerRecordValue]

Love the comprehensiveness of the message 😄 It's a bit of a novel, but clearly describes what is wrong. I like it 👍

[npepinpe]

BTW, when I run the auto format profile locally, nothing happens. Any ideas?

[korthout]

BTW, when I run the auto format profile locally, nothing happens. Any ideas?

It works on my machine. How are you running it? And what do you see?

[npepinpe]

mvn install -PautoFormat -DskipTests -T2C - I also tried calling the plugin directly, same. It runs fine, no warnings/errors, but also nothing to commit.

[korthout]

I don't understand, but it works on my machine...

gh pr checkout 9015
git status

On branch np-drop-protocol-mapping
Your branch is up to date with 'origin/np-drop-protocol-mapping'.

nothing to commit, working tree clean

mvn install -PautoFormat -DskipTests -T2C
git status

On branch np-drop-protocol-mapping
Your branch is up to date with 'origin/np-drop-protocol-mapping'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   protocol/src/main/java/io/camunda/zeebe/protocol/record/ImmutableProtocol.java

no changes added to commit (use "git add" and/or "git commit -a")

If you want I can commit the changes, but I'd rather we find out why it doesn't work for you. Did you have a look at bugs in google-java-format or spotless related to your OS?

[npepinpe]

Not sure what bug I would look for, since it shows everything as working 😄

[korthout]

Have you tried running: mvn process-sources -PcheckFormat -T1C? Important is the profile, which would change which spotless goal we use AND doesn't use ratchetting

[npepinpe]

So I was running this in a separate worktree, and for some reason there it did nothing. When running this in the main one it worked perfectly. I wonder if it's not related to the ratcheting 🤔

[korthout]

That's very interesting. Sounds like it is. You could consider raising a bug for spotless which is doing the ratchetting.

[korthout]

🔧 Please change the return tag into a summary fragment:

/** Returns the builder class for this abstract protocol type */

See #9045

[korthout]

@npepinpe It seems you have some failing tests in zeebe-protocol-test-util https://github.com/camunda/zeebe/runs/5834797919?check_suite_focus=true

[npepinpe]

That's what I get for rebasing 😄

[npepinpe]

I would guess it's a caching issue, since Jenkins doesn't show these error, and the class does exist. Jenkins: 1, GHA: 0 😉

Jenkins only shows flaky tests that are unrelated.

[npepinpe]

It's an issue with the GHA CI. You can see in the tests that it built the protocol module, but then when running the test it downloaded a remote snapshot which doesn't have the latest changes. Please only use the Jenkins tests for validating the PR.

[korthout]

It's an issue with the GHA CI. You can see in the tests that it built the protocol module, but then when running the test it downloaded a remote snapshot which doesn't have the latest changes. Please only use the Jenkins tests for validating the PR.

Are you sure you want to ignore this problem with the GHA CI? This PR will be the one adding a situation where the problem arises. I'd prefer to find a solution to the caching problem before merging. What do you think?

[npepinpe]

I'm going to be unpopular and say that while I appreciate the engagement and motivation you all show to push to migrate to GHA, it's important that it doesn't disrupt us, and if we wanted to work on it we should account for it in our planning instead of simply going ahead and forcing us to make time to migrate to an incomplete solution.

So while yes, we should figure it out, I disagree that we should block PRs. Instead, please open an issue for this which describes the problem, as it is entirely unrelated to the code changes here.

P.S. my tone is very serious I guess, but its not that big a deal. Just in case it doesn't come out correctly via text 😄

[korthout]

Although I disagree, I guess we can allow it since a fix is underway.

[korthout]

Thanks for these changes @npepinpe 🥳 LGTM

[npepinpe]

bors merge

[npepinpe]

Looks like this batch crashed 🤔

bors r-

[npepinpe]

bors r+

[zeebe-bors-camunda]

Build failed (retrying...):

• continuous-integration/jenkins/branch

[npepinpe]

Seems like bors crashed once again...

bors retry

[zeebe-bors-camunda]

Build succeeded:

• Smoke tests on windows-latest
• Smoke tests on ubuntu-latest
• Smoke tests on macos-latest
• continuous-integration/jenkins/branch