GoogleChrome / lighthouse

Automated auditing, performance metrics, and best practices for the web.
https://developer.chrome.com/docs/lighthouse/overview/
Apache License 2.0
28.39k stars 9.38k forks source link

[SEO Audits] Structured data is valid #4359

Open rviscomi opened 6 years ago

rviscomi commented 6 years ago

Audit group: Content Best Practices Description: JSON-LD structured data syntax is valid Failure description: JSON-LD structured data syntax is invalid Help text: Structured data contains rich metadata about a web page. The data may be used to provide more context to users in search results or social sharing. This audit is only checking for JSON-LD structured data syntax errors. For a checkup of Google search eligibility, try the Structured Data Testing Tool.

Success conditions:

Notes:

rviscomi commented 6 years ago

@kdzwinel let's start by setting up a new repo on your personal GH account and getting the plumbing to work so that it is registered on npm and installed by LH. You can send me any code reviews outside of the LH repo.

rviscomi commented 6 years ago

TODO: Is there anything we can reuse from Chromium's "copyless paste" feature, which does JSON-LD parsing? https://github.com/chromium/chromium/search?utf8=%E2%9C%93&q=json-ld&type=

paulirish commented 6 years ago

What's in scope for the first pass here? The three checkboxes above?

Also, do we know how much of the heavy lifting can be done by existing libraries?

rviscomi commented 6 years ago

Some of the lifting can be done with existing projects, eg https://github.com/zaach/jsonlint, assuming the license is compatible. @kdzwinel could you set up some benchmarks and see how good it is? Depending on it directly is probably fine for v1 but we'd also like to fold in other structured data schemas beyond JSON-LD, so it may be worth setting up a standalone SD validator repo anyway and getting the plumbing set up.

paulirish commented 6 years ago

I do see quite a few json-ld projects already, so I was mostly curious on that side. Seems like JSONlint would be good to provide better diagnostic data than what JSON.parse does; seems good if we're finding a lot of invalid JSON.

The three checkboxes above are what you're scoping this MVP to?

rviscomi commented 6 years ago

Oh I actually linked to the wrong project. jsonlint does look interesting and may help. I meant to link to https://github.com/digitalbazaar/jsonld.js, which is JSON-LD specific.

As for the MVP, the first two (valid JSON, standard props) are required and the third is a stretch goal.

Paul, on the subject of an external repo, I was curious to hear your thoughts on internal vs external. My hope/expectation is for the structured data validation code to be reusable in other contexts and also maintained by the web community. I think an external repo would be more conducive to those things, eg how the axe a11y logic is external to LH. WDYT?

kdzwinel commented 6 years ago

Paul, on the subject of an external repo, I was curious to hear your thoughts on internal vs external.

We have been talking a lot on the engineering meeting about solutions such as lerna, yarn workspaces and doing what we are doing with lh-logger (same repo, different package), but I haven't asked why we don't want a separate repo in the first place?

paulirish commented 6 years ago

breaking out the seperate repo discussion into #4955

kdzwinel commented 6 years ago

Small update.

jsonld.js can do some transforms on JSON-LD structures, but I don't see how we can use it for reporting errors. BTW this package has a non-standard license.

I've built a first prototype that checks:

And I found a first production error 😄 (already pinged Eric about it)

screen shot 2018-04-18 at 15 01 45

And a second one 😱 Who would have thought that simple JSON validation will be so useful here.

screen shot 2018-04-18 at 15 21 51
kdzwinel commented 6 years ago

I've put togheter technical decisions and other notes for future reference.

JSON Validation

JSON.parse: ✅ +0kb ❌ doesn't explain what's wrong (in some cases it does tell you where error occurs though e.g. "SyntaxError: Unexpected token b in JSON at position 11") ❌ doesn't catch duplicated keys

https://github.com/zaach/jsonlint (popular, unmaintained, MIT) : ⚠️+12kb ✅explains what's wrong ❌ doesn't catch duplicated keys (https://github.com/zaach/jsonlint/issues/13)

https://github.com/circlecell/jsonlint-mod (fork of the previous one): ⚠️+12kb ✅explains what's wrong ✅catches duplicated keys

JSON-LD doesn't support duplicated keys, so jsonlint-mod was the best choice. It isn't perfect though: error messages are hard to customize and it calls console.error which litters console output. We may want to create our own fork in the long run.

JSON-LD Validation

There are no json-ld linters/validators that I know of (ld-validate that Paul found, doesn't check the json-ld grammar, but rather lets you create and validate object constraints), so for MVP we only do couple of checks. We make sure that:

Schema.org Validation

Using raw data from schema.org we are checking:

ℹ️ Objects that are using some other vocabulary than schema.org (e.g. http://rdf.data-vocabulary.org/) are ignored.

Google Recommendations Validation

Using data scraped from SDTT we are checking if all required and recommended fields are present on schema.org objects.

This is not a long term solution - in the future we would like to use something like shex to validate not only the required/recommended fields, but also their values. We would also like to also support recommendations coming from other companies (Bing/Pinterest/Apple/Yandex etc.).

kdzwinel commented 6 years ago

WIP - if anyone is interested in testing it: https://github.com/kdzwinel/lighthouse/tree/json-ld

screen shot 2018-05-08 at 16 42 29
AymenLoukil commented 6 years ago

Hello, Why not consuming the Google Structured Data Testing tool for that ? No API available ?

rviscomi commented 6 years ago

@AymenLoukil SDTT is closed source and does not provide an API. We're also hoping to make this audit relevant for all search engines, as they all handle structured data bit differently. It's kind of the wild west in terms of consistency 🤠

AymenLoukil commented 6 years ago

@rviscomi OMG how is that closed source 🚪 ? It belongs to Google no ? Why not reuse and enhance.. Anyway, http://jsonapi.org/format/1.1/ may helps in validating JSON/ JSON-LD format.

danbri commented 6 years ago

Objects that are using some other vocabulary than schema.org (e.g. http://rdf.data-vocabulary.org/) are ignored.

I would encourage also acceptance (or not actively complaining) of markup that links a Schema.org term to a non-Schema.org term. For an example I've been looking at using schema.org/Dataset with a mainEntity property whose value is modeled as a W3C csvw:Table. While it is possible that such markup might be in error, it is also good to accept such possibilities as being potentially sensible.

rviscomi commented 6 years ago

@kdzwinel for now let's take out the recommendations based on SDTT scraping. That will help ensure that we don't lag behind recommendations from SDTT in case it changes.

rviscomi commented 6 years ago

@kdzwinel See https://github.com/GoogleChrome/lighthouse/issues/4359#issue-291723379 - I've updated the audit title and help text.

danbri commented 5 years ago

Is this functionality still under development?

rviscomi commented 5 years ago

@danbri the core functionality is complete and awaiting review. See #6750.

Beyond that, we're working on a new results renderer to more clearly show users the JSON-LD errors in the context of the code snippet. See #6901 and #6999.

image

mattzeunert commented 5 years ago

A few observations after running this on a few sites:

1. Case sensitivity

This is a failure because there's no aggregateRating type, just a Thing.aggregateRating property. The type should be AggregateRating. It might not be clear to the user though, because the schema.org URL exists.

Screenshot 2019-04-11 at 09 42 08

JSON-LD is case sensitive, but the Structured Data Testing Tool doesn't complain about it.

2. Empty elements

There's a few JSON-LD elements with no content or just {}. SDTT doesn't care, and it probably just means there's no data, so nothing we need to do I think.

3. Parser errors

I've only seen one example of this, but if you try to escape a single quote in the JSON our error message isn't super helpful. (JSON only allows a few escape sequences.)

Screenshot 2019-04-11 at 10 21 35

You get the same message if you have line breaks in your strings or a trailing comma somewhere. Maybe prefixing the message with "Parse error: " would make it clearer what the problem is.

mmocny commented 5 years ago

Hey folks, I poked around with the patch this morning to see how this is progressing (just a quick look so far -- looks good!)

One thing I found was that the audit didn't detect externalized structured data: e.g. <script type="application/ld+json" src=...> or <link rel="alternate" type="application/ld+json" href=...>

I'm not actually sure if this is meant to be supported. WDYT?

I suspect that this could be fixed by changing this block which currently only handles script with inline content.

EDIT: apparently it's not allowed to use <script src=> for data (per HTML spec)[1].

[1] "When used to include data blocks, the data must be embedded inline, the format of the data must be given using the type attribute, and the contents of the script element must conform to the requirements defined for the format used. The src, charset, async, defer, crossorigin, and nonce attributes must not be specified."

danbri commented 5 years ago

Fwiw Google also does not currently consume structured data published that way

On Fri, 16 Aug 2019 at 16:04, Michal Mocny notifications@github.com wrote:

Hey folks, I poked around with the patch this morning to see how this is progressing (just a quick look so far -- looks good!)

One thing I found was that the audit didn't detect externalized structured data: e.g. Githubissues.

  • Githubissues is a development platform for aggregating issues.