FHIR server $validate accepts resource json with duplicate keys

[mjdisc]

During initial PDM R6.0 testing, it was discovered that a FHIR $validate operation would return a NoError response for a resource with duplicate keys. This can be reproduced as follows:

url: https://us-south.wh-fhir.dev.cloud.ibm.com/wh-fhir-dev/api/v4//Condition/$validate

POST payload:

{
	"resourceType": "Condition",
	"subject": {
		"reference": "Patient/b589c393-4146-4fef-921d-3facbfbd3bd0"
	},
	"subject": {
		"reference": "Patient/e8f4d046-4eb5-4c9e-972a-fb02c5e4d5f4"
	}
}

Strictly speaking, the json spec allows duplicate keys. But many json editors and other tooling flag dup keys as an error. I then sent the above resource to the create API. The Condition was successfully created, with the second subject only.

I think it would be prudent for FHIR to detect duplicate keys on resources and return the appropriate failure responses on validate, create, update. Persisting data in the FHIR database that passed validation and yet is not a reflection of the data sent in by the user is misleading, IMHO.

[JohnTimm]

I was able to reproduce this with the following unit test:

    @Test
    public void testDuplicateKeys() throws Exception {
        try (InputStream in = DuplicateKeysTest.class.getClassLoader().getResourceAsStream("JSON/condition-duplicate-keys.json")) {
            JsonReader jsonReader = Json.createReader(in);
            JsonObject jsonObject = jsonReader.readObject();
            System.out.println(jsonObject);
        }
    }

This indicates that the underlying javax.json library has a bug. I will investigate a mitigation strategy.

[JohnTimm]

According to this stack overflow, the JSON spec doesn't explicitly forbid duplicate keys:
https://stackoverflow.com/questions/21832701/does-json-syntax-allow-duplicate-keys-in-an-object

Even the org.json parser allows duplicate keys by default and requires a source code modification to mitigate:
https://medium.com/@harshalparekh20696/modifying-org-json-to-handle-duplicate-keys-975bfffe2f5a

https://cyantificmusings.wordpress.com/2016/05/16/duplicate-keys-in-json-objects/

[JohnTimm]

In order to mitigate this issue, we would need to either: a) implore the javax.json folks to fix their implementation, b) switch to a different JSON library that rejects duplicates or c) write our own JSON parser.

[lmsurpre]

from https://www.hl7.org/fhir/json.html

Property names SHALL be unique. Note: this is not explicitly stated in the original JSON specification,so stated for clarity here

So it would be nice to have a longer-term plan for this one. Maybe start by just opening an issue at https://github.com/eclipse-ee4j/jsonp/issues to see if they would consider adding it as an option?

[JohnTimm]

I tried swapping out the javax.json implementation with: https://github.com/leadpony/joy and, unfortunately, it has the exact same behavior.

[JohnTimm]

Jackson exhibits the same behavior using this code:

    @Test
    public void testDuplicateKeys2() throws Exception {
        try (InputStream in = DuplicateKeysTest.class.getClassLoader().getResourceAsStream("JSON/condition-duplicate-keys.json")) {
            ObjectMapper objectMapper = new ObjectMapper();
            JsonNode jsonNode = objectMapper.readValue(in, JsonNode.class);
            System.out.println(jsonNode);
        }
    }

[JohnTimm]

Found an option to prohibit duplicate keys:

    @Test
    public void testDuplicateKeys2() throws Exception {
        try (InputStream in = DuplicateKeysTest.class.getClassLoader().getResourceAsStream("JSON/condition-duplicate-keys.json")) {
            ObjectMapper objectMapper = new ObjectMapper();
            objectMapper.enable(JsonParser.Feature.STRICT_DUPLICATE_DETECTION);
            JsonNode jsonNode = objectMapper.readValue(in, JsonNode.class);
            System.out.println(jsonNode);
        }
    }

[JohnTimm]

#After further research, I found that Jackson supports a configuration that will reject duplicate keys:

    @Test
    public void testDuplicateKeys2() throws Exception {
        try (InputStream in = DuplicateKeysTest.class.getClassLoader().getResourceAsStream("JSON/condition-duplicate-keys.json")) {
            ObjectMapper objectMapper = new ObjectMapper();
            objectMapper.enable(JsonParser.Feature.STRICT_DUPLICATE_DETECTION);
            JsonNode jsonNode = objectMapper.readValue(in, JsonNode.class);
            System.out.println(jsonNode);
        }
    }

Jackson also has compatibility with javax.json:
https://github.com/FasterXML/jackson-datatypes-misc/tree/master/jsr-353

This module implements the javax.json object model on top of Jackson. I worked out a way that we could wire this up in the fhir-provider module. It fixes the original issue, but introduces a new issue related to BigDecimal precision. Precision in does not always equal precision out. In order to fix the precision issue, I would need to modify a source file coming from the Jackson project. This doesn't seem like a viable solution. We have raised an issue with the jakarta-ee JSON-P project here:

jakartaee/jsonp-api#240

They said that they would welcome a PR to fix the issue directly in their codebase.

[JohnTimm]

Created this PR:
jakartaee/jsonp-api#241

[prb112]

John mentioned his PR is now accepted. We are awaiting the next release

[prb112]

Reviewing results for QA with John. Sent him a postman collection to confirm the behavior

[prb112]

Discussed with John.

Nested Objects and Arrays result in an unintended result where duplicate keys are ignored.

The postman collection is at https://gist.github.com/prb112/9b1d73bcd606507df393aa497b64ae8c

[JohnTimm]

I created this issue in the the eclipse-ee4j/jsonp GitHub issue tracker:
jakartaee/jsonp-api#292

then I created this PR:
jakartaee/jsonp-api#293

which was immediately reviewed and merged into master. This fix should be part of their next patch release.

[prb112]

PR #2794 moves to parrson groupId and project. It's still part of Eclipse and has John's fix in it.

[prb112]

PR is raised, and just needs QA

[prb112]

{
    "resourceType": "OperationOutcome",
    "issue": [
        {
            "severity": "fatal",
            "code": "invalid",
            "details": {
                "text": "FHIRProvider: Duplicate key 'status' is not allowed"
            },
            "expression": [
                "<no expression>"
            ]
        }
    ]
}

Test passes now as expected and rejects nested duplication in the Bundle, contained and nested JSON objects