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

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

Closed
wants to merge 1 commit into from

Conversation

BurntSushi
Copy link

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

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

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
Copy link
Owner

kazarena commented Oct 5, 2016

@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
Copy link
Author

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
#12 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAb34tUJLla1yDofRwM2XXVdHYybY1FOks5qxA7ngaJpZM4KPCAl
.

@kazarena
Copy link
Owner

kazarena commented Oct 7, 2016

@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

@BurntSushi BurntSushi closed this Dec 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants