-
Notifications
You must be signed in to change notification settings - Fork 965
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
Make ValidationProblem implement Serializable #437
Conversation
|
||
final private String path; | ||
final private ConfigOrigin origin; | ||
final private transient ConfigOrigin origin; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In discussion of #288 it looks like we are assuming ConfigOrigin is serializable (or at least, subclasses of it used in exceptions are). The transient
here can't be the right fix in any case I don't think - we can't reconstruct this field after deserialization, and don't want to lose it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I wasn't sure what to do with that, but looking at your commit here b739f4b, it seems like ConfigOrigin is very intentionally not serializable. Maybe I should follow the lead of ConfigException
, and add custom writeObject
/readObject
methods that call ConfigImplUtil.writeOrigin
and ConfigImplUtil.readOrigin
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, good archaeology, I didn't remember that at all.
I guess you are right - we should copy what ConfigException does with the custom read/write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I've updated it to use the custom serialization. I tried to extract a method to reuse some of the other code but maybe it would've been more straightforward to copy/paste the whole thing ¯_(ツ)_/¯. Let me know
Thanks for working on this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good! Mind adding a unit test or two as appropriate? There's hopefully an existing example for serializing exceptions and test for ValidationProblem could go next to it following a similar pattern. Or test an exception with problems in it.
Sure thing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Great that you have extracted the logic from the way ConfigException
was handled. After adding a missing unit test, this can be merged.
Added a test. Sorry it took almost a year! 😬 |
Is this able to be merged? |
Thanks! sorry for the delay... |
Make ValidationProblem implement Serializable
See #436 for more information.
Just a quick fix to make
ValidationFailed
exceptions actually serializable.