spec icon indicating copy to clipboard operation
spec copied to clipboard

Inconsistencies for schemas in server.variables and channel.parameters

Open magicmatatjahu opened this issue 4 years ago • 10 comments

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 avatar Jul 13 '21 10:07 magicmatatjahu

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 😄

fmvilas avatar Jul 13 '21 14:07 fmvilas

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 avatar Jul 13 '21 14:07 magicmatatjahu

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

magicmatatjahu avatar Jul 27 '21 09:07 magicmatatjahu

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

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 :heart:

github-actions[bot] avatar Jan 14 '22 01:01 github-actions[bot]

It should be handled in the 3.0.0.

magicmatatjahu avatar Jan 14 '22 10:01 magicmatatjahu

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

jonaslagoni avatar Jan 19 '22 18:01 jonaslagoni

@jonaslagoni I can be a champion :)

magicmatatjahu avatar Jan 20 '22 10:01 magicmatatjahu

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

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 :heart:

github-actions[bot] avatar May 22 '22 00:05 github-actions[bot]

Still relevant.

magicmatatjahu avatar May 22 '22 20:05 magicmatatjahu

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

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 :heart:

github-actions[bot] avatar Sep 21 '22 00:09 github-actions[bot]

As we discussed that in the https://github.com/asyncapi/community/issues/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

magicmatatjahu avatar Feb 16 '23 09:02 magicmatatjahu

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.

fmvilas avatar Feb 16 '23 15:02 fmvilas

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

derberg avatar Feb 27 '23 19:02 derberg

@derberg @fmvilas So should we handle that - check that comment https://github.com/asyncapi/spec/issues/880#issuecomment-1451715265, there isn't listed that issue?

magicmatatjahu avatar Mar 06 '23 06:03 magicmatatjahu

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 🙏

derberg avatar Mar 08 '23 12:03 derberg

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

The release is available on GitHub release

Your semantic-release bot :package::rocket:

asyncapi-bot avatar May 29 '23 06:05 asyncapi-bot

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

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 :heart:

github-actions[bot] avatar Sep 27 '23 00:09 github-actions[bot]