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

Add Autoconversion of elegible Objects to Json #125

Merged
merged 8 commits into from
Jan 20, 2025

Conversation

heisluft
Copy link
Contributor

@heisluft heisluft commented Jan 9, 2025

Issue to be solved

A lot of times, a Collection or Map contains Non-Json Values and cannot be serialized directly.

The solution

Expose an interface, JsonWritable for an arbitrary class to represent an instance of itself as a Json-Conforming value via the toJsonValue. The serialization method must yield a String, Number, boolean, Map, Collection, null or another instance of JsonWritable. The process is applied recursively.

Alternatives

Manually loop through these sorts of Collections and Maps, writing each entry, manually converting them through non-standard mechanisms, recursively if necessary.

Risks and Assumptions

The only risk this solution poses is the risk of a StackOverflowError, if an object maliciously returns another instance of its type directly or indirectly in toJsonValue. This case is assumed to be rare at best, as people are not expected to crash the writer on purpose, and returning a value other than the standard values types is an active process, so its unlikely to be done by accident. It is assumed that the overhead resulting from the call to toJsonValue is minimal, as it is just a wrapper for the manual conversions Users already have to do.

@mmastrac
Copy link
Owner

Awesome, thanks for the contribution.

This looks good to me. Could you reorder the test so it is the final one we do before throwing an error? In addition, could you add a test or two to make sure we don't break it in the future?

@heisluft
Copy link
Contributor Author

To clarify - Did you mean I should move the instanceof checks after the isArray() checks?

Also, Food for thought:
JsonWritable may not be the best name for this interface, maybe something like JsonRepresentable could be better?

@mmastrac
Copy link
Owner

To clarify - Did you mean I should move the instanceof checks after the isArray() checks?

Correct -- it should be the final check so that only code that uses the feature will hit that code path.

Also, Food for thought: JsonWritable may not be the best name for this interface, maybe something like JsonRepresentable could be better?

I'm OK with either one. As the author of the PR you can. use whichever you prefer to see. :) I like JsonRepresentable, but ToJson, AsJson, JsonConvertable are all good options too.

@heisluft
Copy link
Contributor Author

heisluft commented Jan 10, 2025

Ok, I've implemented your suggestions and opted for "JsonConvertible".
If there are further changes to make, let me know :)

@mmastrac mmastrac merged commit 45e8740 into mmastrac:master Jan 20, 2025
1 check passed
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

2 participants