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

Invalid Syntax in securityLevels #137

Open
datsirul opened this issue Jul 10, 2024 · 6 comments
Open

Invalid Syntax in securityLevels #137

datsirul opened this issue Jul 10, 2024 · 6 comments

Comments

@datsirul
Copy link

The file beaconConfigurationSchema.yaml contains invalid OpenAPI syntax in it's default value.

Existing Syntax:

securityLevels:
    description: All access levels supported by the Beacon. Any combination is
      valid, as every option would apply to different parts of the Beacon.
    type: array
    items:
      enum:
        - PUBLIC
        - REGISTERED
        - CONTROLLED
      default:
        - CONTROLLED

Corrected Syntax:
The default value should be defined at the same level as the other properties under securityLevels:

securityLevels:
  description: All access levels supported by the Beacon. Any combination is
    valid, as every option would apply to different parts of the Beacon.
  type: array
  items:
    type: string
    enum:
      - PUBLIC
      - REGISTERED
      - CONTROLLED
  default:
    - CONTROLLED

Reference:
OpenAPI Specification - Describing Parameters - Default Parameter Values

Note:
After fixing the .yaml files, the .json should be fixed as well by running the schema conversion script located in the bin folder.

@jrambla
Copy link
Contributor

jrambla commented Jul 10, 2024

Seems fine to me. PR, please

@mbaudis
Copy link
Member

mbaudis commented Jul 11, 2024

+1

@redmitry
Copy link
Collaborator

redmitry commented Jul 15, 2024

I am confused...
beaconConfigurationSchema.yaml is a Json Schema and has nothing with OpenAPI.
Although the "default" is just an annotation that doesn't interfere the validation, it's recommended to be valid against the schema. Unlike its Json counterpart:
beaconConfigurationSchema.json, yaml defaults to String and not an array of Strings, so IMHO this could be the root of the problem with the OpenAPI's tool.

I would add the "uniqueItems": true as it is wrong to have ["PUBLIC", "PUBLIC"].

    type: array
    items:
      enum:
        - PUBLIC
        - REGISTERED
        - CONTROLLED
    default:
        - [CONTROLLED]
    uniqueItems: true

CORRECTED!
Yeah! Of course default: [] and uniqueItems should be on the array level...

@mbaudis
Copy link
Member

mbaudis commented Jul 15, 2024

  • the default parameter is at the root level of the object as @datsirul pointed out (it is not a default for the items...)
  • it is correct in the JSON version which emphasizes the correct way to do the code editing as @datsirul always adds to his issues (+1):

Note: After fixing the .yaml files, the .json should be fixed as well by running the schema conversion script located in the bin folder.

    type: array
    default:
      - CONTROLLED
    items:
      enum:
        - PUBLIC
        - REGISTERED
        - CONTROLLED

@datsirul
Copy link
Author

I've reviewed the syntax again and it seem that these two options are valid and similar:

        default:
          - CONTROLLED
        default: [CONTROLLED]

And after running the beacon yamler, both of them outputted the expected json default array with one value.

When using this syntax - - CONTROLLED it generated an array of an array: "default": [["CONTROLLED"]].
I'll create a PR with - CONTROLLED to keep a cohesive structure with the rest of the code.

datsirul added a commit to datsirul/beacon-v2 that referenced this issue Jul 16, 2024
@mbaudis
Copy link
Member

mbaudis commented Jul 16, 2024

@datsirul Oh well, yes. I got confused by @redmitry ╮(╯▽╰)╭ because this was the issue in another discussion (the lists of lists in examples where this would be the case). Fixed my comment above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants