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

Validation fails for schemas when used in mixed API context #348

Closed
naz opened this issue Oct 2, 2020 · 3 comments · Fixed by #349
Closed

Validation fails for schemas when used in mixed API context #348

naz opened this issue Oct 2, 2020 · 3 comments · Fixed by #349
Assignees
Labels
bug Something isn't working

Comments

@naz
Copy link
Contributor

naz commented Oct 2, 2020

Issue Summary

Data validation cashes schema definition from single API which leads to incorrect validation results. In other words, valid data for v3 API is getting validated against v2 schema.

To Reproduce

  1. Validate post resource data against v2 schema
await jsonSchema.validate({
    data: {
        posts: [{updated_at: '2020-10-02', title: 'whatever'}]
    },
    schema: `posts-edit`,
    version: 'v2'
});
  1. Validate posts which is only valid in v3 API (add email_subject field)
await jsonSchema.validate({
    data: {
        posts: [{updated_at: '2020-10-02', email_subject: 'only available in canary}]
    },
    schema: `posts-edit`,
    version: 'canary'
});
  1. Validation error happens even though email_subject is a valid field for canary/v3 schemas.

Expected behavior is - validation should always match schema definition and not throw errors for valid data.

The cause for a bug is schema caching which done on validator/ajv level.

Technical details:

  • Ghost Version: 3.35
  • Node Version: 12
  • Browser/OS: Ubuntu/FF(latest)
  • Database: MySQL 5.7
@naz naz added the bug Something isn't working label Oct 2, 2020
@naz naz self-assigned this Oct 2, 2020
@naz
Copy link
Contributor Author

naz commented Oct 2, 2020

There were couple approaches considered as a fix:

  1. Having separate admin-api-validator instance per API - this would ensure schemas are never mixed in the cache.
  2. Use additional prefix/postfix when identifying $id in schema

1st approach pros/cons

pros:

  • no need to modify schemas
  • no maintenance for prefix/postfix when new version is released

cons:

  • package API needs to change to create a separate instance of ajv validator
  • need to change Ghost core to use new API
  • adds burden on the client user to "remember" to have a separate instance per API, and not being able to retreive/validate schemas on the same validator instance (unfriendly dev-experience imo)

2nd approach pros/cons

pros:

  • no changes to API or clients
  • keeps full functionality and single interface to access schemas/validations form any API version

cons:

  • need to maintain API prefix/postfix in the schema when creating new schema version folder

Given that the con from 2nd approach only happens 1 time a year (during new version release) and is pretty straight forward search/replace will go down this path. Also, there's possibility to automate this with a script if it's ever painful! IMO, keeping friendly, no "hidden tricks" package api and less changes in Ghost core far outweighs small maintenance burden.

@naz
Copy link
Contributor Author

naz commented Oct 2, 2020

Considerations around prefix or postfix naming change

There's not much difference in either approach. The only reason going with postfix notation $id: "{resource}.{version}" e.g.: "$id": "labels.canary" is to avoid regressions in existing use-case where error message is formed here.

@naz
Copy link
Contributor Author

naz commented Oct 2, 2020

For reference "$id" property naming conventions for JSON Schema - https://json-schema.org/understanding-json-schema/structuring.html#the-id-property. There is no specific convention enforced except:

The $id property should never be the empty string or an empty fragment (#), since that doesn’t really make sense.

And it's primary goal is:

The $id property is a URI-reference that serves two purposes:

  • It declares a unique identifier for the schema.
  • It declares a base URI against which $ref URI-references are resolved.

naz added a commit to naz/Ghost-SDK that referenced this issue Oct 2, 2020
naz added a commit to naz/Ghost-SDK that referenced this issue Oct 2, 2020
closes TryGhost#348

- $id property is meant to be a unique identifier: "It declares a unique identifier for the schema." (ref. https://json-schema.org/understanding-json-schema/structuring.html#the-id-property)
- Previously uniqueness was kept only per folder "version" (v2/canary) but as validator is meant to be used across versions that uniqueness is no longer enough
- The $id clash was effecting schema caching inside ajv. Having unique $id fixes the clash
@naz naz closed this as completed in #349 Oct 2, 2020
naz added a commit that referenced this issue Oct 2, 2020
closes #348

- $id property is meant to be a unique identifier: "It declares a unique identifier for the schema." (ref. https://json-schema.org/understanding-json-schema/structuring.html#the-id-property)
- Previously uniqueness was kept only per folder "version" (v2/canary) but as validator is meant to be used across versions that uniqueness is no longer enough
- The $id clash was effecting schema caching inside ajv. Having unique $id fixes the clash
naz added a commit to TryGhost/Ghost that referenced this issue Oct 2, 2020
refs TryGhost/SDK#348

- There was an issue with schema clashing in the cach that caused validation to be run agains wrong API version
datucommit pushed a commit to datucommit/Ghost that referenced this issue Oct 2, 2020
refs TryGhost/SDK#348

- There was an issue with schema clashing in the cach that caused validation to be run agains wrong API version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant