Conversation
|
|
Please retry analysis of this Pull-Request directly on SonarQube Cloud |
There was a problem hiding this comment.
Several modules use ObjectMapperTool.getMapper() for writing: https://github.com/search?q=org%3Afolio-org+ObjectMapperTool+getMapper+valueAsString&type=code
I would rather generate the POJOs with @JsonInclude(JsonInclude.Include.NON_EMPTY).
Can the RAML tooling been configured to generate this?
If not the POJO generator has a JakartaMigrator that can be renamed to PostProcessor and can add the @JsonInclude(JsonInclude.Include.NON_EMPTY) as needed:
https://github.com/folio-org/raml-module-builder/blob/v35.3.0/domain-models-maven-plugin/src/main/java/org/folio/rest/tools/GenerateRunner.java#L199
There was a problem hiding this comment.
Changing JsonInclude.Include.NON_NULL to JsonInclude.Include.NON_EMPTY in POJOs will result in a breaking change for all APIs. The UI might break where it checks not for null but for emptiness. I think it will generate a significant amount of unexpected work for developers.
But with this approach, it will be a choice for each module, to keep old mapper or use new one.
There was a problem hiding this comment.
Can you add documentation to RMB's README and to RMB's upgrading notes how a module can use PostgresClient with the old mapper or the new mapper?
There was a problem hiding this comment.
Some APIs don't use the object mapper, they directly pass on the JSON from the database to HTTP.
Example with a record from mod-inventory-storage sample data:
/inventory-view/instance-set?limit=9&instance=true&query=id=28*
Currently it returns
{"instanceSets":[ {"id": "28d36163-0425-4452-b1f8-1dc4467c52b1", "instance": {"id": "28d36163-0425-4452-b1f8-1dc4467c52b1", "hrid": "bwinst0004", "notes": [], "title": "Magazine - Q1", "series": [], "source": "FOLIO", "_version": 1, "editions": [], "metadata": {"createdDate": "2024-11-25T21:42:56.000Z", "updatedDate": "2024-11-25T21:42:56.000Z", "createdByUserId": "e9414e8d-f0ca-4af5-8bc6-ffd9d4604cce", "updatedByUserId": "e9414e8d-f0ca-4af5-8bc6-ffd9d4604cce"}, "subjects": [], "languages": [], "identifiers": [{"value": "bw", "identifierTypeId": "2e8b3b6c-0e7d-4e48-bca2-b0b23b376af5"}], "publication": [], "contributors": [], "staffSuppress": false, "instanceTypeId": "6312d172-f0cf-40f6-b27d-9fa8feaf332f", "previouslyHeld": false, "classifications": [], "instanceFormats": [], "electronicAccess": [], "holdingsRecords2": [], "publicationRange": [], "alternativeTitles": [], "discoverySuppress": false, "instanceFormatIds": [], "statusUpdatedDate": "2024-11-25T21:42:56.320+0000", "statisticalCodeIds": [], "administrativeNotes": [], "physicalDescriptions": [], "publicationFrequency": [], "natureOfContentTermIds": []}} ]}
With this PR it returns
{"instanceSets":[ {"id": "28d36163-0425-4452-b1f8-1dc4467c52b1", "instance": {"id": "28d36163-0425-4452-b1f8-1dc4467c52b1", "hrid": "bwinst0004", "title": "Magazine - Q1", "source": "FOLIO", "_version": 1, "metadata": {"createdDate": "2024-11-25T18:26:15.000Z", "updatedDate": "2024-11-25T18:26:15.000Z", "createdByUserId": "e9414e8d-f0ca-4af5-8bc6-ffd9d4604cce", "updatedByUserId": "e9414e8d-f0ca-4af5-8bc6-ffd9d4604cce"}, "identifiers": [{"value": "bw", "identifierTypeId": "2e8b3b6c-0e7d-4e48-bca2-b0b23b376af5"}], "staffSuppress": false, "instanceTypeId": "6312d172-f0cf-40f6-b27d-9fa8feaf332f", "previouslyHeld": false, "discoverySuppress": false, "statusUpdatedDate": "2024-11-245T18:26:15.855+0000"}} ]}
|
|
||
| private static ObjectMapper createDefaultWriteMapper() { | ||
| var mapper = createDefaultMapper(); | ||
| mapper.setSerializationInclusion(JsonInclude.Include.NON_EMPTY); |
There was a problem hiding this comment.
This is not needed because it is already in the default mapper.
| module.addSerializer(Date.class, new DateSerializer(Date.class)); | ||
| module.addDeserializer(Date.class, new DateDeserializer(Date.class)); | ||
| var mapper = new ObjectMapper(); | ||
| mapper.setSerializationInclusion(JsonInclude.Include.NON_EMPTY); |
There was a problem hiding this comment.
We also have the mixin.
Is this line with setSerializationInclusion still needed?
If yes please write a unit test that fails if this line gets removed.
| public static ObjectMapper getWriteMapper() { | ||
| return WRITE_MAPPER; | ||
| } | ||
|
|
There was a problem hiding this comment.
We have the valueAsString method that uses the default (read) mapper.
We also need a similar method that uses the write mapper when generating JSON for the DB.
|
2 similar comments
|
|
3 participants
https://folio-org.atlassian.net/browse/RMB-1012
Purpose
Enhance Database Efficiency by Eliminating Storage of Empty Values
Approach
Define object mapper for writing into database that will skip empty values on serialization.