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 serialized record closures #23

Merged
merged 2 commits into from Apr 26, 2022
Merged

Conversation

greglook
Copy link
Collaborator

This bug surfaces when Sparkplug tries to serialize functions which are closures defined inside of a defrecord form. The resulting serialized value includes a reference to the record name itself as a "namespace" to require before the function is loaded, but this fails on the executor because records aren't namespaces. This wasn't being caught by the existing (class? (resolve x)) check, because in this case the record was in a namespace that had hyphens in it, and class names must use the underscore version (thanks Java).

To fix this, we need to make the logic here a bit more robust - now it correctly detects class names by swapping out the underscores, and if the parent of that class is itself a namespace, use that instead. The existing behavior should continue ignoring values like clojure.lang.Keyword.

@greglook greglook requested a review from a team April 25, 2022 21:07
Copy link
Contributor

@Kristjansson Kristjansson left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link

@drewinglis drewinglis left a comment

Choose a reason for hiding this comment

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

Good find!

@greglook greglook merged commit 490867b into master Apr 26, 2022
@greglook greglook deleted the fix-serialized-record-closures branch April 26, 2022 00:47
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.

None yet

3 participants