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

Inconsistencies for schemas in server.variables and channel.parameters #583

Closed
magicmatatjahu opened this issue Jul 13, 2021 · 17 comments
Closed
Labels
❔ Question A question about the spec or processes released on @next-major-spec stale

Comments

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Jul 13, 2021

I'm wondering why schema (doc/constraints) for server variables uses only a subset of JSON Schema (enum, default, examples and description) rather a full set of JSON Schema keywords like channel.parameters.[*].schema. I can understand that if you have server's url/host then you should have in 99% cases "hardcoded" variables for it - variable's type can be only a string and with some set of values (enum), but maybe we should allow also more complex schema like pattern etc? If channel's parameter has it (which is more generic), so why not server variable? I need some clarification in this place.

Another question/problem: if server variable can have defined enum then for what the examples exist? I mean if there is a case, when given variable have a predefined set of values (enum) then examples === enum, so examples is unnecessary. Other case, when examples field is used to describe possible "other" values for variable, so maybe then we should allow more complex schema to describe the variable (see my first question)?

I hope that you understand my concerns and thanks very much for comments!

@magicmatatjahu magicmatatjahu added the ❔ Question A question about the spec or processes label Jul 13, 2021
@fmvilas
Copy link
Member

fmvilas commented Jul 13, 2021

The reason it was defined as a small subset of JSON Schema was to remove complexity in an area that would probably not need much. I agree pattern could be added. I'd still not go for a full JSON Schema object because that would imply support for oneOf, anyOf, allOf, not, if, then, else, etc. And to be honest, I haven't seen a use case needing such complex schemas yet.

if server variable can have defined enum then for what the examples exist?

Because enum is optional and your variable may be a free-form string. Therefore, you may need to provide examples.

If channel's parameter has it (which is more generic), so why not server variable?

Now I'm wondering if we really need a full JSON Schema object on parameters 😄

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Jul 13, 2021

Because enum is optional and your variable may be a free-form string. Therefore, you may need to provide examples.

And for that we should enable the all JSON Schema keywords for string type like minLength, maxLength, pattern etc. and don't base "validation" on examples. Also, If we really go with current solution as subset of JSON Schema, I opt for enable also the integer and number type to describe much easier the variables which has form of numbers - for that someone can use the minimum, maximum etc keywords.

Now I'm wondering if we really need a full JSON Schema object on parameters 😄

At the moment you must define if it's a string, number or integer, another types are allowed but invalid in this case, but for me it covers 100% cases, so I wouldn't change it. Maybe we should go with this same approach for server variables. I understand that you afraid if someone will use the complex anyOf, oneOf keywords, but I would rather give this option to someone than prevent him, due to this the specification would have missing part or would be described in the description field.

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Jul 27, 2021

So we have options:

  • leave it as it is
  • change the channel parameter's schema field to shape as in the server variable, adding additional constraints for string type like pattern, minLength, maxLength, format - add also these constraints in the Server Variable Object
  • change the type of server variable to Schema Object, not as currently Server Variable Object

I prefer 3rd option, because it covers all cases, also user have possibility to define and use constraints for the number, integer types. So I suggest to change the shape of the server variable to similar object like in parameter without location:

variables: 
  {variable_name}:
    description: string
    schema: ...

EDIT: If we go in 2 or 3 option, it's a breaking change for 3.0.0 spec

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Jan 14, 2022
@derberg derberg removed the stale label Jan 14, 2022
@magicmatatjahu
Copy link
Member Author

It should be handled in the 3.0.0.

@jonaslagoni
Copy link
Sponsor Member

@magicmatatjahu do you want to champion this? 🙂 Or can we consider this issue as needs champion? 🤔

@magicmatatjahu
Copy link
Member Author

@jonaslagoni I can be a champion :)

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label May 22, 2022
@magicmatatjahu
Copy link
Member Author

Still relevant.

@github-actions github-actions bot removed the stale label May 24, 2022
@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Feb 16, 2023

As we discussed that in the asyncapi/community#587 meeting I think that we should make consistent Server Variable Object and Channel Parameter Object with fields copied from JSON Schema but only related to the string, integer and number types (we can also reuse type: boolean, but question if we should? Is there any use case to define variable/parameter as boolean, except serialization?), so it will be:

title
type
description
multipleOf
maximum
exclusiveMaximum
minimum
exclusiveMinimum
maxLength
minLength
pattern
format
default
enum
const
examples

and Parameter Object should has also location property. Also I wonder if we should add "meta" keywords to the object like:

readOnly
writeOnly
contentMediaType
contentEncoding
contentSchema

and maybe also add our ExternalDocumentation Object?

wdyt? cc @fmvilas @derberg @jonaslagoni @dalelane @char0n

@fmvilas
Copy link
Member

fmvilas commented Feb 16, 2023

TBH, I would even keep it simpler. I'd make it like the Server Variable Object right now:

enum
default
description
examples

And add location in the case of Channel Parameters.

Not even interested in type. Most probably this is going to be a string after all so I would rather make it this simple and start improving if we get suggestions from users.

@derberg
Copy link
Member

derberg commented Feb 27, 2023

I agree with the idea of starting with minimum, as adding more keywords is an easy contribution. Also Server Variable Object has been there for a very long and nobody asked for more keywords.

So yeah, definitely just do in Channel Parameters what is in Server Variables

@magicmatatjahu
Copy link
Member Author

@derberg @fmvilas So should we handle that - check that comment #880 (comment), there isn't listed that issue?

Copy link
Member

derberg commented Mar 8, 2023

yeah, I think so, especially that it is also related to changes in defining schemas. And yeah, it is a breaking change. We completely forgot about it when had a call, but we do not make decisions on a call anyway. So if you are willing to champion please make it clear under #880 🙏

@asyncapi-bot
Copy link
Contributor

🎉 This issue has been resolved in version 3.0.0-next-major-spec.11 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Sep 27, 2023
@smoya smoya closed this as completed Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❔ Question A question about the spec or processes released on @next-major-spec stale
Projects
None yet
Development

No branches or pull requests

6 participants