Repository navigation
Allow configuring JsonSerializerSettings for Entity Framework persistence providers - #1450
Conversation
…ence providers - Add constructor overloads to `EntityFrameworkPersistenceProvider` and `LargeDataOptimizedEntityFrameworkPersistenceProvider` accepting an `Action<JsonSerializerSettings>`, e.g. to set a `SerializationBinder`. - Replace the static settings in `ExtensionMethods` with a per-provider instance; defaults are unchanged. - Add `SerializerSettingsFixture` covering default output, allowed round-trip, rejected type and unchanged default behavior. - Document restricting deserialized types in `docs/persistence.md`. Closes danielgerlag#1178 Co-Authored-By: Claude Opus 5.5 <claude-opus-5-5@anthropic.com>
|
Opened #1451 for the observation in the last section: an instance that fails to load (including one rejected by a binder configured via this change) is re-queued every poll interval indefinitely. It includes a minimal repro against 3.21.0. |
danielgerlag
left a comment
There was a problem hiding this comment.
Thanks so much for this, @thrisikos-exida, and welcome! It's a really clean first contribution: opt-in, no change to the stored JSON or default behaviour, overloads instead of optional params so binaries don't break, plus tests and docs. I pulled it locally. It builds clean and the Sqlite (14/14) and integration (45/45) suites pass. I also confirmed that the "pre-middlewares for sync workflows" failure happens on master too. I also ran an end-to-end workflow (ForEach/If/Delay/WaitFor) with the allow-list binder from the docs and it completed fine.
A few small, optional suggestions:
- Persistence providers are registered as transient, so the configure action runs on every resolution. In the docs example, could you create the binder once outside the lambda (e.g.
var binder = new AllowListBinder();) and mention that it must be thread-safe? Otherwise the binder's type cache is rebuilt each time. - Maybe add a short note or example that generic collections like
List<string>on the data class must be allowed too (the example binder currently rejects them), and add theNewtonsoft.Json/Newtonsoft.Json.Serializationusings to the snippet. - Optionally, a test covering
LargeDataOptimizedEntityFrameworkPersistenceProviderwith a binder would be nice.
Thanks also for opening #1451 about poisoned instances being re-polled; we'll track that separately. I've approved the CI run.
- Create the binder once outside the provider factory in the `docs/persistence.md` example and note that it must be thread-safe. - Allow `List<string>` in the example binder and add the required usings. - Run the binder tests in `SerializerSettingsFixture` against `LargeDataOptimizedEntityFrameworkPersistenceProvider` as well. Co-Authored-By: Claude Opus 5.5 <claude-opus-5-5@anthropic.com>
|
Thanks @danielgerlag, I was trying to keep it as clean as possible, and I appreciate all the feedback! I've pushed a follow-up commit addressing all three:
For the CI run, besides the known pre-middlewares failure, Integration-Tests failed in |
…ly check Co-Authored-By: Claude Opus 5.5 <claude-opus-5-5@anthropic.com>
|
After reviewing the doc changes, we realized we could clarify how the example binder treats generic and array types. It now only trusts non-generic, non-array types from the Workflow Core assembly, and the docs note that allowing types by assembly also trusts the concrete types their members declare. |
danielgerlag
left a comment
There was a problem hiding this comment.
Thanks for the quick follow-ups, @thrisikos-exida! The docs and tests look great. Approving.
Describe the change
The Entity Framework persistence providers serialize workflow data, persistence data, outcomes, event data, subscription data and extension attributes with a private, static
JsonSerializerSettingsusingTypeNameHandling.All.Because there is no way to customize those settings, consumers cannot install a
SerializationBinderto restrict which types may be instantiated when a row is read back.This adds an opt-in constructor overload to
EntityFrameworkPersistenceProviderandLargeDataOptimizedEntityFrameworkPersistenceProviderthat accepts anAction<JsonSerializerSettings>, applied on top of the existing defaults.This is defense in depth: exploiting it requires write access to the persistence store, but with a binder configured, a tampered
$typeis rejected with aJsonSerializationExceptioninstead of instantiating an arbitrary type.Closes #1178
Related: #1280
Describe your implementation or design
ExtensionMethodsstaysinternal; the static field is replaced by an internalCreateSerializerSettings(Action<JsonSerializerSettings>)factory, and the mapping methods take the settings instance as a parameter.null, so default behavior and the stored JSON format are unchanged.UsePersistenceafter the usualUseSqlite/UseSqlServer/etc. call, which keeps the purger registration.Convenience overloads on the
Use*extension methods could follow in a separate PR if you prefer that shape.docs/persistence.mdwith an allow-list binder example.Tests
Added
SerializerSettingsFixtureinWorkflowCore.Tests.Sqlite:$typewas overwritten with a disallowed type throwsJsonSerializationException;Updated
ExtensionMethodsFixturefor the new parameter.WorkflowCore.Tests.Sqlite(14/14) andWorkflowCore.IntegrationTests(45/45) pass on all target frameworks.WorkflowCore.UnitTestshas one failure,Should run pre-middlewares for sync workflows, which also fails onmasterwithout this change.Provider test suites that need Docker were not run.
Breaking change
No. Existing constructors and the default serializer settings are unchanged; existing persisted data reads exactly as before.
Additional context
TypeNameHandlingsettings; they could adopt the same pattern in follow-up PRs.Happy to open an issue for that if useful.
AI-assisted: Claude Opus 5.5 (
claude-opus-5-5)