[Fix #1266] Marshaling improvements

[fjtirado]

Fix #1266

[Copilot]

Pull request overview

This PR addresses #1266 by relocating marshaller SPI implementations into the model-defining modules and enhancing the core marshalling infrastructure to better select appropriate CustomObjectMarshaller implementations (including support for polymorphic reads).

Changes:

• Removed the persistence jackson-marshaller module and registered marshallers via META-INF/services inside impl-model and experimental-model.
• Updated the marshalling SPI (CustomObjectMarshaller) and core buffer/selection logic to support class-aware custom unmarshalling.
• Added serialization round-trip tests for both the Jackson-based model and the experimental Java model.

Reviewed changes

Copilot reviewed 26 out of 33 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
impl/persistence/pom.xml	Removes the jackson-marshaller module from the persistence reactor.
impl/persistence/jackson-marshaller/src/main/resources/META-INF/services/io.serverlessworkflow.impl.marshaller.CustomObjectMarshaller	Removes SPI registration from the deleted persistence marshaller module.
impl/persistence/jackson-marshaller/pom.xml	Deletes the persistence Jackson marshaller artifact/module.
impl/model/src/test/java/io/serverlessworkflow/impl/model/jackson/JacksonModelSerializationTest.java	Adds round-trip tests using the new core buffer factory.
impl/model/src/test/java/io/serverlessworkflow/impl/model/jackson/Employee.java	Adds a record used to validate generic object marshalling via Jackson.
impl/model/src/main/resources/META-INF/services/io.serverlessworkflow.impl.marshaller.CustomObjectMarshaller	Registers model-local Jackson marshallers via ServiceLoader.
impl/model/src/main/java/io/serverlessworkflow/impl/model/jackson/JacksonObjectMarshaller.java	Adds a Jackson-backed “catch-all” object marshaller.
impl/model/src/main/java/io/serverlessworkflow/impl/model/jackson/JacksonModelMarshaller.java	Adds a JacksonModel-specific marshaller with NULL normalization.
impl/model/src/main/java/io/serverlessworkflow/impl/model/jackson/JacksonModel.java	Adds equals/hashCode for stable round-trip equality.
impl/model/src/main/java/io/serverlessworkflow/impl/model/jackson/AbstractJacksonMarshaller.java	Refactors Jackson marshalling logic into a reusable abstract base.
impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/WorkflowOutputBuffer.java	Introduces the output buffer API in core.
impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/WorkflowInputBuffer.java	Introduces the input buffer API in core.
impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/WorkflowBufferFactory.java	Introduces the buffer factory abstraction in core.
impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/Type.java	Adds internal type tags used by the buffer encoding.
impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/TaskStatus.java	Adds a task-status enum in the marshaller package.
impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/MarshallingUtils.java	Adds utility logic to choose the “best” custom marshaller.
impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/DefaultOutputBuffer.java	Adds a default DataOutputStream-backed output buffer implementation.
impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/DefaultInputBuffer.java	Adds a default DataInputStream-backed input buffer implementation.
impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/DefaultBufferFactory.java	Sorts ServiceLoader-provided marshallers by priority.
impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/CustomObjectMarshaller.java	Updates the SPI to support class-aware reads.
impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/AbstractOutputBuffer.java	Updates custom-object encoding to persist the runtime class and use MarshallingUtils.
impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/AbstractInputBuffer.java	Updates custom-object decoding to use MarshallingUtils and class-aware reads.
experimental/model/src/test/java/io/serverlessworkflow/impl/model/func/Person.java	Adds a serializable test record for the experimental Java model.
experimental/model/src/test/java/io/serverlessworkflow/impl/model/func/JavaModelSerializationTest.java	Adds round-trip test for experimental Java model marshalling.
experimental/model/src/test/java/io/serverlessworkflow/impl/model/func/Address.java	Adds a serializable nested record used by the experimental test.
experimental/model/src/main/resources/META-INF/services/io.serverlessworkflow.impl.marshaller.CustomObjectMarshaller	Registers experimental-model marshallers via ServiceLoader.
experimental/model/src/main/java/io/serverlessworkflow/impl/model/func/JavaModelMarshaller.java	Adds a JavaModel marshaller delegating to underlying Java objects.
experimental/model/src/main/java/io/serverlessworkflow/impl/model/func/JavaModel.java	Adds equals/hashCode for stable round-trip equality.
experimental/model/src/main/java/io/serverlessworkflow/impl/model/func/DefaultJavaObjectMarshaller.java	Adds a default Java-serialization marshaller for Serializable.
experimental/model/pom.xml	Adds test/logging dependencies for the experimental model module.

Comments suppressed due to low confidence (4)

impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/AbstractOutputBuffer.java:117

• writeClass() uses Class#getCanonicalName(), but readClass() later calls Class.forName(...), which expects the binary name (Outer$Inner) and also fails for local/anonymous classes where getCanonicalName() returns null. Since writeCustomObject() now persists object.getClass(), this can cause NPEs or ClassNotFoundException for nested/local classes. Persist Class#getName() (binary name) instead.
  impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/MarshallingUtils.java:125
• getCustomMarshaller(...) can return a non-deterministic result when multiple marshallers are assignable from the same clazz (e.g., Serializable and Object both match). Because several default marshallers in this PR have the same priority, the chosen marshaller may depend on ServiceLoader ordering. Consider preferring the most specific assignable getObjectClass() (closest ancestor), and/or adding a deterministic tie-breaker when priorities are equal.
  impl/persistence/pom.xml:15
• This change removes the jackson-marshaller module from the persistence reactor, but serverlessworkflow-persistence-jackson-marshaller is still referenced elsewhere (e.g., impl/pom.xml, impl/persistence/tests/pom.xml, impl/test/pom.xml). As-is, the build will fail (missing artifact) and it also contradicts the linked issue’s backward-compatibility plan. Either keep/publish a compatibility artifact/module, or update all remaining dependencies to the new module that provides the SPI implementation.

    <modules> 
      <module>mvstore</module>
      <module>bigmap</module>
      <module>api</module>
      <module>tests</module>

impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/CustomObjectMarshaller.java:25

• CustomObjectMarshaller’s SPI method signature changed from read(WorkflowInputBuffer) to read(WorkflowInputBuffer, Class<? extends T>). This is a breaking change for any third-party marshaller implementations already compiled against the previous interface (binary incompatibility / AbstractMethodError). If backward compatibility is required, consider keeping the old method and adding the new overload as a default method (or providing an adapter base class) so existing providers keep working.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 29 out of 36 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (4)

impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/AbstractOutputBuffer.java:117

• writeClass(object.getClass()) ultimately writes Class#getCanonicalName(). getCanonicalName() can be null (anonymous/local classes) and is not a valid Class.forName(...) name for member classes, which will break deserialization in readClass(). Prefer writing Class#getName() (binary name) or handle null explicitly.
  impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/MarshallingUtils.java:104
• Spelling: this Javadoc uses "marshaler" but the codebase uses "marshaller" (e.g., CustomObjectMarshaller). Align the wording to avoid confusion and improve searchability.
  impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/MarshallingUtils.java:124
• Spelling: the exception message says "marshaler" but the rest of the API uses "marshaller" (double 'l'). Updating the message will make errors clearer and consistent with the type name.
  impl/model/src/main/java/io/serverlessworkflow/impl/model/jackson/AbstractJacksonMarshaller.java:35
• This method signature uses Object rather than T, which defeats the generic type-safety of CustomObjectMarshaller<T> and can allow writing the wrong type without compile-time errors. Consider changing the parameter to T object (and optionally adding a protected helper if you want to share implementation).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 29 out of 36 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

impl/model/src/main/java/io/serverlessworkflow/impl/model/jackson/AbstractJacksonMarshaller.java:35

• CustomObjectMarshaller<T> requires write(WorkflowOutputBuffer, T), but this implementation declares write(..., Object). With @Override this will not override the interface method for generic T and is likely to fail compilation. Change the parameter type to T so subclasses like AbstractJacksonMarshaller<JacksonModel> correctly implement the contract.
  impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/MarshallingUtils.java:104
• Javadoc uses the misspelling "marshaler" / "marshalers"; elsewhere the project uses "marshaller(s)" (and the type name is CustomObjectMarshaller). Updating the wording improves readability/searchability.
  impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/AbstractOutputBuffer.java:116
• In writeObject, the Byte branch writes a LONG (writeLong(number)) even though the type marker is Type.BYTE, which will misalign the stream for subsequent reads. Also, writeInstant() writes epoch seconds (getEpochSecond()), while AbstractInputBuffer.readInstant() reads epoch millis (ofEpochMilli(readLong())), so Instants round-trip incorrectly. These should use consistent encodings (byte->writeByte, instant->both seconds or both millis).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 29 out of 36 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/MarshallingUtils.java:104

• Spelling: use the term "marshaller" consistently (not "marshaler") in this Javadoc so it matches the type name CustomObjectMarshaller and the rest of the codebase terminology.
  impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/MarshallingUtils.java:108
• Spelling: "marshalers" should be "marshallers" (double 'l') to match the existing naming throughout the module.
  impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/MarshallingUtils.java:124
• Spelling: error message says "marshaler"; rename to "marshaller" for consistency and searchability.
  impl/core/src/main/java/io/serverlessworkflow/impl/marshaller/AbstractOutputBuffer.java:117
• writeClass() uses Class.getCanonicalName(), but the reader side (Class.forName(...)) expects a binary name. This breaks for nested classes (Outer.Inner vs Outer$Inner), arrays (e.g. String[]), and can even be null for anonymous/local classes. Since writeCustomObject() now persists object.getClass(), these cases become more likely. Prefer writing objectClass.getName() (binary name) and reading it back with Class.forName(...) to ensure round-trip works for all JVM classes.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ricardozanini]

THANKS!