feat: removed the `type:none` sourceDescriptions Arazzo extension #1773

DmitryAnansky · 2024-10-17T13:04:55Z

What/Why/How?

Removed the type:none sourceDescriptions Arazzo extension.
Add additional lint rule for sourceDescriptions:

sourceDescriptions	[Source Description Object]	REQUIRED. A list of source descriptions (such as an OpenAPI description) this Arazzo Description SHALL apply to. The list MUST have at least one entry.

Reference

https://github.com/Redocly/redocly/issues/11343

Testing

Screenshots (optional)

Check yourself

Code changed? - Tested with redoc/reference-docs/workflows (internal)
All new/updated code is covered with tests
New package installed? - Tested in different environments (browser/node)

Security

Security impact of change has been considered
Code follows company security practices and guidelines

changeset-bot · 2024-10-17T13:04:58Z

🦋 Changeset detected

Latest commit: 9affd5a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@redocly/cli	Patch
@redocly/openapi-core	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-10-17T13:06:25Z

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
`redocly lint packages/core/src/benchmark/benches/rebilly.yaml`	980.2 ± 52.1	941.2	1118.9	1.01 ± 0.06
`redocly-next lint packages/core/src/benchmark/benches/rebilly.yaml`	972.0 ± 17.5	938.4	993.4	1.00

github-actions · 2024-10-17T13:07:35Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	78.57%	4995/6357
🟡	Branches	67.19%	2060/3066
🟡	Functions	73.03%	826/1131
🟡	Lines	78.88%	4712/5974

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / sourceDescription-type.ts	87.5%	50%	100%	100%
🟢	`...` / sourceDescriptions-not-empty.ts	100%	100%	100%	100%

Test suite run success

811 tests passing in 122 suites.

Report generated by 🧪jest coverage report action from 9affd5a

docs/rules/arazzo/sourceDescriptions-empty.md

lornajane · 2024-10-18T13:30:00Z

I can fix the docs header but I thought we were holding off on adding docs until we were happy with how they were all shaping up in #1697 . Maybe we can make the code changes here, and put the docs over there? (where I will also revisit the section headers on the Arazzo rules docs)

DmitryAnansky · 2024-10-18T13:38:57Z

I can fix the docs header but I thought we were holding off on adding docs until we were happy with how they were all shaping up in #1697 . Maybe we can make the code changes here, and put the docs over there? (where I will also revisit the section headers on the Arazzo rules docs)

Also I can merge changes just to have rule described - so we can update docs later.

lornajane

Rule works as expected and I agree it should be in our recommended set. I think we should remove the docs though since we haven't added other docs yet - could you transfer the new rule page into #1697?

DmitryAnansky · 2024-10-18T14:22:35Z

Rule works as expected and I agree it should be in our recommended set. I think we should remove the docs though since we haven't added other docs yet - could you transfer the new rule page into #1697?

Moved docs update to this PR

.changeset/dull-pigs-jump.md

lornajane

Please apply the changelog suggestion!

adamaltman

I don't like this. It seems like it could be part of the spec rule. What am I missing?

DmitryAnansky · 2024-10-21T07:32:40Z

I don't like this. It seems like it could be part of the spec rule. What am I missing?

It is by the Spec rule:

We have sourceDescriptions required in type, so we require it to be defined but can be empty array(so we need an additional rule).
And checking that it has at least one sourceDescription with the rule to follow Arazzo Spec.

Co-authored-by: Lorna Jane Mitchell <[email protected]>

packages/core/src/rules/arazzo/__tests__/sourceDescriptions-not-empty.test.ts

…t-empty.test.ts

DmitryAnansky · 2024-10-21T08:46:38Z

I don't like this. It seems like it could be part of the spec rule. What am I missing?

This can be also done as part of type function check


const Root: NodeType = {
  properties: {
    arazzo: { type: 'string' },
    info: 'Info',
    sourceDescriptions: (items) => {
      if (items.length === 0) {
        throw new Error('SourceDescriptions must not be empty');
      }
      return 'SourceDescriptions'
    },
    workflows: 'Workflows',
    components: 'Components',
  },
  required: ['arazzo', 'info', 'sourceDescriptions', 'workflows'],
  extensionsPrefix: 'x-',
};

But error report is worse than having a rule, we don't have a way for proper line highlight and won't work well in the Editor:

With the rule:

@adamaltman , please clarify what variant would you prefer.

adamaltman · 2024-10-21T12:21:02Z

@DmitryAnansky I think you're missing my point. It must be part of the spec rule. When the spec rule covers less and less of the spec we shouldn't even call it a spec rule then. Get my point?

DmitryAnansky · 2024-10-21T12:53:24Z

@DmitryAnansky I think you're missing my point. It must be part of the spec rule. When the spec rule covers less and less of the spec we shouldn't even call it a spec rule then. Get my point?

In current cli implementation of the rules - spec rule is an abstraction on checking defined custom type.
It is not extendable to apply similar logic as in the rule I introduced.
And our custom types does not have minItems.

Please correct me if I am wrong @tatomyr , @RomanHotsiy

DmitryAnansky · 2024-10-21T12:55:48Z

@DmitryAnansky I think you're missing my point. It must be part of the spec rule. When the spec rule covers less and less of the spec we shouldn't even call it a spec rule then. Get my point?

@adamaltman moreover some of existing unique rules can't be defined in our spec rule.

tatomyr · 2024-10-21T16:36:25Z

And our custom types does not have minItems.

@DmitryAnansky, yes, this is a limitation of the existing spec rule. It doesn't include this type of check out of the box. It can be extended if needed, but I noticed we have a few exceptions in the OAS spec rule as well. For instance, the no-example-value-and-externalValue rule which covers the requirement that the fields are mutually exclusive by the specification, or operation-description which must be unique.

adamaltman · 2024-10-21T21:05:13Z

@tatomyr those were honest omissions. Mostly added as separate rules in the future for backwards compatibility reasons.

We are not in that situation with Arazzo. @DmitryAnansky we should not start the day with technical debt.

tatomyr · 2024-10-22T06:12:00Z

@adamaltman thanks for the clarification!

adamaltman · 2024-10-23T17:05:51Z

Given this information, do we have enough information to move forward?

RomanHotsiy · 2024-10-24T06:40:38Z

@adamaltman I would like to discuss it with you quickly before we proceed.
If we want to make changes in this area we should consider bigger changes or we may end up with not the most optimal design.

The problem is that the spec rule is named incorrectly in my opinion. It should be named struct which means "Structural validation". It was never designed to validate the "spec" conformance.

Sure, we can wrap up all the various spec-related checks under the spec rule but it has some disadvantages:

all the spec related errors will end up with spec ruleId in the output which may limit tooling in the future (or tooling will have to rely on error message)
we loose some extra flexibility to be able to disable some aspects of spec validation (it doesn't make sense in most of cases but openapi-core can be used for various use-cases so you never know).
(minor) spec rule code will get much harder to maintain if we want to keep same performance

I think the better solution would be to:

rename the spec rule into struct rule (or something like that)
prefix all of the other spec-related rules with spec- prefix (we already have a few like that)
create a spec ruleset which includes all of the spec-related rules for each type of spec (openapi, asyncapi, arazzo)

This solution is a breaking change so I'm not suggesting doing it right now (or maybe?). I want to bring it up though so whatever we do right now is not blocking us in the future.

Let's discuss it offline and decide?

adamaltman · 2024-10-24T13:22:19Z

Yes, this is exactly what I was thinking (I like struct as I had a similar name in mind). I think when you say spec-{specname} you mean it is a config that someone would use like:

extends:
  - spec

And other configs like recommended could extend that itself too. Right?

RomanHotsiy · 2024-10-24T13:25:12Z

And other configs like recommended could extend that itself too. Right?

yes!

adamaltman · 2024-10-27T18:34:43Z

@DmitryAnansky we discussed not to rename the spec rule but rather to start a similar struct rule (for Arazzo). We would need to make any breaking changes for OpenAPI and AsyncAPI in version 2 with the struct rule.

The idea is that the struct rule is part of a ruleset that includes the struct rule and other rules such as sourceDescriptions has at least one value, IDs are unique, etc... It is misleading as a spec rule because it is only the basic structure of the spec, not actual things like there must be at least one sourceDescription and workflow IDs are unique.

DmitryAnansky · 2024-11-19T16:43:27Z

Moving code changes to #1800

DmitryAnansky force-pushed the feat/remove-type-none-support-spot branch from 1e05f85 to 2a4af85 Compare October 17, 2024 13:06

DmitryAnansky marked this pull request as ready for review October 17, 2024 16:25

DmitryAnansky requested review from a team as code owners October 17, 2024 16:25

adamaltman reviewed Oct 17, 2024

View reviewed changes

docs/rules/arazzo/sourceDescriptions-empty.md Outdated Show resolved Hide resolved

adamaltman reviewed Oct 17, 2024

View reviewed changes

docs/rules/arazzo/sourceDescriptions-empty.md Outdated Show resolved Hide resolved

adamaltman reviewed Oct 17, 2024

View reviewed changes

docs/rules/arazzo/sourceDescriptions-empty.md Outdated Show resolved Hide resolved

adamaltman reviewed Oct 17, 2024

View reviewed changes

docs/rules/arazzo/sourceDescriptions-empty.md Outdated Show resolved Hide resolved

DmitryAnansky requested review from adamaltman and lornajane October 18, 2024 08:06

DmitryAnansky added 5 commits October 18, 2024 12:25

feat: removed the type:none sourceDescriptions Arazzo extension

38d9d69

chore: prettier

17c5d19

feat: add sourceDescription empty arazzo rule

31029bb

chore: remove broken link

00bad20

chore: docs update

4eb39ad

DmitryAnansky force-pushed the feat/remove-type-none-support-spot branch from c271f47 to 4eb39ad Compare October 18, 2024 09:25

feat: add different sourceDescription rule

3f5d83a

lornajane reviewed Oct 18, 2024

View reviewed changes

chore: remove rule description from the docs

a00a856

DmitryAnansky requested review from lornajane and tatomyr October 18, 2024 14:22

lornajane reviewed Oct 18, 2024

View reviewed changes

.changeset/dull-pigs-jump.md Outdated Show resolved Hide resolved

lornajane approved these changes Oct 18, 2024

View reviewed changes

adamaltman requested changes Oct 18, 2024

View reviewed changes

Update .changeset/dull-pigs-jump.md

d4cc844

Co-authored-by: Lorna Jane Mitchell <[email protected]>

DmitryAnansky requested a review from adamaltman October 21, 2024 07:33

DmitryAnansky commented Oct 21, 2024

View reviewed changes

packages/core/src/rules/arazzo/__tests__/sourceDescriptions-not-empty.test.ts Outdated Show resolved Hide resolved

Update packages/core/src/rules/arazzo/__tests__/sourceDescriptions-no…

9affd5a

…t-empty.test.ts

DmitryAnansky closed this Nov 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: removed the `type:none` sourceDescriptions Arazzo extension #1773

feat: removed the `type:none` sourceDescriptions Arazzo extension #1773

DmitryAnansky commented Oct 17, 2024 •

edited

Loading

changeset-bot bot commented Oct 17, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading

lornajane commented Oct 18, 2024

DmitryAnansky commented Oct 18, 2024

lornajane left a comment

DmitryAnansky commented Oct 18, 2024

lornajane left a comment

adamaltman left a comment

DmitryAnansky commented Oct 21, 2024 •

edited

Loading

DmitryAnansky commented Oct 21, 2024 •

edited

Loading

adamaltman commented Oct 21, 2024

DmitryAnansky commented Oct 21, 2024

DmitryAnansky commented Oct 21, 2024

tatomyr commented Oct 21, 2024

adamaltman commented Oct 21, 2024

tatomyr commented Oct 22, 2024

adamaltman commented Oct 23, 2024

RomanHotsiy commented Oct 24, 2024

adamaltman commented Oct 24, 2024

RomanHotsiy commented Oct 24, 2024

adamaltman commented Oct 27, 2024

DmitryAnansky commented Nov 19, 2024 •

edited

Loading

feat: removed the type:none sourceDescriptions Arazzo extension #1773

feat: removed the type:none sourceDescriptions Arazzo extension #1773

Conversation

DmitryAnansky commented Oct 17, 2024 • edited Loading

What/Why/How?

Reference

Testing

Screenshots (optional)

Check yourself

Security

changeset-bot bot commented Oct 17, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Oct 17, 2024 • edited Loading

github-actions bot commented Oct 17, 2024 • edited Loading

Coverage report

Test suite run success

lornajane commented Oct 18, 2024

DmitryAnansky commented Oct 18, 2024

lornajane left a comment

Choose a reason for hiding this comment

DmitryAnansky commented Oct 18, 2024

lornajane left a comment

Choose a reason for hiding this comment

adamaltman left a comment

Choose a reason for hiding this comment

DmitryAnansky commented Oct 21, 2024 • edited Loading

DmitryAnansky commented Oct 21, 2024 • edited Loading

adamaltman commented Oct 21, 2024

DmitryAnansky commented Oct 21, 2024

DmitryAnansky commented Oct 21, 2024

tatomyr commented Oct 21, 2024

adamaltman commented Oct 21, 2024

tatomyr commented Oct 22, 2024

adamaltman commented Oct 23, 2024

RomanHotsiy commented Oct 24, 2024

adamaltman commented Oct 24, 2024

RomanHotsiy commented Oct 24, 2024

adamaltman commented Oct 27, 2024

DmitryAnansky commented Nov 19, 2024 • edited Loading

feat: removed the `type:none` sourceDescriptions Arazzo extension #1773

feat: removed the `type:none` sourceDescriptions Arazzo extension #1773

DmitryAnansky commented Oct 17, 2024 •

edited

Loading

changeset-bot bot commented Oct 17, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading

DmitryAnansky commented Oct 21, 2024 •

edited

Loading

DmitryAnansky commented Oct 21, 2024 •

edited

Loading

DmitryAnansky commented Nov 19, 2024 •

edited

Loading