fix(sarif): QD-8410: Fix ignore properties during deserialization #17
Conversation
sarif/src/main/java/com/jetbrains/qodana/sarif/model/PropertyBag.java
Outdated
Show resolved
Hide resolved
sarif/src/main/java/com/jetbrains/qodana/sarif/model/MapIgnoreKeysAdapter.java
Outdated
Show resolved
Hide resolved
|
|
||
| public class MapIgnoreKeysAdapter extends TypeAdapter<Map<String, Object>> { | ||
|
|
||
| private static final Gson embedded = new GsonBuilder().setPrettyPrinting().disableHtmlEscaping().create(); |
There was a problem hiding this comment.
Hm, I think it makes more sense to pass configured gson instances via constructor (here, and everywhere else with an embedded gson), for two reasons:
- Serialization Correctness: Let's say we have a property bag that contains
java.lang.Instant: If encountered in other fields, they will be serialized according to spec (seeIsoInstantSerializer), but if they are encountered in property bag, it would use gson's default.
While the spec doesn't necessarily say how custom values in properties are to be serialized, it's at least very confusing to have potentially different formats in the same file - Performance: Gson caches how to create types, and these caches could be shared between our type adapters (
Gson#constructorConstructor)
IIRC the TypeAdapterFactory approach would help us do that
There was a problem hiding this comment.
Yes, we would have to implement a factory, since otherwise we don't have access to gson instance, since type adapter is constructed before, I can do that.
But I'm not convinced that we want this adapter for all classes though. We only want to ignore properties inside PropertyBag, if I create a factory, and refactor to use a single gson instance, gson will use it for all types to which Map<String,Object> is assignable to. If I instantiate a new gson instance just in PropertyBag with a factory, we don't gain much and have added complexity in factory code
There was a problem hiding this comment.
Actually, I can just create a type that wraps map and use a factory with that. Will do
There was a problem hiding this comment.
Actually, why even split the property adapter from the ignore keys adapter? If they are one, we don't have that problem, as that single adapter would only be registered for PropertyBag
EDIT: Because we don't have type info when reading things in properties, so it would not recognize a bag as a bag automatically. Which also applies to all other custom serializers then, like Instant.
So for reading, it wouldn't change much, but for writing it would.
There was a problem hiding this comment.
Hm, actually I like the idea of keeping it all in the property bag adapter. I thought it's cleaner to keep it separate, but the logic inside property bag adapter is very minimal so we could merge it with reading typeless map
sarif/src/test/java/com/jetbrains/qodana/sarif/model/MapIgnoreKeysAdapterTest.java
Outdated
Show resolved
Hide resolved
sarif/src/test/java/com/jetbrains/qodana/sarif/StreamParseCorrectnessTest.java
Show resolved
Hide resolved
|
The qodana check will continue to fail because you're on a fork. At best, you join the JB org on GitHub (or I recreate your PR later) |
Not sure what the process is for that, might do it later, but if you can recreate your PR for now that'd be great 🙏 |
|
Replaced with #18 |
| out.beginObject(); | ||
|
|
||
| for (Map.Entry<String, Object> entry : toSerialize.entrySet()) { | ||
| if (!ignoreKeys.contains(entry.getKey())) { |
There was a problem hiding this comment.
i dont see any purpose to support this for write
Ignoring keys in property bag was not working previously, due to custom type adapter for class
PropertyBag