fix: Add JSON column type for SQL Views

[jason-p-pickering]

Fixes https://dhis2.atlassian.net/browse/DHIS2-21373

Root cause

SqlRowSet.getObject(i) returns a PGobject for PostgreSQL json/jsonb columns. PGobject.toString() returns the JSON text. JacksonRowDataSerializer then calls jgen.writeString() on every grid cell unconditionally, quoting and escaping the already-valid JSON text.

Proposed fix

A RawJsonValue marker record is introduced in dhis-api. HibernateSqlViewStore.populateRows() inspects SqlRowSetMetaData.getColumnTypeName() and wraps non-null json/jsonb values as RawJsonValue. JacksonRowDataSerializer emits RawJsonValue via jgen.writeRawValue()instead of
jgen.writeString().

For CSV/XML outputs, OutputFormatter.maybeFormat() unwraps RawJsonValue to its plain string.

This is a workaround, not a proper fix. See the companion architectural ticket (Ticket 2 below) for why.

Limitations of the approach

• RawJsonValue leaks a SQL-view-specific concept into dhis-api (JacksonRowDataSerializer, OutputFormatter), both of which are shared by analytics and other Grid consumers.

• CSV and XML outputs emit the raw JSON string inline, which may not be valid for those formats depending on consumer expectations.

• Obviously breaks existing clients which may unescape JSON output.

Before:

"rows": [
[
"XX MAL RDT TRK - Age",
"XX MAL RDT - Age",
"",
"",
"{\"icon\": \"star_medium_positive\"}"
],
[
"XX MAL RDT TRK - Village of Residence",
"XX MAL RDT - Village of Residence",
"",
"",
"{\"icon\": \"star_medium_positive\"}"
]
]

After:

[
[
"XX MAL RDT TRK - Age",
"XX MAL RDT - Age",
"",
"",
{
"icon": "star_medium_positive"
}
],
[
"XX MAL RDT TRK - Village of Residence",
"XX MAL RDT - Village of Residence",
"",
"",
{
"icon": "star_medium_positive"
}
],
[
"[DI] ANC Validation rule monitor",
"[DI] ANC Validation rule monitor",
"",
"",
{}
]
]

[jbee]

We discussed this in todays platform meeting and we are happy with this approach but it is a breaking change for anyone that has views extracting JSON columns where the response changes from quoted JSON to plain JSON. Before we do that we need the OK from @stian-sandvold

[jason-p-pickering]

Thanks @jbee . Yes, I think we can maybe include this in the release notes. My feeling is that not many people are using JSON in SQL views, but you never know. Regardless,I think it should be a one line fix on the client side, so even if it does break existing integrations/scripts, it should be a minor fix on the client side. Will wait for @stian-sandvold to weigh in.

[stian-sandvold]

I guess my main questions here are:

1. Why do we need to make this change, when it's already parsable. Is there some specific requirement for this? Since it's potentially breaking clients already expected a parsable to json string.
2. I saw other custom types mentioned as well, like geometry. Is the plan to add custom handling for those as well?
3. If we do have a plan to decouple from Grid/analytics, is there any reason to rush this solution in now, instead of working towards a long term solution?

Assuming its valuable to bring this in now, I am for it (Note the other comments); Just want to understand the need and intention :)

[stian-sandvold]

Looks like this is a bug that's already present, but caught my attention looking at the code:

i represent the number of columns, while the exception highlights the number of rows.

This is however also the same problem in the old code (addRows).

[stian-sandvold]

Just to keep the code a bit more condense, you could move the content of this for loop into the next, and keep everything in one, instead of spreading the column related code in two blocks. Performance wise, I don't see any advantage of pre-calculating these booleans

[stian-sandvold]

from this PR, as far as I can tell in the code, this piece of code is redundant.

Since you're already adding a condition in JacksonRowDataSerializer to not use maybeFormat if the field is an instanceof RawJsonValue, you will in theory never get it in this code execution.

I'd say remove either or, unless there's some reason I cant see we need both checks :)

[jason-p-pickering]

I guess my main questions here are:

1. Why do we need to make this change, when it's already parsable. Is there some specific requirement for this? >Since it's potentially breaking clients already expected a parsable to json string.

This came up because I was working on a custom SQL view for an integration script and noticed that the JSON column was not really "JSON". It is as you say, trivial for the client to deal with this, it's just not really that nice.

2. I saw other custom types mentioned as well, like geometry. Is the plan to add custom handling for those as >well?

No plans for this at the moment with this PR. It could of course be nice to be able to do this. I think we would have to fully decouple Grid first though.

3. If we do have a plan to decouple from Grid/analytics, is there any reason to rush this solution in now, instead >of working towards a long term solution?

I do not think there is really a rush. Its more that it just is wrong. :-D

Assuming its valuable to bring this in now, I am for it (Note the other comments); Just want to understand the >need and intention :)

Let me know how you want to proceed @stian-sandvold .

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[stian-sandvold]

Let's go ahead with it, but we'll have to share this in release notes since it potentially breaks clients using sql views with jsonb :)

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud