SOLR-16825: Fix response serialization bug #1867
Conversation
|
This feels like a bit of a "sigh" change... So for now through the end of time every pojo gets a rather opaque interface that they implement? ;-). |
|
Hmm might be able to "fix" this... |
|
Ok we don't need to use By changing:
I'm going to run the full test suite to make sure this works, but it would be very nice if it does. Also we can keep the |
|
I considered that approach initially, but ended up scrapping the idea as I was worried that changing JavaBinCodec's default would be too broad and "dangerous". But in hindsight maybe that was the wrong call - the JBC fallback behavior is so bad it's hard to imagine anything relying on it. If we're happy relying on tests to vet this for us, and you all think it's reasonable, I'm all for it. Lmk if I can do anything to help! |
Therefore request/response types do not need to extend anything in order to work with our serialization framework
HoustonPutman
force-pushed
the
SOLR-16825-fix-response-serialization-bug
branch
from
584bd7c
to
3ea8db1
Compare
|
Ok, rewrote this so that it will use a Reflection-based writer by default, but still use a string writer if no reflection fields are found. This means that we don't need to worry about the inheritance for new types, but we can still manually choose a "ReflectWritable" if there is ambiguity and we need to make sure. |
| import java.io.IOException; | ||
| import org.apache.solr.common.util.Utils; | ||
|
|
||
| public class DelegateMapWriter implements MapWriter { |
There was a problem hiding this comment.
Good thought to nuke this implementation 👍
| @@ -88,7 +87,7 @@ default void writeVal(String name, Object val, boolean raw) throws IOException { | |||
| } else if (val instanceof MapWriter) { | |||
| writeMap(name, (MapWriter) val); | |||
| } else if (val instanceof ReflectWritable) { | |||
| writeMap(name, new DelegateMapWriter(val)); | |||
| writeVal(name, Utils.getReflectWritable(val)); | |||
There was a problem hiding this comment.
[Q] What purpose does the call to getReflectWritable serve, since we know that "val" is already a ReflectWritable?
There was a problem hiding this comment.
Ok, the name is probably really bad here, but getReflectWritable() does not return a ReflectWritable class. It returns that DelegateReflectWriter. I'll rename this to getReflectWriter(), as it will be much less confusing. Good catch.
Unknown types will try to serialize using Reflection, but will use a "toString()" serialization if no JSONProperty annotations are found. This is backwards compatible. Co-authored-by: Houston Putman <houston@apache.org> (cherry picked from commit 9a45385)
https://issues.apache.org/jira/browse/SOLR-16825
Description
Request and response body POJOs in the 'api' module must implement 'ReflectWritable', a marker interface that indicates to JavabinCodec and other serialization classes that they have field information that can be discovered via reflection.
A prior commit added several POJOs to this module that didn't follow this rule, causing test failures in nightly runs.
Solution
This commit adds the needed marker interface to several classes in the 'api' submodule that were missing it.
Tests
Broken tests resume passing.
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.