kazarena / json-gold

A JSON-LD processor for Go
Apache License 2.0
114 stars 17 forks source link

Don't assume @id values are of type `string`. #12

Closed BurntSushi closed 6 years ago

BurntSushi commented 8 years ago

Instead, we should check if they're a string, and if not, report an error. Otherwise, this will panic.

BurntSushi commented 8 years ago

N.B. I tried to write a test for this: https://github.com/BurntSushi/json-gold/blob/master/ld/testdata/error-0044-in.jsonld --- But I couldn't get it to work. Currently, the test fails because no error seems to be detected.

kazarena commented 8 years ago

@BurntSushi : your test doesn't fail because it's a valid JSON-LD. Absence of @id property is ok.

I appreciate what you are doing in your PR but I struggle as well to come up with any examples which would cause this piece of code to run. You see, invalid @id is taken care of much earlier in the chain, see https://github.com/kazarena/json-gold/blob/master/ld/api_expand.go#L98. As a result, even if I try to feed it with the following JSON:

{
    "@type": "SocialMediaPosting",
    "@id": {
        "key": "value"
    },
    "url": "http://cctvnews.tumblr.com/post/89361760824/sansha-city-chinas-tropical-outpost-its-been",
    "mainEntityOfPage": true,
    "datePublished": "2014-06-20T14:30:53-04:00",
    "author": "cctvnews",
    "articleBody": "..."
}

the code will return a correct error ('invalid @id value')

How did you come across this issue in the first place?

BurntSushi commented 8 years ago

I came across it because the interface conversion failed and caused a panic. My intent was to reproduce the issue in your existing test framework, but that didn't work and I'm not sure why. It looks like my next step is to reproduce the issue I was hitting in a standalone test. I'll get back to you soon. :-)

On Oct 5, 2016 4:56 PM, "kazarena" notifications@github.com wrote:

@BurntSushi https://github.com/BurntSushi : your test doesn't fail because it's a valid JSON-LD. Absence of @id https://github.com/id property is ok.

I appreciate what you are doing in your PR but I struggle as well to come up with any examples which would cause this piece of code to run. You see, invalid @id https://github.com/id is taken care of much earlier in the chain, see https://github.com/kazarena/json-gold/blob/master/ld/api_ expand.go#L98. As a result, even if I try to feed it with the following JSON:

{ "@type": "SocialMediaPosting", "@id": { "key": "value" }, "url": "http://cctvnews.tumblr.com/post/89361760824/sansha-city-chinas-tropical-outpost-its-been", "mainEntityOfPage": true, "datePublished": "2014-06-20T14:30:53-04:00", "author": "cctvnews", "articleBody": "..." }```

the code will return a correct error ('invalid @id value')

How did you come across this issue in the first place?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kazarena/json-gold/pull/12#issuecomment-251796942, or mute the thread https://github.com/notifications/unsubscribe-auth/AAb34tUJLla1yDofRwM2XXVdHYybY1FOks5qxA7ngaJpZM4KPCAl .

kazarena commented 8 years ago

@BurntSushi : ok, thank you for your efforts! There are two reasons I'm asking about this test case:

1) the fact that it went so deep into the code before failing may indicate some other problems upstream 2) as we are looking into it, we may as well remove the TODO comment above your change