SONARJAVA-6896 Improve the serialization format for Spring context caching - #6114
aurelien-coet-sonarsource wants to merge 5 commits into
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| return readFromCache(context, log, cacheKey, SpringContextCacheHelper::deserializePackages); | ||
| } | ||
|
|
||
| private static JsonObject serializeBeans(List<BeanData> beans) { |
There was a problem hiding this comment.
I'd highly recommend to use Gson's TypeAdapter mechanism for serialization/deserialization stuff. It allows us using steaming API (JsonReader / JsonWriter). You'll need to create several adapters for the classes to serialize (actually only two I guess: BeanData and InjectionPoint) and use them in serialize / deserialize methods. The main benefit is that each adapter is self-contained (and you can use its field names are local string literals, not top-level constants). E.g.:
class BeanDataAdapter extends TypeAdapter<BeanData> {
@Override
public void write(JsonWriter out, BeanData bean) throws IOException {
out.beginObject();
out.name("name").value(bean.beanName());
out.name("type").value(bean.type());
out.name("package").value(bean.beanPackage());
// ... etc.
out.endObject();
}
@Override
public BeanData read(JsonReader in) throws IOException {
String name = null, type = null, pkg = null;
// ... locals for all fields
in.beginObject();
while (in.hasNext()) {
switch (in.nextName()) {
case "name" -> name = in.nextString();
case "type" -> type = in.nextString();
case "package" -> pkg = in.nextString();
// ...
default -> in.skipValue();
}
}
in.endObject();
return new BeanData(name, type, pkg, /* ... */);
}
}
asya-vorobeva
left a comment
asya-vorobeva
left a comment
There was a problem hiding this comment.
After switching to using TypeAdapter, it makes sense to add tests for SpringContextCacheHelper. Thus we can probably reduce / simplify caching tests in our GathererTest's.
Code Review ✅ Approved 4 resolved / 4 findingsMigrates Spring context cache serialization from custom delimited strings to structured JSON with Gson and versioning. Round-trip test now asserts typeHierarchy restoration, cache key collision handling is fixed to use full paths instead of just file names, javadoc for readNullableString is corrected, and integration tests validate real parse results through the cache. ✅ 4 resolved✅ Quality: Round-trip test never asserts typeHierarchy is restored
✅ Quality: readNullableString javadoc points at required(), which cannot work
✅ Quality: endsWith(fileName) cannot distinguish the two cache keys
✅ Quality: No test writes a real parse result through the cache anymore
Implementation Status ◻️ 1 of 2 objectives covered◻️ SONARJAVA-6896 - 1 of 2 objectives coveredThis PR improves the Spring context caching serialization format using JSON via Gson type adapters and helper classes, but does not introduce an intermediate abstract class or interface for caching across checks. Other objectives on this issue, possibly covered elsewhere:
✅ 1 covered here
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
| */ | ||
| final class BeanDataTypeAdapter extends TypeAdapter<BeanData> { | ||
|
|
||
| private static final String NAME = "name"; |
There was a problem hiding this comment.
Let's remove these top-level constants as I've noted in initial comment. It's absolutely acceptable to use string literals as they're local to the adapter. And it's much more simpler to read:
@Override
public void write(JsonWriter out, BeanData bean) throws IOException {
out.beginObject();
out.name("name").value(bean.beanName());
// ... etc.
out.endObject();
}
@Override
public BeanData read(JsonReader in) throws IOException {
String name = null, type = null, pkg = null;
// ... locals for all fields
in.beginObject();
while (in.hasNext()) {
switch (in.nextName()) {
case "name" -> name = in.nextString();
// ...
default -> in.skipValue();
}
}
in.endObject();
return new BeanData(name, type, pkg, /* ... */);
}
There was a problem hiding this comment.
Yes, the string literals are local to the adapters, but they are reused at least 3 times in each class, between the read and write methods, so I would argue that it makes sense to centralize them in static fields, so we only need to modify a single location if we want to change them later (I also suspect Gitar will flag these as code quality issues). WDYT ?
2 participants
Summary by Gitar
GsonJSON objects with versioningThis will update automatically on new commits.