fix: remove jsonb_array and fix jsonb data types such that it can be used in schema design and graphql api

[mswertz]

closes #4405

todo:

• remove any jsonb_array function
• add jsonb to schema design and graphql (it clearly never has worked in practice)

out of scope:

• because jsonb has never worked in graphql we assume no migrations are needed.

[esthervanenckevort]

Code looks fine, but is missing a migration step. I would expect a JSONB_ARRAY to be converted to a JSON Object containing an array of JSON objects.

[mswertz]

Code looks fine, but is missing a migration step. I would expect a JSONB_ARRAY to be converted to a JSON Object containing an array of JSON objects.

Not needed, was not useable.

json has never worked so can't be anything to migrate

[svandenhoek]

If #4323 ends up getting merged before this PR, be sure to remove the following lines after merging with master:

molgenis-emx2/backend/molgenis-emx2-rdf/src/main/java/org/molgenis/emx2/rdf/ColumnTypeRdfMapper.java

Line 67 in 23d8cb9

entry(ColumnType.JSONB_ARRAY, RdfColumnType.SKIP), // Unsupported.

molgenis-emx2/backend/molgenis-emx2-rdf/src/test/java/org/molgenis/emx2/rdf/ColumnTypeRdfMapperTest.java

Lines 151 to 152 in 23d8cb9

	ColumnType.JSONB_ARRAY.name(), // TODO: Remove if deprecated.
	"{\"c\":3},{\"d\":4,\"e\":5}",

molgenis-emx2/backend/molgenis-emx2-rdf/src/test/java/org/molgenis/emx2/rdf/ColumnTypeRdfMapperTest.java

Lines 372 to 373 in 23d8cb9

	// TODO: Remove if deprecated.
	() -> Assertions.assertEquals(Set.of(), retrieveValues(ColumnType.JSONB_ARRAY.name())),

#4323 contains a test that validates whether each ColumnType has a mapper to RDF, so pre-emptively removing JSON_ARRAY from src would cause a failing test and removing test code with source code still present seems illogical.

EDIT: Might be some more conflicts as I see JSONB is renamed to JSON. We'll see when we get there then 🙂

[svandenhoek]

Found some things that might need to be adjusted/improved before we merge it:

• docs -> use_schema.md
  • remove jsonb_array
  • rename jsonb to json
• Consider front-end json validation, such as:
  • try-except with JSON.parse(json);
  • a regex like https://stackoverflow.com/a/32155765 (link needs conversion from PHP though)
• While during row-creation invalid data is not accepted, if editing an existing row this can still be done. See f.e. https://preview-emx2-pr-4409.dev.molgenis.org/json_input_test/tables/#/Test :

input	expected	actual
{"key":"value"};alert("test")	error (not valid json)	"{\"key\":\"value\"};alert(\"test\")"
"{"key":"value"};alert("test")"	error (not valid json)	"\"{\"key\":\"value\"};alert(\"test\")\""
{}alert("test")	error (not valid json)	"{}alert(\"test\")"
"alert("test")	error (not valid json)	"\"alert(\"test\")"

• Consider adding tests that include primitives as well ("", "string", -1, true, null), see also https://datatracker.ietf.org/doc/html/rfc7159#section-3
  • null is converted to an empty field (should stay null)
• {"key":"value1", "key":"value2"} yields {"key":"value2"}. While technically correct, an error or warning would be preferred.

[mswertz]

Found some things that might need to be adjusted/improved before we merge it:

• docs -> use_schema.md

  • remove jsonb_array
  • rename jsonb to json

• Consider front-end json validation, such as:

  • try-except with JSON.parse(json);
  • a regex like https://stackoverflow.com/a/32155765 (link needs conversion from PHP though)

• While during row-creation invalid data is not accepted, if editing an existing row this can still be done. See f.e. https://preview-emx2-pr-4409.dev.molgenis.org/json_input_test/tables/#/Test :

input expected actual
{"key":"value"};alert("test") error (not valid json) "{\"key\":\"value\"};alert(\"test\")"
"{"key":"value"};alert("test")" error (not valid json) "\"{\"key\":\"value\"};alert(\"test\")\""
{}alert("test") error (not valid json) "{}alert(\"test\")"
"alert("test") error (not valid json) "\"alert(\"test\")"

• Consider adding tests that include primitives as well ("", "string", -1, true, null), see also https://datatracker.ietf.org/doc/html/rfc7159#section-3

  • null is converted to an empty field (should stay null)

• {"key":"value1", "key":"value2"} yields {"key":"value2"}. While technically correct, an error or warning would be preferred.

Grotendeels goede punten; uitdaging is dat "string" ook valide json is. Maar we zouden kunnen zeggen dat als het resultaat van de parse geen Map of List is dat we dan het niet valide vinden?

[svandenhoek]

@mswertz: Normaliter zou ik voorstander zijn van de RFC standaarden zo goed mogelijk te volgen, al is er hier wel een punt te maken dat qua usecase we zelf al datatypes hebben voor string/int/boolean (behalve null?) waardoor we liever willen dat een gebruiker dat gebruikt i.p.v. een JSON met enkel 1 primitive. Stel wel voor dat we dan:

• Dit helder in de documentatie zetten:

* json : validates json format (must be an array or object!)

• Bij back-end checken of de root node een array/object is, bijvoorbeeld:

JsonNode tree = objectMapper.readTree(jsonString);
    if(!tree.isArray() && !tree.isObject()) {
        throw new MolgenisException("JSON is not an array/object.");
    }

[mswertz]

@mswertz: Normaliter zou ik voorstander zijn van de RFC standaarden zo goed mogelijk te volgen, al is er hier wel een punt te maken dat qua usecase we zelf al datatypes hebben voor string/int/boolean (behalve null?) waardoor we liever willen dat een gebruiker dat gebruikt i.p.v. een JSON met enkel 1 primitive. Stel wel voor dat we dan:

• Dit helder in de documentatie zetten:

* json : validates json format (must be an array or object!)

• Bij back-end checken of de root node een array/object is, bijvoorbeeld:

JsonNode tree = objectMapper.readTree(jsonString);
    if(!tree.isArray() && !tree.isObject()) {
        throw new MolgenisException("JSON is not an array/object.");
    }

yes, kun jij dat doen of wil je dat ik het doe?

[svandenhoek]

Changes:

• Updated docs
• Fixed a bug (serialize must throw CoercingSerializeException and not MolgenisException)
• Added back-end validation (through TypeUtils.toJsonb())
  • Validates on trailing data
  • Validates on uniqueness (remove if performance hit is too big)
  • Validation now also works when editing existing field
• Added front-end validation
  • Does not validate in key-uniqueness (relies on back-end for this)
  • TODO: textarea compains about :value="modelValue" (though TextInput.vue also does it this way but is not labeled as lang="ts"). Might need some fixing before merge or simply use input instead of textarea.
  • TODO:

    molgenis-emx2/apps/metadata-utils/src/fieldHelpers.ts

    Line 49 in 996dfe1

    export const isValueType = (column: IColumn) => {

    does not include UUID in isValueType(). Should UUID and/or JSON included here? UUID was present, just not in the same order as fieldTypes from that same file.

[mswertz]

looks good, one question left. I cannot approve so if you agree with comment can you then approve yourself?

[mswertz]

doesn't check if it is an array?

[svandenhoek]

It should allow arrays, see for example: https://jsbin.com/paperodivi/1/edit?html,output

While null is also an object, it does fail in the form (probably due to if(!parsed)):

[svandenhoek]

I'll have a quick look if I can fix null giving the correct error message though (no array/object instead of invalid JSON).

[svandenhoek]

Test code for front-end now also added:

molgenis-emx2/apps/molgenis-components/src/components/forms/formUtils/formUtils.test.ts

Lines 380 to 391 in ff6cdbf

	describe("isJsonObjectOrArray", () => {
	test("only JSON object/array should return true (after parsing from JSON string)", () => {
	expect(isJsonObjectOrArray(JSON.parse('{"key":"value"}'))).toBe(true);
	expect(isJsonObjectOrArray(JSON.parse('["string1", "string2"]'))).toBe(true);
	expect(isJsonObjectOrArray(JSON.parse('{"key1":{"key2":["value1", "value2"]}}'))).toBe(true);
	expect(isJsonObjectOrArray(JSON.parse('"string"'))).toBe(false);
	expect(isJsonObjectOrArray(JSON.parse('1'))).toBe(false);
	expect(isJsonObjectOrArray(JSON.parse('true'))).toBe(false);
	expect(isJsonObjectOrArray(JSON.parse('false'))).toBe(false);
	expect(isJsonObjectOrArray(JSON.parse('null'))).toBe(false);
	});
	});

[chinook25]

looks good, just some small thingies.

[sonarqubecloud]

Quality Gate passed

Issues
14 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[svandenhoek]

@mswertz: I approved and @chinook25 did another review and that feedback is included as well.