core(jsonld): Structured data validation updates #8137

mattzeunert · 2019-04-10T13:07:14Z

Summary

Updates to the SD validation logic:

add line numbers to error messages
store invalid entity type for "unexpected property" errors

We use the entity type to include them in the error messages and link to their schema.org URL so the user can see what properties are valid:

From the audit's perspective the validator would ideally generate the full error message including the markdown link, but that logic is in the audit to make the sd validator more re-usable.

Related Issues/PRs

Part of #4359

patrickhulce

hurray for having some real usage-driven changes here now! :D 🎉

patrickhulce · 2019-04-10T14:34:02Z

lighthouse-core/lib/sd-validation/jsonld-keyword-validator.js

@@ -41,7 +41,7 @@ module.exports = function validateJsonLD(json) {
    if (name.startsWith('@') && !VALID_KEYWORDS.has(name)) {
      errors.push({
        path: path.join('/'),
-        message: 'Unknown keyword',
+        message: 'Unknown keyword ' + name,


should we put this in quotes or after a colon or something? this also makes we wonder what our i18n story is here 😬

lighthouse-core/lib/sd-validation/json-linter.js

patrickhulce · 2019-04-10T14:36:16Z

lighthouse-core/test/lib/sd-validation-test.js

@@ -17,7 +17,7 @@ describe('JSON validation', () => {
    `);

    assert.equal(errors.length, 1);
-    assert.equal(errors[0].path, 2);


we should probably use strict equal here then, these would have been failing before I guess

lighthouse-core/lib/sd-validation/sd-validation.js

patrickhulce · 2019-04-10T14:38:05Z

lighthouse-core/lib/sd-validation/sd-validation.js

+ * @returns null | number - line number of the path value in the prettified JSON
+ */
+function getLineNumberFromJsonLDPath(obj, path) {
+  // To avoid having an extra dependency on a JSON parser we set a unique key in the


has that shipped sailed with jsonlint-mod is there any way to leverage that?

also deja vu, I feel like I've reviewed this before so sorry if you've answered this in the previous PR

has that shipped sailed with jsonlint-mod is there any way to leverage that?

I don't see a direct way to leverage jsonlint-mod here. It just uses Douglas Crockford's JSON parser.

I guess we could parse it ourselves or find a small parser that we can include?

also deja vu, I feel like I've reviewed this before so sorry if you've answered this in the previous PR

Yup, I sent you a pre-PR to get some initial feedback.

lighthouse-core/lib/sd-validation/sd-validation.js

patrickhulce · 2019-04-10T14:43:07Z

lighthouse-core/lib/sd-validation/sd-validation.js

+      // but key provided by validator is "author"
+      const keyParts = key.split('/');
+      const relativeKey = keyParts[keyParts.length - 1];
+      if (relativeKey === pathPart && currentObj[key] !== undefined) {


seems like if we have currentObj[key] as undefined we could just return before this whole loop

True, I don't see why we'd need it at all actually...

patrickhulce · 2019-04-10T14:44:30Z

lighthouse-core/lib/sd-validation/sd-validation.js

+        } else {
+          currentObj = currentObj[key];
+        }
+        keyFound = true;


seems like keyFound is unnecessary, we could just return here and throw if we ever make it through the loop without returning

patrickhulce · 2019-04-10T14:46:05Z

lighthouse-core/test/lib/sd-validation-test.js

@@ -169,7 +171,9 @@ describe('schema.org validation', () => {
    }`);

    assert.equal(errors.length, 1);
+    assert.equal(errors[0].invalidTypes[0], 'http://schema.org/Article');


this name is a bit confusing to me for how it's used. the type isn't invalid, right? it's just the type of the entity that has some other invalid stuff going on?

The entity is not a valid type instance because it has a property that doesn't exist on type.

I originally just called that array types, but then it wasn't clear that having that array means invalid. (And we use that array to generate the Invalid Event: unexpected property asdf message.)

Maybe we can call the array invalidForTypes?

oh gotcha, I actually like the direction you were headed with .types then. I think it's clear something is invalid by virtue of the fact that we are giving an error. Maybe typeOfInvalidEntity or something similar if we're going the more explicit route?

I'm at validTypes right now, since we only include the types in the validation error if they are valid. (And we don't want to show invalid types in the UI, because in that case they'll be part of the error message.)

patrickhulce · 2019-04-10T14:49:13Z

lighthouse-core/lib/sd-validation/schema-validator.js

        errors.push({
+          invalidTypes: error.invalidTypes,
+          message: error.message,
          // get rid of the first chunk (/@type) as it's the same for all errors
          path:
            '/' +
            path
              .slice(0, -1)


wait a second, the path is backwards? I think we're missing some tests that have paths of length >2 then 🤔

I can look into this more tomorrow, but what makes you think the path is backwards? Here's an example of the path:

[ 'http://schema.org/author', '0', 'http://schema.org/colleague', '0', '@type' ]

For this:

{ "@context": "https://schema.org", "@type": "Article", "author": { "@type": "Person", "displayNaame": "Sally", "colleague": { "@type": "Person", "displayNaame": "Sally" } } }

Oh the comment confused me. It says it's getting rid of the "first chunk" but it's removing the last chunk. That made me think it is backwards and all paths started with /@type, but I see now :)

maybe update the comment? a test for longer paths still couldn't hurt though sometime :)

Oooh, yeah good catch, that's a very confusing comment! Added a test case for deeper nesting too.

mattzeunert · 2019-04-11T10:41:41Z

I don't think this can happen if the input is a JSON-LD object and the path is from the expanded version of that object. Should we just take it out?

brendankenny

this is looking good so far to me, just some surface level comments

brendankenny · 2019-04-12T01:11:02Z

types/structured-data.d.ts

+        | "json"
+        | "json-ld"
+        | "json-ld-expand"
+        | "schema-org";


ok, where is this leading | coming from :P Not even prettier does this, I don't think

my prettier definitely does this in other projects, not sure about here :)

my prettier definitely does this in other projects, not sure about here :)

maybe it's a typescript prettier thing? I couldn't get it to repro in their playground

brendankenny · 2019-04-12T01:20:20Z

lighthouse-core/lib/sd-validation/schema-validator.js

@@ -49,7 +49,7 @@ function findType(type) {
 *
 * @param {string|Array<string>} typeOrTypes
 * @param {Array<string>} keys
- * @returns {Array<string>}
+ * @returns {Array<{message: string, key?: string, typesOfInvalidEntity?: Array<string>, path?: string}>}


maybe pull into a typedef so these properties can have docs?

brendankenny · 2019-04-12T01:27:25Z

lighthouse-core/lib/sd-validation/sd-validation.js

+ * @param {string} path
+ * @returns {null | number} - line number of the path value in the prettified JSON
+ */
+function getLineNumberFromJsonLDPath(obj, path) {


these two are utility-y enough, maybe they're worth exposing and having explicit tests for them?

They're getting test coverage right now but I doubt it's much, and it would have the benefit of documenting them a bit more. e.g. these aren't called with literals anywhere, so it's not possible to know what the path string generally looks like without also looking at the code that generates paths.

Yeah, adding some tests is a good idea 👍

brendankenny

LGTM!

mattzeunert · 2019-04-16T09:07:26Z

@patrickhulce LGTY?

patrickhulce

Oh sorry, yes! LGTM :)

JSON-LD validation updates

32840b0

mattzeunert requested review from brendankenny and paulirish as code owners April 10, 2019 13:07

Fix indentation

85c8f5c

patrickhulce suggested changes Apr 10, 2019

View reviewed changes

mattzeunert added 2 commits April 10, 2019 17:18

Brackets

50b8a5e

Remove keyFound

5e065aa

connorjclark changed the title ~~core(jsonld): Structured data validation udpates~~ core(jsonld): Structured data validation updates Apr 10, 2019

mattzeunert added 7 commits April 11, 2019 08:01

Merge branch 'master' into sd-validation-update

1275d95

strictequal line numbers

048d1f2

Quotes for unknown keyword

fae6c39

Remove undefined check

700c4b8

Colon after "unrecognized schema org type"

b4b4271

Fix comment

626c77d

Test case for longer paths

d699fa3

mattzeunert closed this Apr 11, 2019

mattzeunert reopened this Apr 11, 2019

mattzeunert added 2 commits April 11, 2019 11:50

invalidTypes -> typesOfInvalidEntity

a3e00ae

No need to throw in unreachable place

7becbc7

brendankenny reviewed Apr 12, 2019

View reviewed changes

mattzeunert added 5 commits April 12, 2019 09:05

Single line TS string union

cb7e9cc

Re-use ValidationError type

e62b7fd

Test cases for getLineNUmberFromJsonLDPath

37e9236

typesOfInvalidEntity -> validTypes

1efcefd

Re-use ValidationError type

545ee2f

brendankenny approved these changes Apr 13, 2019

View reviewed changes

patrickhulce approved these changes Apr 16, 2019

View reviewed changes

patrickhulce merged commit c9668d7 into master Apr 16, 2019

patrickhulce deleted the sd-validation-update branch April 16, 2019 15:15

mmocny mentioned this pull request Aug 21, 2019

[SEO Audits] Structured data is valid #4359

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(jsonld): Structured data validation updates #8137

core(jsonld): Structured data validation updates #8137

mattzeunert commented Apr 10, 2019

patrickhulce left a comment

patrickhulce Apr 10, 2019

patrickhulce Apr 10, 2019

patrickhulce Apr 10, 2019

patrickhulce Apr 10, 2019

mattzeunert Apr 10, 2019

patrickhulce Apr 10, 2019

mattzeunert Apr 11, 2019

patrickhulce Apr 10, 2019

patrickhulce Apr 10, 2019

mattzeunert Apr 10, 2019

patrickhulce Apr 10, 2019

mattzeunert Apr 12, 2019

patrickhulce Apr 10, 2019

mattzeunert Apr 10, 2019

patrickhulce Apr 10, 2019

mattzeunert Apr 11, 2019

mattzeunert commented Apr 11, 2019

brendankenny left a comment •

edited

Loading

brendankenny Apr 12, 2019

patrickhulce Apr 12, 2019

brendankenny Apr 13, 2019

brendankenny Apr 12, 2019

brendankenny Apr 12, 2019

mattzeunert Apr 12, 2019

brendankenny left a comment

mattzeunert commented Apr 16, 2019

patrickhulce left a comment

core(jsonld): Structured data validation updates #8137

core(jsonld): Structured data validation updates #8137

Conversation

mattzeunert commented Apr 10, 2019

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattzeunert commented Apr 11, 2019

brendankenny left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

mattzeunert commented Apr 16, 2019

patrickhulce left a comment

Choose a reason for hiding this comment

brendankenny left a comment •

edited

Loading