Documentation of schema with allOf changes based on presence of an attribute in one of the included schemas

jpjpjp commented 3 months ago

This may be user error, but I'm struggling to get the desired documentation for a schema object that uses the allOf attribute to include a previously defined schema and then add some additional properties to it.

The documentation of a schema defined like this:

publicBaseGroupObject:
      description: >
        The object is a superset of the baseObject.  It includes everything
        that the baseObject has, but also two additional parameters.
        We want the documentation for this object to show this description as 
        well as expanding to show the doc strings for the attributes in the 
        baseObject and extra attributes added here.
      allOf:
        - $ref: '#/components/schemas/baseObject'
        - type: object
          properties:
            children:
              type: array
              nullable: true
              description: An array that exists only in the baseGroupObject.
              items:
                type: number
            hydrated_children:
              type: array
              nullable: true
              description: Another array that exists only in the baseGroupObject this 
                is populated by the full baseObjects that are children of a 
                baseObject
              items:
                $ref: '#/components/schemas/baseObject'

changes radically if the referenced baseObject contains one of the additional children or hydrated_children attributes. It's understandable that if the baseObject contained those attributes, it would cause a collision or an issue, but the desired documentation happens only if one of those attributes exists.

This is easier explained via an example: https://elements-demo.stoplight.io/?spec=https://gist.githubusercontent.com/jpjpjp/897141d93c4e9196cfe783ff88f275b0/raw/56420f879807f0c3c5631427c5046c90d9497c55/replicate_doc_issue.yaml#/

Context

We are unable to provide correct documentation for schemas that are a superset of other schemas without completely redefining the schema, which can lead to inconsistencies.

Current Behavior

It is best seen in the example provided above, but if baseObject does not also contain one of the additional properties defined in the publicBaseGroupObject, the documentation for the publicBaseObject shows only the baseObject. An example is here: https://elements-demo.stoplight.io/?spec=https://gist.githubusercontent.com/jpjpjp/897141d93c4e9196cfe783ff88f275b0/raw/56420f879807f0c3c5631427c5046c90d9497c55/replicate_doc_issue.yaml#/schemas/publicBaseGroupObjectWithoutHydratedChildrenInBase

Expected Behavior

The desired behavior, I think, would be that the documentation would provide the description for the allOf attribute and then provide all the documentation for everything within the allOf. An example of this working as described is here: https://elements-demo.stoplight.io/?spec=https://gist.githubusercontent.com/jpjpjp/897141d93c4e9196cfe783ff88f275b0/raw/56420f879807f0c3c5631427c5046c90d9497c55/replicate_doc_issue.yaml#/schemas/publicBaseGroupObject

Note however, that this only works if the hydrated_children attribute is defined in both the baseObject and the additional properties described in the publicBaseGroupObject.

Possible Workaround/Solution

The only workaround I can think of is to avoid using the allOf tag and redefining all the attributes in the baseObject again in the publicBaseGroupObject.

Steps to Reproduce

YAML to replicate the problem: https://gist.githubusercontent.com/jpjpjp/897141d93c4e9196cfe783ff88f275b0/raw/56420f879807f0c3c5631427c5046c90d9497c55/replicate_doc_issue.yaml

Example of problem in the demo environment: https://elements-demo.stoplight.io/?spec=https://gist.githubusercontent.com/jpjpjp/897141d93c4e9196cfe783ff88f275b0/raw/56420f879807f0c3c5631427c5046c90d9497c55/replicate_doc_issue.yaml#/

Environment

Version used:
- Stoplight Elements Demo Environment running on Chrome Version 125.0.6422.77
- Running locally on a Node express server with Node v21.7.2
Operating System and version (desktop or mobile):
- Mac OS Sonoma
Link to your environment/workspace/project: https://gist.github.com/jpjpjp/897141d93c4e9196cfe783ff88f275b0

brendarearden commented 3 months ago

Thank you for providing all the details!

Looking at your spec, we believe that changing additionalProperties: false in your baseObject to additionalProperties: true would resolve the issue you are seeing. Would you try this and let us know?

weyert commented 3 months ago

Yeah, that will fix it. I am having the same issue only I am not sure what the expected behaviour of allOf is regarding merging two schemas. Should it ignore additionalProperties?

jpjpjp commented 3 months ago

Hi @brendarearden and @weyert. Sorry for the late response. Indeed it does fix the problem but doesn't give me the desired result which is a new object that combines the properties of the baseObject along with the new properties in the publicBaseGroupObject AND applies the additionalProperties:false property the new combined set of properties.

I realize this isn't necessarily an elements issue, but does OpenAPI support something like:

# In components/schemas...
baseObject:
  type: object
  properties:
    basePropertyOne:
      type: string
    basePropertyTwo:
      type: int

superObject:
      description: >
        The object is a superset of the baseObject.  It includes everything
        that the baseObject has, but also additional parameters.
        We want the documentation for this object to show this description as 
        well as expanding to show the doc strings for the attributes in the 
        baseObject and extra attributes added here.
        We also want the additionalProperties attribute to apply allow all
        the properties defined here but cause errors in validation if others
        exist
     additionalProperties: false
      allOf:
        - $ref: '#/components/schemas/baseObject'
        - type: object
          properties:
            superPropertyOne:
              type: string

Such that a superObject that looks like this:

{
  "basePropertyOne": "foo",
  "basePropertyTwo": 2,
  "superPropertyOne": "bar"
}

Is valid, but one that looks like this:

{
  "basePropertyOne": "foo",
  "basePropertyTwo": 2,
  "superPropertyOne": "bar",
  "extraProperty": "baz"
}

Fails validation because of it is an additionalProperty at the "superObject" level?

What I've found is that an additionalProperties: false setting seems to apply only to the baseObject and actually causes validators to complain about "superPropertyOne" (and also the generated docs don't look good).

weyert commented 3 months ago

Yes,I tried to read the JSON schema specification about how allOf merge behaviour should be when additionalProperties is used. Only I can't figure it out. If you look at the behaviour of Redocly it appears to somehow ignore it?

They do have a page describing allOf here: https://redocly.com/docs/resources/all-of/

I do wonder if the validation should differ from what is shown in schema viewer in Elements?

I have the feeling that Redocly always merges the schemas and ignores additionalProperties-setting when displaying the schemas. A similar functionality can be achieved by setting the ignoreAdditionalProperties in @stoplight/json-schema-merge-allof

ignoreAdditionalProperties default false

Allows you to combine schema properties even though some schemas have additionalProperties: false This is the most common issue people face when trying to expand schemas using allOf and a limitation of the json schema spec. Be aware though that the schema produced will allow more than the original schema. But this is useful if just want to combine schemas using allOf as if additionalProperties wasn't false during the merge process. The resulting schema will still get additionalProperties set to false.

jpjpjp commented 3 months ago

Thanks @weyert! The "ignoreAdditionalProperties" in stoplight seems like would address the documentation issue, but given my desire to validate the responses in tests I ended up replicating all the shared parameters across my two schema objects.

(Now the spec no longer follows DRY - don't repeat yourself - standards but the generated docs and tests work as I want)

I hope to follow up at some point with the swagger folks to see if there is a better resolution to this issue.

weyert commented 3 months ago

@jpjpjp My apologies, what do you mean with you want to validate the responses in tests?

jpjpjp commented 3 months ago

Ah sorry. As I said, this isn't a stoplight issue per day but just general "Un clarity" on the combination of "allOf" and "additionalProperties"

Not only does it screw up stoplight elements doc generation it also seems to mess up tools that do validation to ensure that response bodies match the specified schema.

In an ideal world (to me) the schema that results when allOf references an existing schema along with additional properties would be exactly the same as if you just listed all of the properties of the referenced schema and the new properties by hand. Any "additionalProperties" attribute would apply to the sum of this properties.

From a doc standpoint it would look like one schema and validation tools should treat it that way too.

Sorry if that was confusing!

brendarearden commented 3 months ago

@frankkilcommins could you take a look at this discussion?

weyert commented 3 months ago

@jpjpjp Makes sense. Currently, I am patching that package to set ignoreAdditionalProperties to solve my problem in Elements. As we only use it to display documentation so it should be fine for us. I think the problem is actually in @stoplight/json-schema-viewer than in this component.

I hope @frankkilcommins can shed some light on what the expected behaviour is of allOf and ignoreProperties. Are we using it wrong? How can fix the rendering of the schema in Elements in the correct manner?

stoplightio / elements