[Fix #1690] Add metadata() method to WorkflowInstance

[fjtirado]

Fix #1690

All metadata operations are now optional (since metadata is not really on the spec and should not be mandatory)

[Copilot]

🟡 Changes recommended

Restore constant compatibility and expose metadata through the persistence-facing contract.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds metadata access APIs to workflow instances for persistence integrations.

Changes:

• Introduces MetadataOperations for metadata retrieval and mutation.
• Exposes metadata operations through WorkflowInstance and implements them in WorkflowMutableInstance.
• Narrows CLOUD_EVENT_IDS visibility.

File summaries

File	Summary
impl/persistence/api/src/main/java/io/serverlessworkflow/impl/persistence/AbstractPersistenceInstanceWriter.java	Changes cloud-event constant visibility.
impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowMutableInstance.java	Implements metadata access and typed removal.
impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowInstance.java	Extends the metadata operations API.
impl/core/src/main/java/io/serverlessworkflow/impl/MetadataOperations.java	Defines metadata operations.

Review details

Suppressed comments (1)

impl/core/src/main/java/io/serverlessworkflow/impl/MetadataOperations.java:29

• metadata() is only declared on MetadataOperations, which is inherited by WorkflowInstance, but persistence callbacks receive WorkflowContextData whose instanceData() type is WorkflowInstanceData. Consequently a persistence implementation cannot call this new method through the public contract (unlike findMetadata), so the stated persistence use case still requires an implementation-specific cast. Expose the map from WorkflowInstanceData or change the persistence context contract so writers can access it directly.

  Map<String, Object> metadata();

• Files reviewed: 4/4 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Critical API compatibility issues and a destructive typed-removal bug remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowMutableInstance.java:459

• The entry is removed before its type is checked, so removeMetadata("key", WrongClass.class) returns Optional.empty() but still deletes the metadata. This makes a failed typed removal destructive and differs from findMetadata's type-filtering behavior; check the type before removing (using an atomic conditional removal) so mismatched calls leave the entry intact.

    Object value = additionalObjects.remove(key);
    return clazz.isInstance(value) ? Optional.of(clazz.cast(value)) : Optional.empty();

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review details

Suppressed comments (3)

impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowInstance.java:72

• This new abstract method likewise breaks external WorkflowInstance implementations. The existing findMetadata and removeMetadata(String) methods are sufficient for a compatibility-preserving default implementation, so this should be defaulted rather than requiring every consumer to update immediately.

   * Remove metatadata key if present and return a non empty key if the object being deleted match

impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowInstanceData.java:37

• Because WorkflowInstanceData is a public extension point consumed by persistence modules, making this new method abstract breaks every existing implementation (and makes existing WorkflowInstance implementations fail to compile) even though those implementations previously had no metadata to expose. Make this a default method returning an empty map for compatibility; WorkflowMutableInstance can continue to override it with the actual metadata.

  Map<String, Object> metadata();

impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowMutableInstance.java:459

• This removes the entry before checking its type, so a call such as removeMetadata("key", String.class) silently deletes an existing Integer value and returns empty. That is destructive on a type mismatch and inconsistent with findMetadata, which leaves mismatched values intact; check the value first and conditionally remove the same key/value.

  public <T> Optional<T> removeMetadata(String key, Class<T> clazz) {
    Object value = additionalObjects.remove(key);
    return clazz.isInstance(value) ? Optional.of(clazz.cast(value)) : Optional.empty();

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowInstance.java:75

• This new abstract overload likewise breaks existing implementations of the public WorkflowInstance interface, although its behavior can be composed from the existing findMetadata and removeMetadata methods. Make it a default method to preserve compatibility for existing implementations.

  <T> Optional<T> removeMetadata(String key, Class<T> clazz);

impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowInstanceData.java:37

• Because WorkflowInstanceData is a public interface consumed by persistence adapters, adding this abstract method makes existing external implementations fail to compile (and can cause AbstractMethodError in already compiled implementations). Preserve compatibility with a default empty map, or explicitly treat this as a breaking API release.

  Map<String, Object> metadata();

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review details

Suppressed comments (3)

impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowInstance.java:73

• The new Javadoc has a subject–verb agreement error: the object "match" should be "matches" the specified class type.

   * match the specified class type

impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowInstance.java:75

• This unrelated overload is also a new abstract method on the public WorkflowInstance interface, so existing custom implementations will no longer compile (and old binaries can fail when the method is invoked). Make it a default implemented via the existing findMetadata/removeMetadata methods, or omit it from this change.

  default <T> Optional<T> removeMetadata(String key, Class<T> clazz) {

impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowInstanceData.java:37

• WorkflowInstanceData is a public interface, so adding this abstract member breaks source (and can cause AbstractMethodError for binary) compatibility for every external implementation that does not override it. Make the new accessor a default returning an empty map, while WorkflowMutableInstance continues to provide the real metadata, or otherwise introduce this as a compatibility-preserving API change.

  Map<String, Object> metadata();

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowMutableInstance.java:464

• The new public metadata accessor has no test coverage. Add a focused test that adds metadata through addMetadataIfAbsent, verifies it is returned by metadata(), and confirms the exposed map cannot be mutated; this protects the persistence-facing contract.

  public Map<String, Object> metadata() {
    return Collections.unmodifiableMap(additionalObjects);

• Files reviewed: 3/3 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review details

Suppressed comments (1)

impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowInstanceData.java:37

• WorkflowInstanceData is a public interface consumed by persistence implementations, so adding this abstract method is a source-compatibility break for every downstream implementation and can produce AbstractMethodError for already-compiled implementations. Since this is an additive accessor, preserve compatibility with a default empty map (while allowing implementations such as WorkflowMutableInstance to override it), or make this change only as part of an intentional major-version break.

  Map<String, Object> metadata();

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The default removal implementation must avoid a lookup/removal race that can delete a replacement value.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowInstanceData.java:40

• The new persistence-facing contract silently returns an empty map for any existing/custom WorkflowInstanceData implementation that does not override metadata() (including implementations that already expose metadata through findMetadata), so persistence can lose metadata without a compile-time signal. Make this method abstract so every implementation must provide the complete metadata view, as intended for this API.

  default Map<String, Object> metadata() {
    return Map.of();

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

No unresolved review issues were identified.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[gmunozfe]

Looks good to me, well done @fjtirado