Skip to content

Conversation

@jeluard
Copy link
Contributor

@jeluard jeluard commented Jan 11, 2024

A number of Schema and Response $refs embed description and example in a way that is not supported by the openapi spec (relying on an incorrect allOf indirection).

Starting with OpenAPI 3.1 description can be overridden at the $ref level.
example can be defined at the Response level, fixing most issues. Some others were duplicates or irrelevant.

Note that some components were already leveraging this syntax, so this PR merely aligns on a single syntax. This also reduces the validation and error messages complexity.

I also validated that example still show up in the swagger UI after this change.

requires #409

Copy link
Member

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

To make this easier to review and merge I think it should be split into three separate PRs

  • update openapi version (this might require separate discussion)
  • remove unneeded allOf usages
  • correct example definitions / indentation fixes

Removing example values should only be done if those are set incorrectly, otherwise I would also be in favor of keeping them.

@jeluard
Copy link
Contributor Author

jeluard commented Jan 12, 2024

@nflaig @dapplion This PR now depends on #409 for the openapi upgrade.
I removed all irrelevant example usages.

So we are left with what is required to remove the incorrect allOf usages:

  • rely on the new description override feature
  • pull up example in Responses

Note that some allOf were already useless.

description: "The genesis_time configured for the beacon node, which is the unix time in seconds at which the Eth2.0 chain began."
example: "1590832934"
Copy link
Member

Choose a reason for hiding this comment

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

Should make sure to use same order of properties everywhere, or how did you determine if description vs example should be 2nd property? Looking at ExecutionOptimistic below, the original order was swapped

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no consistent ordering in general. But that's probably for a different PR.
That said, I would be in favor of the following:

  • type or $ref
  • description
  • example

Copy link
Member

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

lgtm, just a few nits regarding consistent ordering of properties.

This fixes error examples when working on spec in dev mode (without bundling step)

Comment on lines +75 to +77
description: 'A bit is set if a signature from the validator at the corresponding index in the subcommittee is present in the aggregate `signature`.'
$ref: "../primitive.yaml#/Bitvector"
example: "0xffffffffffffffffffffffffffffffff"
Copy link
Member

Choose a reason for hiding this comment

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

does not follow ordering discussed in #407 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is to keep the order unchanged in this PR (should be the case now) and have another one introducing a consistent order with associated CI and command to fix it.

Copy link
Member

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

Looks good to me, as discussed in #407 (comment), previous ordering is kept to keep changes as minimal as possible

@rolfyone
Copy link
Contributor

This runs ok, are we ok to merge?

@jeluard
Copy link
Contributor Author

jeluard commented Jan 16, 2024

@rolfyone fine with me!

Copy link
Member

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

LGTM

@rolfyone rolfyone merged commit 4bbb87d into ethereum:master Jan 17, 2024
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.

4 participants