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.

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

Introduce new immutable protocol #8945

Merged
merged 5 commits into from
Mar 25, 2022
Merged

Conversation

npepinpe
Copy link
Member

Description

Introduces a new immutable protocol directly under protocol, which annotates the types directly. This has several advantages:

  • We can now easily recursively copy the types (except for Record, which is a special case due to its generic typing)
  • It's less error prone thanks to an ArchUnit test guaranteeing we don't forget to annotate the types
  • It's less overhead for developers when adding new types
  • It allows us to easily collect metadata about the types via reflection using annotations

This is the first step for #8837 - the next step will remove the immutable variants in protocol-jackson and replace them with this.

Related issues

related to #8837

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.

@npepinpe npepinpe changed the base branch from main to 8837-record-copyof March 21, 2022 14:00
@npepinpe npepinpe requested a review from korthout March 21, 2022 14:43
Copy link
Member

@korthout korthout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work again @npepinpe

As I've reviewed most of this before, I haven't got much to say about it anymore.

LGTM 👍

protocol-asserts/pom.xml Outdated Show resolved Hide resolved
Base automatically changed from 8837-record-copyof to main March 22, 2022 10:58
@npepinpe npepinpe force-pushed the 8837-immutable-protocol branch 2 times, most recently from 13ccb2e to 5ed71b9 Compare March 22, 2022 13:30
@npepinpe
Copy link
Member Author

bors merge

@npepinpe
Copy link
Member Author

bors r-

@ghost
Copy link

ghost commented Mar 22, 2022

Canceled.

@npepinpe
Copy link
Member Author

npepinpe commented Mar 22, 2022

I think it might be better to postpone this for the next release, primarily because this will cause additional effort for the Operate team which is already using this, and zeebe-process-test. It might be better instead to do it for the next release where they have time to adapt. WDYT?

I will check in with them, and if they tell me it's fine we can go ahead anyway, but let's see.

@npepinpe npepinpe linked an issue Mar 24, 2022 that may be closed by this pull request
Adds immutable protocol generation to the Zeebe record protocol. This is
done via an annotation process, Immutables, which requires two
annotations on each leaf types:

- `@Value.Immutable`
- `@ImmutableProtocol`

All leaf types - that is, types expected to have a single, direct, concrete
implementations - need to be annotated with both values to ensure that
there is always one immutable variant available, especially for
deserialization.

Meta-types such as `ProcessInstanceRelated`, for example, do not need to
be annotated but can be left as is. This may of course change in the
future.

This commit is the first step to removing the immutable protocol from
protocol-jackson, but for now both are existing at the same time.
Generates flat assertions for each protocol type as opposed to
hierarchical ones, omitting generated immutable variants. This should
solve some compile time issues related to code generation, and simplify
usage. No more need to use RecordValueWithVariables.hasVariables when
dealing with a JobRecordValue - instead you can keep using
`JobRecordValueAssert.hasVariables`.
Adds an ArchUnit tests which ensures all protocol leaf types are
annotated, guaranteeing an immutable variant will be generated for each
of these types.

At the moment, this means `Record` and type under `record.value` package
or one of its sub-packages, except `ProcessInstanceRelated`.
@npepinpe
Copy link
Member Author

bors merge

@zeebe-bors-camunda
Copy link
Contributor

Build succeeded:

@zeebe-bors-camunda zeebe-bors-camunda bot merged commit d325919 into main Mar 25, 2022
@zeebe-bors-camunda zeebe-bors-camunda bot deleted the 8837-immutable-protocol branch March 25, 2022 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Protocol Jackson improvements
3 participants