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.

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

HAPI fails to reject JSON with duplicate object keys #848

Open
lawley opened this issue Feb 8, 2018 · 12 comments

Comments

@lawley
Copy link
Contributor

@lawley lawley commented Feb 8, 2018

While the following example is valid JSON, it is not valid FHIR JSON. It looks like HAPI is using GSON to parse this and GSON silently throws away all but the last value for concept.

{
  "resourceType": "CodeSystem",
  "id": "dupe-properties",
  "url": "http://csiro.au/test/duplicate-properties",
  "concept": [
    {
      "code": "A",
      "display": "The letter A",
      "definition": "The letter A"
    }],
  "concept": [
    {
      "code": "B",
      "display": "The letter B",
      "definition": "The letter B"
    }
  ]
}
@splatch

This comment has been minimized.

Copy link
Contributor

@splatch splatch commented Mar 21, 2018

@lawley you are right, from syntax/EBNF point of view this is valid structure, however it's not valid object representation. I've found following issue registered for gson: google/gson#386.

Quote form linked issue, coming from https://www.ietf.org/rfc/rfc4627.txt

2.2. Objects
An object structure is represented as a pair of curly brackets
surrounding zero or more name/value pairs (or members). A name is a
string. A single colon comes after each name, separating the name
from the value. A single comma separates a value from a following
name. The names within an object SHOULD be unique.

According to gson it's still open, so it might be valid to poke Google about it. :-)

Cheers,
Lukasz

@jamesagnew

This comment has been minimized.

Copy link
Owner

@jamesagnew jamesagnew commented Mar 26, 2018

I kind of agree with the argument in the linked bug report- This issue is real, but actually enforcing it would mean a nontrivial performance penalty.

I'd be inclined to fix this in the validator as opposed to in the parser personally, since the anyone using the validator is consciously paying the validation performance penalty already.

@grahamegrieve

This comment has been minimized.

Copy link
Collaborator

@grahamegrieve grahamegrieve commented Mar 26, 2018

@lawley

This comment has been minimized.

Copy link
Contributor Author

@lawley lawley commented Oct 17, 2019

The FHIR spec is clear that duplicate keys are invalid: https://www.hl7.org/fhir/json.html
The JSON spec is clear that duplicate keys are acceptable, but behaviour is unspecified.

GSON is unlikely to prevent duplicates (but they may accept a pull request that allowed for detection of duplicate keys).

Since the FHIR+JSON parsing in HAPI (& therefore the Java validator) uses GSON, it won't pick it up unless it does a separate pass or deserialises to a custom Map class (rather than JsonObject) that detects duplicates - e.g., see google/gson#647 (comment)

@grahamegrieve

This comment has been minimized.

Copy link
Collaborator

@grahamegrieve grahamegrieve commented Oct 17, 2019

@lawley

This comment has been minimized.

Copy link
Contributor Author

@lawley lawley commented Oct 17, 2019

Hmm, so it seems the issue we're facing is that HAPI is doing GSON-based parsing of resources before calling methods annotated with @Validate, and thus those errors are hidden.

Perhaps HAPI should use org.hl7.fhir.*.elementmodel.JsonParser (is that the right one?) for @Validate methods?

@grahamegrieve

This comment has been minimized.

Copy link
Collaborator

@grahamegrieve grahamegrieve commented Oct 17, 2019

@jamesagnew

This comment has been minimized.

Copy link
Owner

@jamesagnew jamesagnew commented Oct 17, 2019

I thought it did pass the bytes through...

But looking at this, it looks like there is a bug. It's not actually passing the raw bytes to the validator like it should be.

FIxing now.

@lawley

This comment has been minimized.

Copy link
Contributor Author

@lawley lawley commented Oct 17, 2019

@jamesagnew I think ValidationContext.forText also needs patching?

@jamesagnew

This comment has been minimized.

Copy link
Owner

@jamesagnew jamesagnew commented Oct 17, 2019

I don't see any obvious issues with that method.. what do you think needs to change?

@lawley

This comment has been minimized.

Copy link
Contributor Author

@lawley lawley commented Oct 17, 2019

If I have raw JSON and call FhirValidator.validateWithResult(String) then I would expect complaints about duplicate JSON keys, but as far as I can tell the code path goes from this to ValidationContext.forText to parse the JSON and that uses GSON which will hide the errors.

jamesagnew added a commit that referenced this issue Oct 17, 2019
@jamesagnew

This comment has been minimized.

Copy link
Owner

@jamesagnew jamesagnew commented Oct 17, 2019

Hmm, it works fine for me with the fix above. I just committed a test, can you compare it to your use case and see if there are obvious differences?

b2f68fd

@jamesagnew jamesagnew reopened this Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.