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.

Sign up for GitHub

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

Jump to bottom

Make ValidationProblem implement Serializable #437

Merged
merged 4 commits into from
Mar 29, 2018
Merged

Make ValidationProblem implement Serializable #437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ce6d096
Make ValidationProblem implement Serializable
Nov 2, 2016
3448ef5
Add custom serialization to ValidationProblem
Nov 2, 2016
3d44d90
Merge remote-tracking branch 'upstream/master'
iantabolt Oct 6, 2017
2692aa7
Add test case for serializing ValidationFailed
iantabolt Oct 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 31 additions & 10 deletions config/src/main/java/com/typesafe/config/ConfigException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,29 +58,36 @@ private void writeObject(java.io.ObjectOutputStream out) throws IOException {
ConfigImplUtil.writeOrigin(out, origin);
}

private void readObject(java.io.ObjectInputStream in) throws IOException,
ClassNotFoundException {
in.defaultReadObject();
ConfigOrigin origin = ConfigImplUtil.readOrigin(in);
// For deserialization - uses reflection to set the final origin field on the object
private static <T> void setOriginField(T hasOriginField, Class<T> clazz,
ConfigOrigin origin) throws IOException {
// circumvent "final"
Field f;
try {
f = ConfigException.class.getDeclaredField("origin");
f = clazz.getDeclaredField("origin");
} catch (NoSuchFieldException e) {
throw new IOException("ConfigException has no origin field?", e);
throw new IOException(clazz.getSimpleName() + " has no origin field?", e);
} catch (SecurityException e) {
throw new IOException("unable to fill out origin field in ConfigException", e);
throw new IOException("unable to fill out origin field in " +
clazz.getSimpleName(), e);
}
f.setAccessible(true);
try {
f.set(this, origin);
f.set(hasOriginField, origin);
} catch (IllegalArgumentException e) {
throw new IOException("unable to set origin field", e);
} catch (IllegalAccessException e) {
throw new IOException("unable to set origin field", e);
}
}

private void readObject(java.io.ObjectInputStream in) throws IOException,
ClassNotFoundException {
in.defaultReadObject();
ConfigOrigin origin = ConfigImplUtil.readOrigin(in);
setOriginField(this, ConfigException.class, origin);
}

/**
* Exception indicating that the type of a value does not match the type you
* requested.
Expand Down Expand Up @@ -310,10 +317,10 @@ public NotResolved(String message) {
* {@link ConfigException.ValidationFailed} exception thrown from
* <code>checkValid()</code> includes a list of problems encountered.
*/
public static class ValidationProblem {
public static class ValidationProblem implements Serializable {

final private String path;
final private ConfigOrigin origin;
final private transient ConfigOrigin origin;
Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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.

Copy link
Contributor Author

@iantabolt iantabolt Nov 2, 2016 •
edited
Loading

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

final private String problem;

public ValidationProblem(String path, ConfigOrigin origin, String problem) {
Expand Down Expand Up @@ -347,6 +354,20 @@ public String problem() {
return problem;
}

// We customize serialization because ConfigOrigin isn't
// serializable and we don't want it to be
private void writeObject(java.io.ObjectOutputStream out) throws IOException {
out.defaultWriteObject();
ConfigImplUtil.writeOrigin(out, origin);
}

private void readObject(java.io.ObjectInputStream in) throws IOException,
ClassNotFoundException {
in.defaultReadObject();
ConfigOrigin origin = ConfigImplUtil.readOrigin(in);
setOriginField(this, ValidationProblem.class, origin);
}

@Override
public String toString() {
return "ValidationProblem(" + path + "," + origin + "," + problem + ")";
Expand Down
15 changes: 15 additions & 0 deletions config/src/test/scala/com/typesafe/config/impl/ValidationTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,21 @@ class ValidationTest extends TestUtils {
checkValidationException(e, expecteds)
}

@Test
def validationFailedSerializable(): Unit = {
// Reusing a previous test case to generate an error
val reference = parseConfig("""{ a : [{},{},{}] }""")
val conf = parseConfig("""{ a : 42 }""")
val e = intercept[ConfigException.ValidationFailed] {
conf.checkValid(reference)
}

val expecteds = Seq(WrongType("a", 1, "list", "number"))

val actual = checkSerializableNoMeaningfulEquals(e)
checkValidationException(actual, expecteds)
}

@Test
def validationAllowsListOverriddenWithSameTypeList() {
val reference = parseConfig("""{ a : [1,2,3] }""")
Expand Down