snyk / sweater-comb

API linting rules

@snyk/sweater-comb

“Sweats the small stuff, so you don’t have to. OpenAPI linting rules for Snyk APIs.”

What is Sweater Comb?

At Snyk, we’re starting an API Program whose goal it is to create a beautiful garden of repeatable & concise APIs that empower Snyk customers, partners, and Snyk’ers alike to easily and quickly build new experiences and products.

Such an API needs some guardrails to stay cohesive, consistent and “unsurprising” to its consumers, as the platform scales in the number of concepts it provides and the number of teams delivering them.

Sweater Comb is a tool which provides some of those guardrails with automation, initially by applying custom Optic CI rules to Snyk's OpenAPI specifications.

Read more about the project in Sweater Comb documentation.

Installation and Usage

npm install @snyk/sweater-comb --save-dev

Contributing

If you are contributing to Sweater Comb, read the contributing guidelines

License

Licensed under the Apache 2.0 License

Copyright (c) 2021-present, Snyk Ltd.

sweater-comb's Issues

Catch arrays without types

Earlier today there was a broken build in CircleCI where an OpenAPI spec was merged lacking an items: { type: ... } on an array property.

Spectral's provided OpenAPI rules did not catch it, but it failed vervet's compiler (which validates OpenAPI3 with github.com/kin-openapi/openapi3.

We need to add a rule to close this gap (and find any others in spectral's rules).

Consider opening a spectral bug.

[Optic CI] Kebab case rule does not allow single-word names

The kebab-case rule on header keys is failing on single-word keys, which should be allowed.

To reproduce: node_modules/.bin/ts-node src/index.ts compare --to end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml --context '{"changeDate":"2021-11-11","changeResource":"thing","changeVersion":"2021-11-10"}'

CC @acunniffe

Document HTTP 409 Conflict

We're using 409 in a couple of v3 API resources to indicate a DELETE isn't allowed because of existing references. Should update the docs to cover this.

Resources should include parent IDs

Our standards should generally require a parent ID (such as a tenant in the path) to be present in a resource's attributes.

Update: this may be better represented as a relationship link to the parent. Why? Because it provides the ID with additional context needed to walk the resource graph.

Should not be allowed to change stability

This should fail a check, but it currently passes:

node_modules/.bin/ts-node src/index.ts compare \
  --from end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml \
  --to end-end-tests/api-standards/resources/thing/2021-11-10/001-fail-stability-change.yaml \
  --context '{"changeDate":"2021-11-11","changeResource":"thing","changeVersion":"2021-11-10"}'

Basically, we do not want the stability level to be changed if the --from stability is experimental or higher.

We'll allow wip to be changed to a higher level, because wip stability is not exposed to users. And in general, wip should be exempt from breaking change rules for this reason.

Better name

bellybutton, sweater-comb, ...

Rules fail even if there is no change that violates them; unexpected behavior

If a rule (typically naming) is violated in both the from and to, Optic CI fails. The problem is, we have some API specs that landed prior to some of our standards being complete, and/or completely implemented in the Spectral rules.

Optic CI can be introduced into such a project if it applies rules to the changes between --from and --to; that would have the nice “forcing function” of requiring a team to release a new API version once they want to significantly change a non-compliant one. And, it’d be fairly non-blocking — if they complained, we could always say, “hey, no problem, cut a new version and do what you need to do!“.

To address the special case of "let's run all the rules on everything to see where we're at wrt conformance" we could then tell Optic CI "everything is new". But the normal CI case should be "only apply rules to spec changes in this PR".

To reproduce: See #115. Using fixture introduced there:

optic-ci compare --from 000-fail-naming.yaml --to 000-fail-naming.yaml should PASS and exit 0, because that is effectively "no change". Even though the spec introduced from scratch with optic-ci compare --to 000-fail-naming.yaml should FAIL.

Resources must be plural nouns

This will likely require some slight smarts because natural language, but all resource names should be plural nouns.

PUT not allowed

JSONAPI does not allow PUT requests, so we shouldn't either.

Document position on updating singleton resources

When updating singleton resources (there can be only one, like the Highlander), .data.id should be optional in the PATCH request body. This is a decision to depart from JSON API, which has this as an unconditional MUST.

Prevent back-dating release versions (within reason)

Back-dating resource versions can create all sorts of problems:

• Circumvention of rules (by exploiting effectiveOn)
• Misrepresenting lifecycle promises, skewing deprecation and sunset dates
• Surprising users with unexpected API changes

However, it is tricky to restrict release dates because:

• It can take several days for a PR to get reviewed, pass CI, and actually land
• Git commit dates may not represent the actual real-clock timeline (because of rebasing, etc)

Some possible approaches to this:

• Allow a grace period for back-dating, perhaps up to 5 days, to allow for long landing times?
• Allow post-dating versions?

I'm leaning toward a grace period for back-dating to allow for landing lag.

Tenancy rule failing when it seems like it shouldn't be

After landing #99, I had to update our e2e example specs in #101 to satisfy the org tenancy requirements.

Even with all paths having been prefixed by /orgs/{org_id}, a tenancy rule is still failing.

$ /Users/cmars/Projects/sweater-comb/node_modules/.bin/ts-node src/index.ts compare --from ./end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml --to ./end-end-tests/api-standards/resources/thing/2021-11-10/001-ok-add-property-field.yaml
...
  FAIL   This Specification
  x this specification:  must have an org or group tenant
    expected support for org or group tenant: expected [] to have a length above 0 but got 0
    at (/Users/cmars/Projects/sweater-comb/end-end-tests/api-standards/resources/thing/2021-11-10/001-ok-add-property-field.yaml:3:120)
...

[Optic CI] Required parameters should be allowed in new versions

We don't want to allow required parameters to be added once a version has been introduced -- but we need to be able to introduce them initially.

When running node_modules/.bin/ts-node src/index.ts compare --to end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml --context '{"changeDate":"2021-11-11","changeResource":"thing","changeVersion":"2021-11-10"}'

Define 202 Accepted response

Need to either:

• Define what our 202 responses look like, or
• Decide we're not going to allow them

Sunset rules not being enforced

The removal of an entire resource spec version should not be allowed, without a context indicating that the removed version is deprecated by a newer one, and is eligible for sunset based on the change date.

For example, this is from a test run driven by Vervet from a live example where the --from spec exists (sourced here from a git branch) but the working copy was deleted, so there is no --to argument. This should represent the complete removal of the spec, which should fail without an eligible deprecatedBy and changeDate in the --context:

2021/11/30 12:34:21 [run --rm -v /Users/cmars/Project:/from -v /var/folders/xg/5fst00tj1m7gxf88yxqvrhfr0000gp/T/718988177/ae02c198f5aca147653bcc553d511f76ec38fb5d:/from/src/resources/_examples/hello_world/2021-06-07/spec.yaml ghcr.io/snyk/sweater-comb:optic-main compare --context {"changeDate":"2021-11-30","changeResource":"hello_world","changeVersion":{"date":"2021-06-07","stability":"ga"}} --from /from/src/resources/_examples/hello_world/2021-06-07/spec.yaml]
Loading specifications for comparison:
Current specification: done
Next specification: done

It currently exits 0, but should have failed and displayed a lifecycle rule error.

CC @acunniffe Opening this to track; perhaps this is already fixed upstream?

`optic-ci compare` no longer works

Regression possibly introduced by #112.

optic-ci compare no longer works. To reproduce:

$ yarn run compare --to ./end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml 

Loading specifications for comparison:
Current specification: done
Next specification: done


  FAIL   Entire Resource
  x api lifeycle:  must lifeycle rules have to be followed
    Cannot read property 'date' of undefined
    Read more in our API guide (​https://github.com/snyk/sweater-comb/blob/main/docs/version.md​)
    at (/Users/cmars/Projects/sweater-comb/end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml:1:0)
  x api lifeycle:  must follow sunset rules
    Cannot read property 'stability' of undefined
    at (/Users/cmars/Projects/sweater-comb/end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml:1:0)


  PASS   Resource Document


  PASS   This Specification


  PASS  DELETE /orgs/{org_id}/thing/{thing_id}


  PASS  GET /orgs/{org_id}/thing


  PASS  GET /orgs/{org_id}/thing/{thing_id}


  PASS  PATCH /orgs/{org_id}/thing/{thing_id}


  PASS  POST /orgs/{org_id}/thing





1728 checks passed
2 checks failed

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

HTTP 405 for methods not supported

Some resources may not support all methods (POST, PATCH, DELETE) as well as others not included in our standards (HEAD, OPTIONS, etc).

Need to add:

• Response type for 405
• Guidance on when a caller might expect a 405
  • PATCH on an immutable resource
  • POST or DELETE on a resource whose lifecycle is determined by an external process? Is that even a thing we even want to allow?

Should we allow users to pass ?version=<stability-level>

This will result in breaking behaviour over time if they don't pass a date. Since we're still early, we could remove this behaviour from our spec and prevent customers from ever getting into a broken state.

"From scratch" compare (--to, no --from) fails breaking change rules

Since 9dcd17f adding a new resource version spec fails the "no breaking changes" rules around required parameters:

$ yarn ts-node src/index.ts compare --to ./end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml --context '{"changeDate": "2021-11-11", "changeResource": "thing", "changeVersion": "2021-11-10"}'
$ /Users/cmars/Projects/sweater-comb/node_modules/.bin/ts-node src/index.ts compare --to ./end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml --context '{"changeDate": "2021-11-11", "changeResource": "thing", "changeVersion": "2021-11-10"}'
yarn run v1.22.11
$ yarn ts-node src/index.ts compare --to ./end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml --context '{"changeDate": "2021-11-11", "changeResource": "thing", "changeVersion": "2021-11-10"}'
$ /Users/cmars/Projects/sweater-comb/node_modules/.bin/ts-node src/index.ts compare --to ./end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml --context '{"changeDate": "2021-11-11", "changeResource": "thing", "changeVersion": "2021-11-10"}'
...

  FAIL  DELETE /orgs/{org_id}/thing/{thing_id}
  x added query-parameter: version must not be required
    expected true to be false
    Read more in our API guide (​https://github.com/snyk/sweater-comb/blob/main/docs/version.md#breaking-changes​)
    at (/Users/cmars/Projects/sweater-comb/components/parameters/version.yaml:1:0)


  FAIL  GET /orgs/{org_id}/thing
  x added query-parameter: version must not be required
    expected true to be false
    Read more in our API guide (​https://github.com/snyk/sweater-comb/blob/main/docs/version.md#breaking-changes​)
    at (/Users/cmars/Projects/sweater-comb/components/parameters/version.yaml:1:0)


  FAIL  GET /orgs/{org_id}/thing/{thing_id}
  x added query-parameter: version must not be required
    expected true to be false
    Read more in our API guide (​https://github.com/snyk/sweater-comb/blob/main/docs/version.md#breaking-changes​)
    at (/Users/cmars/Projects/sweater-comb/components/parameters/version.yaml:1:0)


  FAIL  PATCH /orgs/{org_id}/thing/{thing_id}
  x added query-parameter: version must not be required
    expected true to be false
    Read more in our API guide (​https://github.com/snyk/sweater-comb/blob/main/docs/version.md#breaking-changes​)
    at (/Users/cmars/Projects/sweater-comb/components/parameters/version.yaml:1:0)


  FAIL  POST /orgs/{org_id}/thing
  x added query-parameter: version must not be required
    expected true to be false
    Read more in our API guide (​https://github.com/snyk/sweater-comb/blob/main/docs/version.md#breaking-changes​)
    at (/Users/cmars/Projects/sweater-comb/components/parameters/version.yaml:1:0)





2096 checks passed
7 checks failed
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Changing `format:` or `pattern:` on a string parameter or property should be a breaking change

Changing format or pattern should not be allowed within a resource version as it's a breaking change.

Example (using test cases introduced in #105):

$ diff end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml end-end-tests/api-standards/resources/thing/2021-11-10/001-fail-format-change.yaml
227c227,228
<         format: uuid
---
>         # Should fail: changing from a UUID to a base64 string is breaking
>         format: byte
274c275,277
<           example: thing
---
>           example: bob
>           # Should fail: introducing a more restrictive pattern is a breaking change
>           pattern: "^bob$"

Prevent in-place modification of version stability

A lifecycle version rule should prevent a version from being retroactively modified to change the stability.

For example, you can't "promote" 2021-06-04~experimental to 2021-06-04~beta, you have to release a new beta version dated to when the beta is made available.

See #52 for why this shouldn't be allowed.

bulk-compare does not allow "from-scratch" comparison

Since 9dcd17f, when the "from" is missing in a comparison, in bulk-compare input, the command errors out. This will be the case when a new resource version spec file is added.

input.yaml:

{
    "comparisons": [{
        "to": "end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml", "context": {"changeDate": "2021-11-11", "changeResource": "thing", "changeVersion": "2021-11-10"}
    }]
}

yarn ts-node src/index.ts bulk-compare --input input.yaml shows:

Comparison doesn't match expected format, found: {"to":"/Users/cmars/Projects/sweater-comb/end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml","context":{"changeDate":"2021-11-11","changeResource":"thing","changeVersion":"2021-11-10"}}

Do not allow additionalProperties: true free-form maps, generally

Unless specifically exempted, do not allow free-form additionalProperties: true maps, as these leave room open for unexpected responses.

Exceptions:

• JSON API meta
• Places where this is actually what we need, like tags

Exceptions will need to be made on a case-by-case basis. Default should be "don't allow this".

Can't add a new resource version at a higher stability than wip

The stability change rule seems to be catching a false positive on new specs when they are introduced at a level higher than wip. We do want to allow a new resource version to be introduced at any level -- it's just that once it has been, it cannot be changed.

$ /Users/cmars/Projects/sweater-comb/node_modules/.bin/ts-node src/index.ts compare --to ./end-end-tests/api-standards/resources/thing/2021-11-10/001-ok-add-property-field.yaml
Loading specifications for comparison:
Current specification: done
Next specification: done


  FAIL   Resource Document
  x published stability:  must not change unless it was wip
    expected undefined to equal 'experimental'
    at (/Users/cmars/Projects/sweater-comb/end-end-tests/api-standards/resources/thing/2021-11-10/001-ok-add-property-field.yaml:4:135)

Document pagination cursor format

Document the format of pagination cursors in our public APIs, for the purpose of consistency and interoperability. This will basically be {version}.{opaque contents}.

Position on sparse fieldsets

It may be necessary / useful for a client to request only a subset of attributes on a resource, especially when paging through a high volume of them.

JSON API provides for this with sparse fieldsets.

We need to document our position on these. My thinking is currently:

• Drop the square-bracket notation for reasons already stated in the docs: they're ugly (forces "percent" URL encoding, hard to read and write) and unnecessary (we don't allow compound documents, so there's no need to differentiate type)
• If fields is not defined as a parameter, resource does not support sparse fieldsets and returns everything.
• If fields is defined as a parameter:
  • It must be style: form, explode: false. This defines a comma-separated []string like ?fields=a,b,c.
  • No fields parameter present in the request means I'm not filtering fields, so "give me everything"
  • ?fields= means "I don't want any fields at all" (empty or omitted attributes)

Breaking changes need to take operations carried forward into account

In fixing snyk/vervet#99 we introduced a change to our resource version compilation which "carries forward" operations not explicitly redeclared from prior versions. This policy change was made based on feedback from various teams, who found redeclaring operations that do not change, to be tedious and error-prone.

This is fine, but it poses a new challenge for our "breaking changes" rules.

When a new resource version is introduced, it is OK to make a breaking change in that version.

However, once it has landed, it is NOT OK to add an operation to it which makes a breaking change on an operation already carried forward. Why? Because this is actually a breaking change in an existing release to an implicit operation (the one carried forward).

To illustrate, imagine a scenario like :

	2021-09-06~beta	2021-10-06~beta
Declares operations	GET,POST	GET
Effective operations	GET,POST	GET,POST @ 2021-09-06~beta

In the 2021-10-06~beta release, the POST from the prior release is "carried forward" to the next, even though it is not explicitly declared. When the versions are compiled to describe the entire services' API, these are copied and made explicit. However, the rules are operating on the source resource specs.

Two ways to fix this:

1. Change existing rules to be able to evaluate implicit operation changes as described above.
2. Run the rules on the vervet compiled specs to check this. This will require Vervet Underground integration w/Optic CI, as we do not commit our compiled OpenAPI to git.

Documentation: Pattern for public, but not advertised

There are several use cases for things that we want to be publicly available but not advertised. However, we still want these APIs to be advertised in our internal documentation portals. For example:

• An implementation of a webhook for another service that powers an integration
• An API used to ship analytics from our surface area into our data pipeline

We also should develop a set of rules for these things as well.

Cannot pass CI with pre-existing API spec that lacks tenancy params

When introducing Optic CI into an existing project, I found this rule failing with no changes:

  FAIL   This Specification
  x this specification:  must have an org or group tenant
    expected support for org or group tenant: expected [] to have a length above 0 but got 0
    Read more in our API guide (​https://github.com/snyk/sweater-comb/blob/main/docs/standards.md#organization-and-group-tenants-for-resources​)
    at (/Users/cmars/Projects/registry/src/web/routes/api/v3/resources/_examples/hello_world/2021-06-13/spec.yaml:1:0)

The paths are: /examples/hello_world, /examples/hello_world/{id}. They aren't in compliance, but they're also not changed in the CI diff.

We want to improve these, but if they've already landed, they shouldn't block introduction of Optic CI into the project. Tenancy rules may also need to evolve over time...

Clarification on PATCH 200 responses

Call out in standards doc, that a 200 response to a PATCH with full contents is OK. It's not forbidden by the spec, the use of meta is not a requirement as we interpret it.

Props to @gablaxian for catching this.

bulk-compare exits 0 on invalid input

If bulk-compare fails to process its inputs, it can exit 0. It should exit non-zero if it fails to run any of the tests due to configuration issues like this:

$ /Users/cmars/Projects/sweater-comb/node_modules/.bin/ts-node src/index.ts bulk-compare --input input.yaml
Reading input file...
Comparison doesn't match expected format, found: {"nope":"nope","context":{"changeDate":"2021-11-11","changeResource":"thing","changeVersion":"2021-11-10"}}
Bulk comparing


✨  Done in 3.77s.
zksnyk:sweater-comb cmars$ echo $?
0

One-to-many relationships

Standards documentation needs an opinion on one-to-many relationships. We need to navigate how we're going to represent this given the subset of JSON API we want to stay within.

Misleading naming rule error message

I think the error message should read "must have snake case keys" here:

  FAIL  GET /orgs/{org_id}/apps/{client_id}
  x requirement for field: clientId must have camel case keys
    expected false to be truthy
    at (/path/to/resources/schemas/models/app.yaml:21:432)
  x requirement for field: redirectUris must have camel case keys
    expected false to be truthy
    at (/path/to/resources/schemas/models/app.yaml:26:589)
  x requirement for field: isPublic must have camel case keys
    expected false to be truthy

CC @acunniffe

Documentation: Pattern for Async APIs

At Snyk, we have many APIs that represent Asynchronous Tasks (e.g. work that requires multiple steps and can't be done in a reasonable time span or must be retried). We need to develop a pattern for expressing this kind of task and then encode those rules into Sweater Comb.

[Optic CI] Type changes are breaking changes

If a property's type changes, this is a breaking change that requires a new version (lifecycle rule, CC @acunniffe)

Breaking change rule output when an operation is removed

This breaking change scenario fails, but the output is not very clear about what needs fixing.

Steps to reproduce:

$ node_modules/.bin/ts-node src/index.ts compare \
  --from end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml \
  --to end-end-tests/api-standards/resources/thing/2021-11-10/001-fail-operation-removed.yaml \
  --context '{"changeDate":"2021-11-11","changeResource":"thing","changeVersion":"2021-11-10"}' 2>&1 | pbcopy

Output:

Loading specifications for comparison:
Current specification: loading...
Next specification: loading...
�[2K�[1A�[2K�[1A�[2K�[1A�[2K�[GLoading specifications for comparison:
Current specification: done
Next specification: loading...
�[2K�[1A�[2K�[1A�[2K�[1A�[2K�[GLoading specifications for comparison:
Current specification: done
Next specification: done
running rules...
�[2K�[1A�[2K�[1A�[2K�[1A�[2K�[1A�[2K�[GLoading specifications for comparison:
Current specification: done
Next specification: done


  PASS  DELETE /thing/{thing_id}


  PASS  GET /thing


  PASS  GET /thing/{thing_id}


  FAIL  PATCH /thing/{thing_id}
  x removed response: undefined must not be removed
    Cannot read property 'statusCode' of undefined
  x removed response: undefined must not be removed
    Cannot read property 'statusCode' of undefined
  x removed response: undefined must not be removed
    Cannot read property 'statusCode' of undefined
  x removed response: undefined must not be removed
    Cannot read property 'statusCode' of undefined
  x removed response: undefined must not be removed
    Cannot read property 'statusCode' of undefined
  x removed response: undefined must not be removed
    Cannot read property 'statusCode' of undefined
  x removed response: undefined must not be removed
    Cannot read property 'statusCode' of undefined
  x removed response: undefined must not be removed
    Cannot read property 'statusCode' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined
  x removed field: undefined must not be removed
    Cannot read property 'key' of undefined


  PASS  POST /thing





1051 checks passed
88 checks failed

GET, POST, PATCH responses must not be empty

Operations for these HTTP methods must declare a response with content type application/vnd.api+json that is:

• Non-empty
• Conforms to JSON API structure

Currently an operation with no content will pass CI. This should only be allowed for DELETE operations.

This can also happen if content types other than application/vnd.api+json is declared.

[Optic CI] Snake case parameter name rule failing a valid param

thing_id should be a valid snake case param, but it's getting rejected:

To reproduce: node_modules/.bin/ts-node src/index.ts compare --to end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml --context '{"changeDate":"2021-11-11","changeResource":"thing","changeVersion":"2021-11-10"}'

"From scratch" compare (--to, no --from) fails lifecycle rules

Since 9dcd17f, adding a new resource version spec fails some versioning lifecycle rules:

yarn run v1.22.11
$ yarn ts-node src/index.ts compare --to ./end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml --context '{"changeDate": "2021-11-11", "changeResource": "thing", "changeVersion": "2021-11-10"}'
$ /Users/cmars/Projects/sweater-comb/node_modules/.bin/ts-node src/index.ts compare --to ./end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml --context '{"changeDate": "2021-11-11", "changeResource": "thing", "changeVersion": "2021-11-10"}'
...

  FAIL   Entire Resource
  x api lifeycle:  must lifeycle rules have to be followed
    Cannot read property 'date' of undefined
    Read more in our API guide (​https://github.com/snyk/sweater-comb/blob/main/docs/version.md​)
    at (/Users/cmars/Projects/sweater-comb/end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml:1:0)
  x api lifeycle:  must follow sunset rules
    Cannot read property 'stability' of undefined
    at (/Users/cmars/Projects/sweater-comb/end-end-tests/api-standards/resources/thing/2021-11-10/000-baseline.yaml:1:0)
...

Missing operation summary fails CI; fixing it triggers "invalid operation ID" (catch-22)

When introducing Optic CI into an existing project, in addition to #136 I found these failures:

  FAIL   This Specification
  x this specification:  must have an org or group tenant
    expected support for org or group tenant: expected [] to have a length above 0 but got 0
    Read more in our API guide (​https://github.com/snyk/sweater-comb/blob/main/docs/standards.md#organization-and-group-tenants-for-resources​)
    at (/Users/cmars/Projects/registry/src/web/routes/api/v3/resources/_examples/hello_world/2021-06-13/spec.yaml:1:0)


  FAIL  GET /examples/hello_world/{id}
  x requirement for operation: GET /examples/hello_world/{id} must have a summary
    expected undefined to exist
    Read more in our API guide (​https://github.com/snyk/sweater-comb/blob/main/docs/standards.md#operation-summary​)
    at (/Users/cmars/Projects/registry/src/web/routes/api/v3/resources/_examples/hello_world/2021-06-13/spec.yaml:65:2767)


  FAIL  POST /examples/hello_world
  x requirement for operation: POST /examples/hello_world must have a summary
    expected undefined to exist
    Read more in our API guide (​https://github.com/snyk/sweater-comb/blob/main/docs/standards.md#operation-summary​)
    at (/Users/cmars/Projects/registry/src/web/routes/api/v3/resources/_examples/hello_world/2021-06-13/spec.yaml:14:250)

Oh, easy fix, I thought, I'll go and add summaries to those operations, it's a non-breaking change, and we'll be good!

Well, no, when I added the summaries, it triggered this rule failure because the operation IDs are also non-standard:

  FAIL  GET /examples/hello_world/{id}
  x updated operation: GET /examples/hello_world/{id} must have the correct operationId format
    operationId "helloWorldGetOne" must be camelCase (helloWorldGetOne) and start with get|create|list|update|delete: expected false to be truthy
    Read more in our API guide (​https://github.com/snyk/sweater-comb/blob/main/docs/standards.md#operation-ids​)
    at (/Users/cmars/Projects/registry/src/web/routes/api/v3/resources/_examples/hello_world/2021-06-13/spec.yaml:66:2834)


  FAIL  POST /examples/hello_world
  x updated operation: POST /examples/hello_world must have the correct operationId format
    operationId "helloWorldCreate" must be camelCase (helloWorldCreate) and start with get|create|list|update|delete: expected false to be truthy
    Read more in our API guide (​https://github.com/snyk/sweater-comb/blob/main/docs/standards.md#operation-ids​)
    at (/Users/cmars/Projects/registry/src/web/routes/api/v3/resources/_examples/hello_world/2021-06-13/spec.yaml:14:250)

And I can't fix those without a breaking change. Even if I introduce a new release version that fixes this, I'm still not going to be able to get these existing operations to pass CI.

jsonapi-response-data doesn't allow for certain valid response data types

jsonapi-response-data is too naive and rigid, in that it expects the data property of a response to be an object with a type property. This doesn't apply when the type is constructed by composition with allOf, for example.

Workaround in https://github.com/snyk/registry/pull/23157/files#diff-f32495909ce2bc0b58612071fadcc121016885bec5130edf51cf151b65b3baa9R3

The "consistent operation IDs" rule doesn't allow fixing pre-existing non-compliant operation IDs

(This is using release tag v1.0.9)

I have an API spec in an existing project that I would like to improve and get into compliance with our standards. The consistent operation IDs rule seems to be preventing me from fixing them.

The rule failure is:

  FAIL  POST /examples/hello_world
  x updated operation: POST /examples/hello_world must have consistent operation IDs
    expected 'helloWorldCreate' to equal 'createHelloWorld'
    at (/Users/cmars/Projects/registry/src/web/routes/api/v3/resources/_examples/hello_world/2021-06-13/spec.yaml:14:250)

and this is the change I'm trying to get passing CI:

diff --git a/src/web/routes/api/v3/resources/_examples/hello_world/2021-06-13/spec.yaml b/src/web/routes/api/v3/resources/_examples/hello_world/2021-06-13/spec.yaml
index f619f7033a..64782d55b5 100644
--- a/src/web/routes/api/v3/resources/_examples/hello_world/2021-06-13/spec.yaml
+++ b/src/web/routes/api/v3/resources/_examples/hello_world/2021-06-13/spec.yaml
@@ -12,8 +12,9 @@ tags:
 paths:
   /examples/hello_world:
     post:
+      summary: Create a single result from the hello_world example
       description: Create a single result from the hello_world example
-      operationId: helloWorldCreate
+      operationId: createHelloWorld
       tags:
         - Examples
       parameters:

Sparse relationships

Similar to sparse fieldsets & #159, what if we want to express sparse relationships in JSON API?

A resource might relate to many other resources. However, in fulfilling a request, it might be expensive to provide all of them. Consider that each relationship might require a table join in order to determine existence and resolve the public identifier. Or worse yet, an internal service-to-service API request.

We can express sparse relationships using a relationships parameter, with a notation similar to sparse fieldsets.

• If relationships is not defined as a parameter, resource does not support sparse relationships and returns all of them if it has them.
• If relationships is defined as a parameter:
  • It must be style: form, explode: false. This defines a comma-separated []string like ?relationships=a,b,c.
  • No relationship parameter present in the request means I'm not filtering relationships, so "give me everything"
  • ?relationships= means "I don't want any relations at all" (empty or omitted relationships)

Given a resource like:

data: {
  id: some-id,
  type: some-resource,
  relationships: {
    org: {id: some-org, type: org, links: {related: /orgs/some-org}},
    group: {id: some-group, type: group, links: {related: /groups/some-group}},
    target: {id: some-target, type: target, links: {related: /targets/some-target}},
    snapshot: {id: some-snapshot, type: snapshot, links: {related: /snapshots/some-snapshot}},
    monitor: {id: some-snapshot, type: snapshot, links: {related: /snapshots/some-snapshot}},
  },
}

We might request a subset of these with GET /some-resource/some-id?relationships=org,group:

data: {
  id: some-id,
  type: some-resource,
  relationships: {
    org: {id: some-org, type: org, links: {related: /orgs/some-org}},
    group: {id: some-group, type: group, links: {related: /groups/some-group}},
  },
}

or none at all with GET /some-resource/some-id?relationships=:

data: {
  id: some-id,
  type: some-resource,
}

Section explaining the responsibility of API designers with regard to contract and application behavior

OpenAPI contracts can shape an application's API surface such that it cannot express invalid or unexpected results. With request and response validation (API governance), this can catch application changes that would break these contracts.

For example, an enumerated type in a resource property prevents unexpected and surprising results if new values creep in. Imagine a case where that enumerated value is assumed to be a small set of values, but is modeled as a free-form string.

In general, these are design principles that our standards documentation should cover.

CC @ianlivingstone @vibhaingit

Improve README

How to include / use in your project

Spectral OAS rules failing when they shouldn't be

The Spectral OAS integration seems to be failing on false positives. When I follow these "unused component" errors, they point at locally referenced components that are indeed referenced from the OpenAPI spec.

$ /Users/cmars/Projects/sweater-comb/node_modules/.bin/ts-node src/index.ts compare --to ./end-end-tests/api-standards/resources/thing/2021-11-10/001-ok-add-property-field.yaml
...
  FAIL   This Specification
...
  x requirement  must oas3-unused-component
    Potentially unused component has been detected.
    at (/Users/cmars/Projects/sweater-comb/end-end-tests/api-standards/resources/thing/2021-11-10/001-ok-add-property-field.yaml:150:8158)
  x requirement  must oas3-unused-component
    Potentially unused component has been detected.
    at (/Users/cmars/Projects/sweater-comb/end-end-tests/api-standards/resources/thing/2021-11-10/001-ok-add-property-field.yaml:158:8306)
  x requirement  must oas3-unused-component
    Potentially unused component has been detected.
    at (/Users/cmars/Projects/sweater-comb/end-end-tests/api-standards/resources/thing/2021-11-10/001-ok-add-property-field.yaml:167:8500)
  x requirement  must oas3-unused-component
    Potentially unused component has been detected.
    at (/Users/cmars/Projects/sweater-comb/end-end-tests/api-standards/resources/thing/2021-11-10/001-ok-add-property-field.yaml:175:8847)
  x requirement  must oas3-unused-component
    Potentially unused component has been detected.
    at (/Users/cmars/Projects/sweater-comb/end-end-tests/api-standards/resources/thing/2021-11-10/001-ok-add-property-field.yaml:183:9212)
  x requirement  must oas3-unused-component
    Potentially unused component has been detected.
    at (/Users/cmars/Projects/sweater-comb/end-end-tests/api-standards/resources/thing/2021-11-10/001-ok-add-property-field.yaml:196:9672)
  x requirement  must oas3-unused-component
    Potentially unused component has been detected.
    at (/Users/cmars/Projects/sweater-comb/end-end-tests/api-standards/resources/thing/2021-11-10/001-ok-add-property-field.yaml:202:9849)
  x requirement  must oas3-unused-component
    Potentially unused component has been detected.
    at (/Users/cmars/Projects/sweater-comb/end-end-tests/api-standards/resources/thing/2021-11-10/001-ok-add-property-field.yaml:206:9949)
...

Running spectral lint does not have these errors:

$ spectral lint ./end-end-tests/api-standards/resources/thing/2021-11-10/001-ok-add-property-field.yaml
OpenAPI 3.x detected
OpenAPI 3.0.x detected

/Users/cmars/Projects/sweater-comb/end-end-tests/api-standards/resources/thing/2021-11-10/001-ok-add-property-field.yaml
 5:6  warning  info-contact      Info object should contain `contact` object.                             info
 5:6  warning  info-description  OpenAPI object info `description` must be present and non-empty string.  info

✖ 2 problems (0 errors, 2 warnings, 0 infos, 0 hints)

Resource content must go under .data.attributes

For request bodies and responses.

In other words, we don't want API authors to mistakenly put their properties in the top-level object, or in .data. The resource "contents" needs to go under .data.attributes. This is a core JSON API principal (is it called such? I'm inferring it as such!) that separates "transport-layer" and "application-layer".

Allow relationship self links

Decide if allowing relationship self-links is worth it for the purpose of:

• Querying through a relationship
• Updating through a relationship

We currently do not have a strong use case for these, but may need to develop support for this as we grow the API.

bulk-compare should exit non-zero on any failed rule

When bulk-compare finds a failed rule, it needs to exit non-zero so that we can fail the CI build. It currently exits 0 even on failures. For example, at the end of a run:

293 checks passed
2 checks failed


$ echo $?
0

Documentation: Settings as a Pattern

There are many different types of settings throughout the Snyk data model. Generally, settings belong to a tenant (e.g. group or org) and can be thought of things such as configuration or policy depending on what the setting does. We want to develop a pattern for how products should be configured based on the owner of the setting.