Compatibility fixes for system database sharing - #476
Merged
Conversation
Be liberal in what you accept. On a system database shared by several applications the row may have been written by another SDK, and the Go SDK stores a workflow's error as a non-nullable string — so a workflow that succeeded leaves "" behind where this one writes NULL. Java parsed whatever was there as JSON and threw `MismatchedInputException: No content to map due to end-of-input`, on a workflow that never failed. The other SDKs already read the two the same way. TypeScript coerces on the way in (`row.error ? row.error : null`), and Python never reads the column unless the status says ERROR. Empty, not blank: no SDK writes a non-empty run of whitespace here, so a value that is blank without being empty is content to hand back rather than quietly discard. Applied where the column is read — a workflow's and a step's alike, including the transactional step schema — rather than inside the deserializers, which should not have to know how a column came to be written. What this SDK writes is unchanged and stays conservative: an absent error is NULL.
`DBOS.getWorkflowStatus` threw `IllegalArgumentException: Serialization is not
available` on any row written in a format this runtime has no deserializer for.
Workflow IDs address the whole system database, so a status read reaches rows
another application owns on purpose — and what is wanted from such a row is the
metadata: who owns it, what it is, whether it finished. Losing all of that over
a payload the caller may not have asked for is the wrong trade.
It is not only an interop problem. Two Java applications on one system database,
one configured with a custom serializer and one not, hit exactly the same thing:
`custom_base64` is as unreadable here as `py_pickle` is.
So `SerializationUtil.canDeserialize` asks the question directly — the two
built-in formats, plus whatever a configured custom serializer names — and the
reads that assemble a record from a row skip the payloads when the answer is no,
reporting them as null. A predicate rather than a caught exception: there is no
deserializer to be had, and trying and failing is a slower way to learn it.
Only where the payload is one field of a record — a workflow's status, a
workflow's steps. Everywhere the payload is the answer still throws, because
there is nothing else to hand back: getEvent, a workflow's result, a recv'd
message, a stream, and a recorded step result on replay. That last one matters
most — a step that "returned null" because its output could not be read would
corrupt the run it is replaying — so it has a test of its own.
Three paths read a record but must not accept a null payload, and check for
themselves:
* Running a workflow. The arguments are the point, and one invoked with
arguments it never had is worse than one marked ERROR, so executeWorkflowById
refuses and names the format it would have taken. The error is recorded in
this runtime's own format: one we cannot read is one we cannot write, and
serializing into it would throw and leave the workflow PENDING forever, which
is the hang this refusal exists to prevent.
* Exporting one. An export is imported back, so a payload dropped on the way
out restores a workflow that never had it. Conductor can ask an application
to export a peer's workflow, which is exactly when this bites.
* Importing one. Export and import are a single-SDK affair but not a
single-configuration one, and import re-serializes every payload, so it needs
a serializer for the recorded format as much as export did. It also writes a
null payload straight through as NULL, so a lossy batch would import as an
emptied workflow rather than an error. Checked for the whole batch before the
transaction opens.
Adding applicationName to the WorkflowSchedule and Queue records changed their canonical constructors in place, breaking every caller that built one positionally. The DBOS conductor test app was one, and its CI now fails to compile against the published SDK. Add constructors taking the pre-application-name argument lists, which delegate with a null owner -- the same thing those callers meant, since a null owner records the creating application. The overloads delegate positionally, so the test checks every component round-trips; a mis-ordered delegation would otherwise compile cleanly.
Adding applicationName to ListWorkflowsInput, GetStepAggregatesInput and GetWorkflowAggregatesInput changed their canonical constructors in place. These are records developers build themselves, so add constructors taking the pre-application-name argument lists. They forward a null filter, which covers the caller's own application plus unclaimed rows -- the same default the no-arg constructors already use. GetWorkflowAggregatesInput gained its two components mid-list rather than appended, so that delegation reorders its arguments and additionally forwards groupByApplicationName as false. The read-only output records that also gained the field are left alone; callers receive those rather than construct them. Tests compare each overload against the canonical constructor, which catches a mis-ordered delegation that would otherwise compile cleanly.
A sweep of every public signature #471 changed turned up one more record developers construct: DBOSClient.EnqueueOptions, which gained a trailing applicationName. Add the constructor taking the pre-application-name argument list, forwarding a null owner, which enqueues for the enqueueing application. That sweep found 30 changed public signatures in all. The rest are either read-only outputs callers receive rather than construct (WorkflowStatus, StepInfo, VersionInfo) or internal plumbing: the conductor wire DTOs, the DAO layer, DbContext, SystemDatabase, DBOSExecutor, and the internal packages. Those are left to change in place.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Compatibility fallout from #471, which added application names and system
database sharing. Two independent kinds, in one PR because they share that
cause.
The first three commits were opened before as #475, which was closed unmerged;
they are unchanged here.
Reading rows another application wrote
Sharing a system database means a read can land on a row this runtime did not
write, and two such reads failed outright.
Read an empty error column as no error (
e07f89b). The Go SDK stores aworkflow's error as a non-nullable string, so a workflow that succeeded
leaves
""where this SDK writes NULL. Java parsed that as JSON and threwMismatchedInputException: No content to map due to end-of-input— on aworkflow that never failed. TypeScript and Python already tolerate both forms.
Applied where the column is read, not inside the deserializers, which should
not have to know how a column came to be written. What this SDK writes is
unchanged.
Don't try to deserialize a format we don't recognize (
11897bc).getWorkflowStatusthrewIllegalArgumentException: Serialization is not availablefor any row in a format this runtime has no deserializer for. Aworkflow ID addresses the whole database, so a status read reaches other
applications' rows by design, and what is wanted from one is the metadata.
This is not only cross-SDK: two Java apps on one database, one with a custom
serializer, hit it identically.
SerializationUtil.canDeserializenow asksdirectly, and the reads that assemble a record from a row report unreadable
payloads as null.
Only where the payload is one field of a record. Where the payload is the
answer it still throws —
getEvent, a workflow's result, a recv'd message, astream, and a recorded step result on replay. That last one matters most: a
step that "returned null" because its output could not be read would corrupt
the run it is replaying, so it has a test of its own. Three paths read a record
but must not accept a null payload and check for themselves: running a workflow
(refusing rather than invoking it with arguments it never had), exporting one,
and importing one.
Restoring constructors #471 broke
#471 added
applicationNameto a number of public records. For a Java recordthat changes the canonical constructor in place, so every caller that built
one positionally stopped compiling — including the DBOS conductor test app,
whose CI cannot build against the published SDK
(failing run):
Restore constructors that omit the application name (
2a7aeeb) —WorkflowSchedule,Queue.Restore input constructors that omit the application name (
68297a2) —ListWorkflowsInput,GetStepAggregatesInput,GetWorkflowAggregatesInput.Restore the EnqueueOptions constructor that omits the application name
(
3054cc6) —DBOSClient.EnqueueOptions.Each takes the pre-#471 argument list and forwards a null owner, which is the
same default the existing no-arg constructors already use: the creating or
enqueueing application for the records that record an owner, and "this
application's rows plus unclaimed ones" for the filters.
GetWorkflowAggregatesInputneeded care — its two new components landed mid-list rather than appended, so
that delegation reorders its arguments and passes
groupByApplicationNameasfalse.
Scope
A sweep of every public signature #471 changed (1,614 → 1,678 across all six
published modules) found 30 changed in place. They fall out as:
WorkflowStatus,StepInfo,VersionInfoDbContext,SystemDatabase,DBOSExecutor, and theinternalpackagesInternal APIs deliberately get no compatibility overloads; they are fixed at
their call sites when they change. Only external app code, which cannot be
fixed that way, gets them.
No public interface, enum, or abstract class was modified, so there are no
implementor breaks, and no record component was removed or renamed — every
change was an addition.
transact-clionly gained a new class.Testing
The overloads delegate positionally, where a mis-ordered argument would compile
cleanly and silently scramble fields, so each is compared against the canonical
constructor rather than spot-checked. Both new test classes are container-free.
Verified non-vacuous by mutation: swapping two same-typed arguments in the
GetWorkflowAggregatesInputandEnqueueOptionsdelegations makes thecorresponding test fail.
Note that this does not unblock conductor CI on its own — the test app resolves
dev.dbos:transact:+, so it needs a new prerelease published.