kfaraz commented on code in PR #12612:
URL: https://github.com/apache/druid/pull/12612#discussion_r890804596
##########
processing/src/main/java/org/apache/druid/jackson/JacksonModule.java:
##########
@@ -43,28 +47,37 @@ public void configure(Binder binder)
}
@Provides @LazySingleton @Json
- public ObjectMapper jsonMapper()
+ public ObjectMapper jsonMapper(Properties props)
{
- return new DefaultObjectMapper();
+ return new DefaultObjectMapper(getServiceName(props));
}
/**
* Provides ObjectMapper that suppress serializing properties with null
values
*/
@Provides @LazySingleton @JsonNonNull
- public ObjectMapper jsonMapperOnlyNonNullValue()
+ public ObjectMapper jsonMapperOnlyNonNullValue(Properties props)
{
- return new
DefaultObjectMapper().setSerializationInclusion(JsonInclude.Include.NON_NULL);
+ return new
DefaultObjectMapper(getServiceName(props)).setSerializationInclusion(JsonInclude.Include.NON_NULL);
}
@Provides @LazySingleton @Smile
- public ObjectMapper smileMapper()
+ public ObjectMapper smileMapper(Properties props)
Review Comment:
Should we keep the older zero argument method too for ease of use in tests?
It wouldn't be a provider method anymore, just a utility.
##########
processing/src/main/java/org/apache/druid/jackson/DefaultObjectMapper.java:
##########
@@ -61,11 +77,54 @@ public DefaultObjectMapper(JsonFactory factory)
configure(MapperFeature.ALLOW_FINAL_FIELDS_AS_MUTATORS, false);
configure(SerializationFeature.INDENT_OUTPUT, false);
configure(SerializationFeature.FLUSH_AFTER_WRITE_VALUE, false);
+
+ addHandler(new DefaultDeserializationProblemHandler(serviceName));
}
@Override
public ObjectMapper copy()
{
return new DefaultObjectMapper(this);
}
+
+ /**
+ * A custom implementation of {@link #DeserializationProblemHandler} to add
custom error message so
+ * that users know how to troubleshoot unknown type ids.
+ */
+ static class DefaultDeserializationProblemHandler extends
DeserializationProblemHandler
+ {
+ private final String serviceName;
+
+ public DefaultDeserializationProblemHandler(@Nullable String serviceName)
+ {
+ this.serviceName = serviceName == null ? "unknown" : serviceName;
+ }
+
+ @VisibleForTesting
+ String getServiceName()
+ {
+ return serviceName;
+ }
+
+ @Override
+ public JavaType handleUnknownTypeId(DeserializationContext ctxt,
+ JavaType baseType, String subTypeId,
TypeIdResolver idResolver,
+ String failureMsg)
+ throws IOException
+ {
+ String msg = String.format("Please make sure to load all the necessary
extensions and jars " +
+ "with type '%s' on '%s' service. " +
+ "Could not resolve type id '%s' as a subtype of %s",
+ subTypeId, serviceName, subTypeId,
ClassUtil.getTypeDescription(baseType));
+ throw InvalidTypeIdException.from(ctxt.getParser(),
extraFailureMessage(msg, failureMsg), baseType, subTypeId);
+ }
+
+ private String extraFailureMessage(String msgBase, @Nullable String extra)
+ {
+ if (extra == null) {
+ return msgBase;
+ }
+ return msgBase + " " + extra;
Review Comment:
super nit: style choice
```suggestion
return extra == null ? msgBase : msgBase + " " + extra;
```
##########
processing/src/main/java/org/apache/druid/jackson/DefaultObjectMapper.java:
##########
@@ -61,11 +77,54 @@ public DefaultObjectMapper(JsonFactory factory)
configure(MapperFeature.ALLOW_FINAL_FIELDS_AS_MUTATORS, false);
configure(SerializationFeature.INDENT_OUTPUT, false);
configure(SerializationFeature.FLUSH_AFTER_WRITE_VALUE, false);
+
+ addHandler(new DefaultDeserializationProblemHandler(serviceName));
}
@Override
public ObjectMapper copy()
{
return new DefaultObjectMapper(this);
}
+
+ /**
+ * A custom implementation of {@link #DeserializationProblemHandler} to add
custom error message so
+ * that users know how to troubleshoot unknown type ids.
+ */
+ static class DefaultDeserializationProblemHandler extends
DeserializationProblemHandler
+ {
+ private final String serviceName;
+
+ public DefaultDeserializationProblemHandler(@Nullable String serviceName)
+ {
+ this.serviceName = serviceName == null ? "unknown" : serviceName;
+ }
+
+ @VisibleForTesting
+ String getServiceName()
+ {
+ return serviceName;
+ }
+
+ @Override
+ public JavaType handleUnknownTypeId(DeserializationContext ctxt,
+ JavaType baseType, String subTypeId,
TypeIdResolver idResolver,
+ String failureMsg)
+ throws IOException
+ {
+ String msg = String.format("Please make sure to load all the necessary
extensions and jars " +
+ "with type '%s' on '%s' service. " +
+ "Could not resolve type id '%s' as a subtype of %s",
+ subTypeId, serviceName, subTypeId,
ClassUtil.getTypeDescription(baseType));
+ throw InvalidTypeIdException.from(ctxt.getParser(),
extraFailureMessage(msg, failureMsg), baseType, subTypeId);
+ }
+
+ private String extraFailureMessage(String msgBase, @Nullable String extra)
Review Comment:
Nit: rename
```suggestion
private String appendFailureMessage(String msgBase, @Nullable String
extra)
```
##########
processing/src/main/java/org/apache/druid/jackson/DefaultObjectMapper.java:
##########
@@ -61,11 +77,54 @@ public DefaultObjectMapper(JsonFactory factory)
configure(MapperFeature.ALLOW_FINAL_FIELDS_AS_MUTATORS, false);
configure(SerializationFeature.INDENT_OUTPUT, false);
configure(SerializationFeature.FLUSH_AFTER_WRITE_VALUE, false);
+
+ addHandler(new DefaultDeserializationProblemHandler(serviceName));
}
@Override
public ObjectMapper copy()
{
return new DefaultObjectMapper(this);
}
+
+ /**
+ * A custom implementation of {@link #DeserializationProblemHandler} to add
custom error message so
+ * that users know how to troubleshoot unknown type ids.
+ */
+ static class DefaultDeserializationProblemHandler extends
DeserializationProblemHandler
+ {
+ private final String serviceName;
+
+ public DefaultDeserializationProblemHandler(@Nullable String serviceName)
+ {
+ this.serviceName = serviceName == null ? "unknown" : serviceName;
+ }
+
+ @VisibleForTesting
+ String getServiceName()
+ {
+ return serviceName;
+ }
+
+ @Override
+ public JavaType handleUnknownTypeId(DeserializationContext ctxt,
+ JavaType baseType, String subTypeId,
TypeIdResolver idResolver,
+ String failureMsg)
+ throws IOException
+ {
+ String msg = String.format("Please make sure to load all the necessary
extensions and jars " +
+ "with type '%s' on '%s' service. " +
+ "Could not resolve type id '%s' as a subtype of %s",
Review Comment:
Super nit: I think it's better to state the problem first and then a
possible resolution.
```suggestion
String msg = String.format("Could not resolve type id '%s' as a
subtype of %s. " +
"Please ensure that the extensions and jars defining the
subtype '%s' " +
"are loaded on service '%s'.\n\n\n",
```
Also maybe add trailing newlines so that the appended `failureMsg` does not
obscure the error message we want to communicate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
