Fix serialization of subclasses #92
Merged
Conversation
This file contains 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
albertchae
force-pushed
the
poc-UnrecognizedPropertyException-anyparentclass-java-class
branch
from
8faaf8c to
988a10e
Compare
This is the reproduction of an error reported by a user. Because StepError contains additional fields to a generic Exception, deserialization fails.
This shows that the issue is not limited to StepError/Exception.
Previously we were deserializing step data based on the T provided by the compiler. However, this could be lossy if the step data was actually a subclass of T with additional fields and is semantically incorrect if we try to deserialize it as the parent class. To remedy this, we need to know what the actual class of the step data was before it was memoized with the Inngest server. We are adding this as an additional field `class` on the JSON when serializing, then reading (and deleting) this field and using it with reflection to deserialize using the proper class. It might make sense to put the field `class` in some kind of meta/extension section of the payload to avoid conflict, but it's highly unlikely a field will legitimately be named class in the user code since it is a reserved keyword in Java. This strategy doesn't entirely work for schema evolution because it's still possible the classes have changed to the point where deserialization would fail. We should follow up here and probably have a fallback mechanism of returning a generic hash if deserialization fails.
albertchae
force-pushed
the
poc-UnrecognizedPropertyException-anyparentclass-java-class
branch
from
a317066 to
644a99e
Compare
albertchae
requested review from
djfarrelly,
darwin67,
jpwilliams and
tonyhb
as code owners
albertchae
changed the title
albertchae
commented
Nov 7, 2024
|
|
||
| val writeableJson = serializedStepData as ObjectNode | ||
| val className = writeableJson.remove("class").asText() | ||
| return mapper.treeToValue(writeableJson, Class.forName(className)) as T |
There was a problem hiding this comment.
We need to figure out how to handle the case where this will fail because the class no longer exists (or renamed) or has been changed enough since serialization that this won't deserialize
albertchae
commented
Nov 7, 2024
Comment on lines
+269
to
+270
| val jsonString = mapper.writeValueAsString(stepData) | ||
| val readOnlyJson = mapper.readTree(jsonString) |
There was a problem hiding this comment.
converting an object to a JSON string only to read that back in to get a JSON node is silly, but replacing this with
val writeableJson = mapper.convertValue(stepData, ObjectNode::class.java)
resulted in a bunch of test failures, so we decided to punt on figuring this out to a follow up
There was a problem hiding this comment.
Sounds good - I think I may have had code like this in an early PoC. There wasn't a great pattern for this in Java and the JSON libraries from what I could tell that didn't result in duplicate parsing/serialization work. I'm find with leaving this for a future improvement.
djfarrelly
approved these changes
Nov 11, 2024
albertchae
deleted the
poc-UnrecognizedPropertyException-anyparentclass-java-class
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Changes:
StepErroras a genericExceptionChecklist
Update documentationRelated
https://inngest.slack.com/archives/C06KB85RXRV/p1728067438991139