Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RMB-1012: Enhance Database Efficiency by Eliminating Storage of Empty Values #1178

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

RMB-1012: Enhance Database Efficiency by Eliminating Storage of Empty Values #1178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 33 additions & 5 deletions dbschema/src/main/java/org/folio/dbschema/ObjectMapperTool.java
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"}} ]}

Original file line number Diff line number Diff line change
@@ -1,23 +1,23 @@
package org.folio.dbschema;

import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.databind.JsonDeserializer;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.introspect.ClassIntrospector;
import com.fasterxml.jackson.databind.module.SimpleModule;

import java.io.IOException;
import java.io.UncheckedIOException;
import java.util.Date;

import org.folio.dbschema.util.DateDeserializer;
import org.folio.dbschema.util.DateSerializer;

/**
* @author shale
*
*/
public final class ObjectMapperTool {
private static final ObjectMapper DEFAULT_MAPPER = createDefaultMapper();
private static final ObjectMapper MAPPER = createDefaultMapper();
private static final ObjectMapper WRITE_MAPPER = createDefaultWriteMapper();

private ObjectMapperTool() {
throw new UnsupportedOperationException("Cannot instantiate utility class.");
Expand All @@ -31,6 +31,10 @@ public static ObjectMapper getMapper() {
return MAPPER;
}

public static ObjectMapper getWriteMapper() {
return WRITE_MAPPER;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

public static <M, D extends JsonDeserializer<M>> void registerDeserializer(Class<M> clazz, D deserializer) {
SimpleModule module = new SimpleModule();
module.addDeserializer(clazz, deserializer);
Expand All @@ -40,9 +44,9 @@ public static <M, D extends JsonDeserializer<M>> void registerDeserializer(Class
/**
* Map (deserialize) JSON String to java type instance.
*
* @param content JSON content
* @param content JSON content
* @param valueType Resulting type.
* @param <T> Type
* @param <T> Type
* @return instance of type.
*/
public static <T> T readValue(String content, Class<T> valueType) {
Expand Down Expand Up @@ -72,8 +76,32 @@ private static ObjectMapper createDefaultMapper() {
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);
Copy link
Contributor

@julianladisch julianladisch Nov 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

mapper.registerModule(module);
return mapper;
}

private static ObjectMapper createDefaultWriteMapper() {
var mapper = createDefaultMapper();
mapper.setSerializationInclusion(JsonInclude.Include.NON_EMPTY);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed because it is already in the default mapper.

mapper.setMixInResolver(new WriteMixInResolver());
return mapper;
}

@JsonInclude(JsonInclude.Include.NON_EMPTY)
private static class MixIn {
}

private static class WriteMixInResolver implements ClassIntrospector.MixInResolver {

@Override
public Class<?> findMixInClassFor(Class<?> cls) {
return MixIn.class;
}

@Override
public ClassIntrospector.MixInResolver copy() {
return new WriteMixInResolver();
}
}
}
6 changes: 3 additions & 3 deletions domain-models-runtime/src/main/java/org/folio/rest/persist/Conn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ public Future<JsonObject> getByIdForUpdate(String table, String id) {
* @return the JSON converted into a T pojo.
*/
public <T> Future<T> getById(String table, String id, Class<T> clazz) {
return getById(false, table, id, json -> PostgresClient.MAPPER.readValue(json, clazz));
return getById(false, table, id, json -> PostgresClient.READ_MAPPER.readValue(json, clazz));
}

/**
Expand All @@ -157,7 +157,7 @@ public <T> Future<T> getById(String table, String id, Class<T> clazz) {
* @return the JSON converted into a T pojo.
*/
public <T> Future<T> getByIdForUpdate(String table, String id, Class<T> clazz) {
return getById(true, table, id, json -> PostgresClient.MAPPER.readValue(json, clazz));
return getById(true, table, id, json -> PostgresClient.READ_MAPPER.readValue(json, clazz));
}

/**
Expand Down Expand Up @@ -313,7 +313,7 @@ public <T> Future<T> saveAndReturnUpdatedEntity(String table, String id, T entit
String updatedEntityString = rowSet.iterator().next().getValue(0).toString();
try {
@SuppressWarnings("unchecked")
T updatedEntity = (T) PostgresClient.MAPPER.readValue(updatedEntityString, entity.getClass());
T updatedEntity = (T) PostgresClient.READ_MAPPER.readValue(updatedEntityString, entity.getClass());
return updatedEntity;
} catch (JsonProcessingException e) {
throw new UncheckedIOException(e);
Expand Down
15 changes: 8 additions & 7 deletions domain-models-runtime/src/main/java/org/folio/rest/persist/PostgresClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ public class PostgresClient {
static final String COUNT_FIELD = "count";

static final int STREAM_GET_DEFAULT_CHUNK_SIZE = 100;
static final ObjectMapper MAPPER = ObjectMapperTool.getMapper();
static final ObjectMapper READ_MAPPER = ObjectMapperTool.getMapper();
static final ObjectMapper WRITE_MAPPER = ObjectMapperTool.getWriteMapper();

@SuppressWarnings("java:S2068") // suppress "Hard-coded credentials are security-sensitive"
// we use it as a key in the config. We use it as a default password only when testing
Expand Down Expand Up @@ -623,7 +624,7 @@ public static JsonObject pojo2JsonObject(Object entity) throws JsonProcessingExc
if (entity instanceof JsonObject) {
return ((JsonObject) entity);
} else {
return new JsonObject(MAPPER.writeValueAsString(entity));
return new JsonObject(WRITE_MAPPER.writeValueAsString(entity));
}
}

Expand Down Expand Up @@ -2635,7 +2636,7 @@ public <T> void getById(String table, String id, Class<T> clazz,
*/
public <T> void getById(AsyncResult<SQLConnection> conn,
String table, String id, Class<T> clazz, Handler<AsyncResult<T>> replyHandler) {
getById(conn, false, table, id, json -> MAPPER.readValue(json, clazz), replyHandler);
getById(conn, false, table, id, json -> READ_MAPPER.readValue(json, clazz), replyHandler);
}

/**
Expand All @@ -2648,7 +2649,7 @@ public <T> void getById(AsyncResult<SQLConnection> conn,
*/
public <T> void getByIdForUpdate(AsyncResult<SQLConnection> conn,
String table, String id, Class<T> clazz, Handler<AsyncResult<T>> replyHandler) {
getById(conn, true, table, id, json -> MAPPER.readValue(json, clazz), replyHandler);
getById(conn, true, table, id, json -> READ_MAPPER.readValue(json, clazz), replyHandler);
}

/**
Expand Down Expand Up @@ -2740,7 +2741,7 @@ public void getById(String table, JsonArray ids,
*/
public <T> void getById(String table, JsonArray ids, Class<T> clazz,
Handler<AsyncResult<Map<String,T>>> replyHandler) {
getById(table, ids, json -> MAPPER.readValue(json, clazz), replyHandler);
getById(table, ids, json -> READ_MAPPER.readValue(json, clazz), replyHandler);
}

static class ResultsHelper<T> {
Expand Down Expand Up @@ -2863,7 +2864,7 @@ <T> Object deserializeRow(
try {
// is this a facet entry - if so process it, otherwise will throw an exception
// and continue trying to map to the pojos
o = MAPPER.readValue(jo.toString(), org.folio.rest.jaxrs.model.Facet.class);
o = READ_MAPPER.readValue(jo.toString(), org.folio.rest.jaxrs.model.Facet.class);
org.folio.rest.jaxrs.model.Facet of = (org.folio.rest.jaxrs.model.Facet) o;
org.folio.rest.jaxrs.model.Facet facet = resultsHelper.facets.get(of.getType());
if (facet == null) {
Expand All @@ -2874,7 +2875,7 @@ <T> Object deserializeRow(
resultsHelper.facet = true;
return o;
} catch (Exception e) {
o = MAPPER.readValue(jo.toString(), resultsHelper.clazz);
o = READ_MAPPER.readValue(jo.toString(), resultsHelper.clazz);
}
} else {
o = resultsHelper.clazz.newInstance();
Expand Down
30 changes: 29 additions & 1 deletion domain-models-runtime/src/test/java/org/folio/rest/jaxrs/model/User.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@

package org.folio.rest.jaxrs.model;

import jakarta.validation.Valid;
import jakarta.validation.constraints.NotNull;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonPropertyDescription;
import com.fasterxml.jackson.annotation.JsonPropertyOrder;

import jakarta.validation.constraints.Size;
import java.util.ArrayList;
import java.util.List;

/**
* User Schema
Expand Down Expand Up @@ -68,6 +71,16 @@ public class User {
@JsonPropertyDescription("A dummy field to be set by testing trigger")
private String dummy;

/**
* Administrative notes
*
*/
@JsonProperty("administrativeNotes")
@JsonPropertyDescription("Administrative notes")
@Size(min = 0)
@Valid
private List<String> administrativeNotes = new ArrayList<String>();


/**
* A unique name belonging to a user. Typically used for login
Expand Down Expand Up @@ -170,4 +183,19 @@ public void setDummy(String dummy) {
this.dummy = dummy;
}

@JsonProperty("administrativeNotes")
public List<String> getAdministrativeNotes() {
return administrativeNotes;
}

@JsonProperty("administrativeNotes")
public void setAdministrativeNotes(List<String> administrativeNotes) {
this.administrativeNotes = administrativeNotes;
}

public User withAdministrativeNotes(List<String> administrativeNotes) {
this.administrativeNotes = administrativeNotes;
return this;
}

}
8 changes: 2 additions & 6 deletions domain-models-runtime/src/test/java/org/folio/rest/persist/PostgresClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.folio.rest.persist;

import static java.util.Collections.emptyList;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.instanceOf;
Expand Down Expand Up @@ -796,7 +797,7 @@ public void preprocessSqlStatements() throws Exception {
@Test
public void pojo2JsonObject() throws Exception {
String id = UUID.randomUUID().toString();
User user = new User().withId(id).withUsername("name").withVersion(5);
User user = new User().withId(id).withUsername("name").withVersion(5).withAdministrativeNotes(emptyList());
JsonObject json = PostgresClient.pojo2JsonObject(user);
assertThat(json.getMap(), is(Map.of("id", id, "username", "name", "_version", 5)));
}
Expand Down Expand Up @@ -827,11 +828,6 @@ public void pojo2JsonObjectMap2() throws Exception {
Assert.assertEquals("{\"" + id.toString() + "\":\"b\"}", PostgresClient.pojo2JsonObject(m).encode());
}

@Test(expected = Exception.class)
public void pojo2JsonObjectBadMap() throws Exception {
PostgresClient.pojo2JsonObject(this);
}

@Test
public void getTotalRecordsTest() {
assertNull(PostgresClient.getTotalRecords(10, null, 0, 0));
Expand Down