Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix step state incorrect type when returning null from steps #96

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

KiKoS0
Copy link
Collaborator

@KiKoS0 KiKoS0 commented Jan 17, 2025

Summary

Should fix the step state incorrect type that was being
thrown when a step returns a null result.

Changes:

  • Allow returning a null value from the data field of a step for step.run.

  • Make sure the data field is a null json node for step.sleep. Here's the memoized state payload sent to the JS sdk which the Kotlin code is closely following:

     await step.sleep("sleep", 100);
     await step.run("null", () => null);
     {
       "steps": {
         "c3ca5f787365eae0dea86250e27d476406956478": null,
         "244fb75b19415c9ee4f143b34b4b241236fb63f5": {
           "data": null
         }
       }
     }

Checklist

  • Update documentation
  • Added unit/integration tests

Related

https://inngest.slack.com/archives/C06KB85RXRV/p1736536607871969

Should fix the `step state incorrect type` that was being
thrown when a step returns a null result.
@KiKoS0 KiKoS0 self-assigned this Jan 17, 2025
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just a showcase poc of the bug, I'll be dropping the commit before merging.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it failing before and working after 👍

image

Comment on lines -106 to -108
if (stepResult != null) {
return stepResult
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like we inadvertently introduced this bug with #19

Copy link
Collaborator Author

@KiKoS0 KiKoS0 Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i believe it was always present because even the older version check returns false when type checking a null result, because null can never be a specific type:

val stepResult = state.getState<T>(hashedId)
if (stepResult is T) {

i made sure it's the case with this generic example:

inline fun <reified T> isOfType(value: Any?): Boolean {
    return value is T
}

fun main() {
    println(isOfType<String>(null)) 
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome, thanks for verifying

stepResult.has(fieldName) -> deserializeStepData(stepResult.get(fieldName), type)
stepResult.has("error") -> throw mapper.treeToValue(stepResult.get("error"), StepError::class.java)
// NOTE - Sleep steps will be stored as null
stepResult is NullNode -> null
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it failing before and working after 👍

image

@KiKoS0 KiKoS0 marked this pull request as ready for review January 17, 2025 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants