Open jandppw opened 1 year ago
Hi @jandppw , I have not looked into this before as normally we seldom define a schema allowing free-form map. Sometimes a simple joi.object() might be enough. But if it does have such requirement to restrict the key/value type, then I think we do need to enhance the additionalProperties as schema instead of boolean.
@kenspirit Thx for the response.
Ok, I'll try to work on this over the next weeks.
First let's agree on the Joi schema to use.
Since the only thing that OpenAPI seems to support at the moment is a type: object
with the model of the properties as value of additionalProperties
, and there seems to be no way currently in OpenAPI to says something in a structured way about the allowed keys, I suggest using a Joi.object().pattern()
schema, where we ignore the first argument (users can add a description to the Joi.object()
and describe the keys that are allowed in text). Since we are ignoring the first argument, it can both be a RegExp or a Joi schema. If there are other properties in the Joi.object()
, they would be listed in OpenAPI under the properties
heading.
The OpenAPI expresses that additionalProperties
must have a value according to the mentioned model, if I'm correct. Thus, apart from the properties
explicitly listed, and the additionalProperties
, no other properties are possible in according to the OpenAPI model. According to the Joi schema, it is possible to have a 3rd kind of property. I.e., properties that are not mentioned explicitly, and do not match the pattern()
RegExp or property name schema. In Joi, you can control this with unknown()
. The only variant that can be expressed in OpenAPI is unknown(false)
: there can be no properties of the 3rd kind.
There are different ways we can deal with this. I don't think it is a good idea to forbid unknown(true)
in this case, with an error or something, because joi-to-json
does not forbid anything else. Alternatively, we can ignore unknown()
in this case completely. A third option is to let unknown(true)
override the pattern()
. In OpenAPI, effectively, since we cannot discern between property names that adhere to the property name RegExp or schema, this comes down to “everything is allowed”.
What do you think? Which option do you think is most appropriate? Or do you know of more appropriate Joi schemata to be translated to OpenAPI dictionaries?
For OpenAPI (https://swagger.io/specification/):
additionalProperties - Value can be boolean or object. Schema Object, not JSON schema.
For normal JSON schema (https://json-schema.org/understanding-json-schema/reference/object.html?highlight=pattern#additional-properties):
The additionalProperties keyword is used to control the handling of extra stuff, that is, properties whose names are not listed in the properties keyword or match any of the regular expressions in the patternProperties keyword. By default any additional properties are allowed.
The value of the additionalProperties keyword is a schema that will be used to validate any properties in the instance that are not matched by properties or patternProperties. Setting the additionalProperties schema to false means no additional properties will be allowed.
Now we are talking about if we want the OpenAPI to have both properties
and additionalProperties
, how to define the JOI schema.
From my perspective:
unknown
setting should still be able to use as OpenAPI does allow boolean value for additionalProperties
.unknown(false)
and pattern
, I think we can take the pattern and see how to convert it to a Schema object for additionalProperties
.unknown(true)
and pattern
, this falls into the case Joi schema, it is possible to have a 3rd kind of property
. This inconsistency between Joi and JSON can only be hardcoded/set by us to see which is more preferred?I am inclined to take pattern
higher priority which means ignoring the unknown
. Because if ignoring pattern
, why define it in the first place? Or at most we can make use of the meta
API in JOI, and use a special instruction to indicate other options?
Agreed on ignoring unknown
for the OpenAPI model when the object has a pattern
.
I'm not an option-kind-of-guy. YAGNI. I like to get something working for business cases I know, and if somebody complains, extra business cases can be considered.
I do think we still need to consider JSON schema patternProperties
for a moment though. I have not worked much with JSON schema, so no experience with that here. Is this supported in the library at this time? Do we need to take into account that this shouldn't break? Or are there good reasons why we need to take it into account with this planned change? Or is it possible to just bypass the issue?
I do not want to add an extra option either, at least not before a real case is raised.
For OpenAPI, it's not supported. For non-OpenAPI JSON schema version, it's supported already by converting the pattern
case to patternProperties
as it is more fit to Joi's pattern
scenario and it also allows us to have the "3rd option properties" as you mentioned.
Clear.
I suggest setting up the cases for this in the tests first, in a branch, which will then fail.
I'm a bit scared of diving into the code to realize this. My earlier contributions where scalpel surgery, and I do not understand the overal code base that well. Might need some help.
That is alright. Everyone has gone through that. You might just clone the repo and try locally. After enhancement & verification by test, PR can be submitted.
Test cases can be collected and raised here if some discussion is required. Not sure if you have any real business case already.
Checking in. Still intending to do this, but it has been a busy year. Sorry.
That is alright and thanks for paying attention and keep improving this lib all the time.
I think the current code base with clearer fixture separation allows us to define what kind of dictionary support we need. Now in OpenAPI 3.1, its dictionary definition is also just using additionalProperties
.
https://spec.openapis.org/oas/v3.1.0#model-with-map-dictionary-properties
Maybe we can work backward by taking a valid JSON schema business use case and see what a reasonable JOI definition should be
I cannot find a way to define a dictionary ("Dictionaries, HashMaps and Associative Arrays", OpenAPI 3.0.3) in OpenAPI.
I would expect this to work with
Joi.object().pattern()
, butNote that
patternProperties
, part of JSON Schema, does not seem to be supported by even the latest version of OpenAPI. For OpenAPI, it seems that we have to output the property value schema underadditionalProperties
, instead of a Boolean.Willing to help fix this, but first I'd like to check whether I'm missing something.