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

Maybe we should add some regex patterns for server host and pathname #433

Closed
derberg opened this issue Oct 10, 2023 · 5 comments
Closed
Labels
enhancement New feature or request

Comments

@derberg
Copy link
Member

derberg commented Oct 10, 2023

in spec v3 we have new fields in Server Object:

  • host
  • pathname

in json schema we do not have any basic validation: https://github.com/asyncapi/spec-json-schemas/blob/next-major-spec/definitions/3.0.0/server.json#L15-L22

I think we should have at least some basic, that will fail if someone uses host the same way as it was with url, so localhost:3000/ws would be an error

JSON schema format keywords, might not be good, as hostname I think should be without port for example. I'm not 100% sure what formats from https://json-schema.org/understanding-json-schema/reference/string#built-in-formats we can reuse, thus suggesting patterns

Also in specification we are not so detailed and accurate, not pointing to standards like JSON Schema

cc @fmvilas that introduced a change in spec
cc @jonaslagoni release coordinator
cc @magicmatatjahu who I think introduced changes in json schemas
cc codeowners of the json schemas @dalelane @char0n @smoya

@derberg derberg added the enhancement New feature or request label Oct 10, 2023
@smoya
Copy link
Member

smoya commented Oct 10, 2023

JSON schema format keywords, might not be good, as hostname I think should be without port for example

Yeah, it doesn't contemplate ports. Only the first human-readable section. So 👍 to use a pattern.

Copy link
Member Author

derberg commented Oct 11, 2023

for host it should be easy, just pattern that complains when / is added. With pathname? patter that just complains if it do not start with/?

@dalelane
Copy link
Collaborator

yeah, makes sense - I suspect that might help catch some instances of people mistakenly cramming everything into host without realising

@jonaslagoni
Copy link
Member

Duplicate of #336

@fmvilas fmvilas closed this as completed Dec 7, 2023
@fmvilas
Copy link
Member

fmvilas commented Dec 7, 2023

Closing in favor of #336.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants