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

Feature/id reference for feeds validation #412

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

civsiv
Copy link
Contributor

@civsiv civsiv commented Jun 9, 2022

Closes #408 and #413

@civsiv civsiv changed the base branch from master to feature/id-reference-validation June 9, 2022 14:18
Base automatically changed from feature/id-reference-validation to master July 16, 2022 00:05
Copy link
Contributor

@nickevansuk nickevansuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies this has taken so long to get to review!

model.hasSpecification = true;

it('should validate that superEvent within the ScheduledSession is a ID reference, and not an object', async () => {
const options = new OptionsHelper({ validationMode: 'RPDEFeed', rpdeKind: 'ScheduledSession' });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, I think we might have crossed wires slightly in our design chat on this...

There are some challenges with specifying rpdeKind at the very top level:

  1. We'd need to have the kind selectable or attributed to a validationMode in the validator GUI or have a different mode selection), or add an to that GUI that the RPDE kind of all items is the same as the first item (which isn't in the RPDE spec)
  2. The error messages will be unclear as the kind will be hidden from view if the item level validation (linked from validation results HTML in the test suite) is still shown in the GUI at the data level as now.
  3. As a separation of concerns, the validationMode is something that the data-model-validator cannot possibly know, as it's about the request/response context in which it finds the JSON. However the rpdeKind could be known to the validator, as it can be gleaned from the JSON itself.
  4. It precludes feeds with mixed RPDE item kinds from being validated in future.

So as an alternative proposal, I'd recommend we make the following changes:

  1. Equip the validator to validate the RPDE item level (e.g. the example JSON below)
  2. Update the rules here to look at the RPDE item kind in the parent, rather than looking at a global rpdeKind setting
  3. Update the test suite to pass the RPDE item (rather than just the contents of its data property) to the validator from the feed harvesting, and to include this in the encoded JSON in the links within the HTML validation output
  4. Also test this works when passing a whole feed page to the validator (as in the validator GUI samples dropdown), as each RPDE item should provide the kind for this rule running on its data.

So hence following example should pass the validator:

{
  "state": "updated",
  "kind": "ScheduledSession",
  "id": "C5EE1E55-2DE6-44F7-A865-42F268A82C63",
  "modified": 1521565719,
  "data": {
    "@context": [
      "https://openactive.io/",
      "https://openactive.io/ns-beta"
    ],
    "@type": "ScheduledSession",
    "@id": "https://opensessions.io/api/session-series/1402CBP20150217#/subEvent/C5EE1E55-2DE6-44F7-A865-42F268A82C63",
    "identifier": "C5EE1E55-2DE6-44F7-A865-42F268A82C63",
    "superEvent": "https://opensessions.io/api/session-series/1402CBP20150217",
    "startDate": "2016-05-09T18:15:00Z",
    "endDate": "2016-05-09T19:15:00Z",
    "duration": "PT1H",
    "eventStatus": "https://schema.org/EventScheduled",
    "maximumAttendeeCapacity": 10,
    "remainingAttendeeCapacity": 0,
    "url": "https://bookwhen.com/hulafit/e/ev-ssyp-20160510011500?r=oa"
  }
}

const ValidationErrorType = require('../../errors/validation-error-type');
const ValidationErrorSeverity = require('../../errors/validation-error-severity');

describe('IdReferencesForCertainFeedsRule', () => {
Copy link
Contributor

@nickevansuk nickevansuk Jul 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these rules, see the refactoring of #407.

Recommend refactoring to separate the request/response concerns (which should be covered by the validation mode), so that these rules rely purely on the validation files.

Note although #407 limits the @id reference rules to apply only to certain validation modes as an initial release, once this PR is completed suggest that we update them to apply to all validation modes (i.e. targetValidationModes = '*')

'FacilityUse/Slot',
'IndividualFacilityUse/Slot',
],
imperativeConfigurationWithContext: {
Copy link
Contributor

@nickevansuk nickevansuk Jul 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this same property is being used here for both RPDE kind and parent, suggest this data models property should be renamed imperativeConfigurationByRpdeKind (and perhaps for clarity the existing property renamed to imperativeConfigurationByParentPropertyName or something more intuitive?)

imperativeConfigurationWithContext is only used within the data-model-validator, so should not have any impact on other dependencies

This should make the configuration clearer

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.

2 participants