Need an option to expand "allOf" blocks for proper semantical difference about oasdiff HOT 33 CLOSED

@wtrocki , how do you plan to flatten oneOf/anyOf? Is it really possible?

allOf can be flattened to get fully equivalent schema, but oneOf/anyOf cannot...

from oasdiff.

This project can potentially be used to implement this feature: https://github.com/mokkabonna/json-schema-merge-allof

from oasdiff.

@reuvenharrison , understood. Unfortunately I have almost zero experience with Golang and NodeJS. I use my own fork of https://github.com/equinor/openapi-flattener to dereference original OpenAPI and remove "allOf" before feeding it to oasdiff.
I will need some time to polish my forked version (significantly improved original one), then I will publish the link here.

from oasdiff.

We're making progress on this issue and want to test our code with real use cases. Could any of you provide a spec that uses allOf and specify the library used for flattening? Your input would be really helpful. Thanks.

As I posted earlier in this chat I used a combination of https://github.com/mokkabonna/json-schema-merge-allof and https://github.com/equinor/openapi-flattener. First - as is, second needed to change to support 'allOf' constructs in different parts of OpenAPI spec (e.g. callbacks, parameters) which was not supported originally.

from oasdiff.

@reuvenharrison I have investigated the issue and there are a couple of ways to do it:

1. Preprocessing step that does flattening as mentioned by @anikitin
2. Write dedicated optional flattening for oneOf and allOf in oasDiff controlled by cli flags

IMHO option nr2 is superior as it would enable oasdiff to test the whole experience end to end and
minimize issues like duplicate reports when schema.component is reused across different apis.
Transformation logic should cater to 3 different constructs:

Approach for flattening/normalization/transformation

When performing flattering 2 different strategies can be taken:

1. Whole schema flattening involving resolving every dependency
2. Targetted flattening like mentioned in this jira

Full Schema Flatten

. This approach will fix some of the confusion with Cardiff reporting multiple breaking changes for a single model change. It will focus on the final API. There are some existing examples of golang libraries that perform such mechanisms:

• https://goswagger.io/usage/flatten.html (https://github.com/go-openapi/analysis/blob/master/flatten.go)

Flattenining strategies

Involves triggering dedicated transformations:

• AllOf (https://github.com/tcdsv/kin-openapi/tree/merge-allof)
• OneOf
• AnyOf

from oasdiff.

Looks reasonable, seems like it is not difficult to implement. It requires only to change the scheme diffs calculation to merge recursively properties of all allOf instances before diff calculation if the option is specified in Config. I suppose I could try implement it if nobody else does it in a week.

from oasdiff.

Any progress? I am trying to work around by preprocessing the spec before applying oasdiff.
Resolving allOf also requires full or partial dereferencing. Haven't built a solution which fully satisfies me so far...

from oasdiff.

Hi @anikitin
If you use refs instead of inline schemas under the allOf tag, oasdiff will detect changes properly, perhaps this could provide a workaround.

To support inline schemas we would need to merge the schemas before the diff which could be quite complicated. If we reduce the scope to merge only certain kinds of schemas it can be easier. Is your example of objects and properties comprehensive or do you expect to merge other kinds of schemas under allOf?

from oasdiff.

Hi @anikitin please let me know if you have any updates.

from oasdiff.

Closing due to inactivity.

from oasdiff.

Sorry, didn't notice your previous comment.

If you use refs instead of inline schemas under the allOf tag, oasdiff will detect changes properly, perhaps this could provide a workaround.

Good to know and it will help in some cases but not solve the problem globally.
We have hundreds of specs written by different people and most of the valid OpenAPI constructs are just allowed, so in some cases it is more convenient to use inline, we don't prevent it today.

To support inline schemas we would need to merge the schemas before the diff which could be quite complicated. If we reduce the scope to merge only certain kinds of schemas it can be easier. Is your example of objects and properties comprehensive or do you expect to merge other kinds of schemas under allOf?

I think we are mostly interested in resolving request/response schemas for path methods, potentially callbacks as well (stretch goal). I don't remember where else we can use allOf practically...

from oasdiff.

Continuing the topic: if you can support it for paths/{path}/{method}/requestBody/schema and paths/{path}/{method}/responses/{code}/{contentType}/schema it would help in 99% of cases

@reuvenharrison

from oasdiff.

i have the same issue regarding oneOf tag. no resolving. semantically equivalent schemas inside that tag are recognised as different and semantically not equivalent schemas with same schema name are not recognised as different.

from oasdiff.

@floty could you please submit two specs and the command-line to replicate this?

from oasdiff.

i have the same issue regarding oneOf tag. no resolving. semantically equivalent schemas inside that tag are recognised as different and semantically not equivalent schemas with same schema name are not recognised as different.

oneOf/anyOf might be more tricky. allOf can be just expanded before comparison, while it is not possible to do the same for oneOf/anyOf ... I have no ideas how it can be resolved, unless some guidelines are introduced for spec authoring which limit where these tags can appear in the spec.

from oasdiff.

Before we go ahead and invest in this "merge schemas" feature I want to emphasize two limitations:

1. Merging schemas may have unexpected results, for example, when two schemas under AllOf have the same property with conflicting values. This may cause an unexpected diff.
2. Merging schemas works only for AllOf and not for the other composites: OneOf or AnyOf.

The only robust solution for this problem is to define composite schemas using $refs instead of inline schemas. oasdiff can compare composite schemas with $refs because the $ref link is used as a unique ID that allows us to know which schemas correspond to each other across the two specs. This is impossible when schemas are defined inline.

To clarify, this is a composite schema with "referenced" schemas:

schema:
  oneOf:
  - $ref: '#/components/schemas/schema1'
  - $ref: '#/components/schemas/schema2'

And this is a composite schema with "inline" schemas:

schema:
  oneOf:
  - type: object
    properties:
      name:
        type: string
  - type: object
    properties:
      id:
        type: int

Your feedback is welcome.

from oasdiff.

@floty could you please submit two specs and the command-line to replicate this?

Here are some examples:

1. identical_specs_showing_diff_1.zip
   identical specs (one with inline, one with ref schema) but output diff with "modified: field: oneOf: added: 1, deleted: 1", expectation: no diff
2. identical_specs_showing_diff_2.zip
   identical specs (both with ref schema but different schema names) but output diff with "modified: field: oneOf: added: 1, deleted: 1", expectation: no diff
3. different_specs_no_diff.zip
   not identical specs (one field is missing) but no diff with option "-breaking-only", expectation: diff

I think resolving the schemas (convert to inline definition) before comparing can be the solution for fixing the oneOf issue.

from oasdiff.

Thanks @floty.
We could add a syntactic comparison to detect cases where the composite schema is identical (cases 1 and 2 above).

Case 3 is not breaking. If you define the deleted property as "required" then it will be breaking.

In general, as I mentioned above, the only solution for oneOf is to use $refs (with fixed names), otherwise there is no reliable way to associate the objects in the original spec and the revised spec.

I don't understand your suggestion, "convert to inline definition", perhaps you can elaborate?

from oasdiff.

We could add a syntactic comparison to detect cases where the composite schema is identical (cases 1 and 2 above).

Yes, handling cases 1 and 2 would be great.

Case 3 is not breaking. If you define the deleted property as "required" then it will be breaking.

You are completely right. I didn't pay attention to that.

In general, as I mentioned above, the only solution for oneOf is to use $refs (with fixed names), otherwise there is no reliable way to associate the objects in the original spec and the revised spec.

But that is only valid if you do not consider the order of the oneOf field? If the order is relevant you can directly compare every schema from first to last oneOf element. Than there is no problems regarding reliability I think.

I don't understand your suggestion, "convert to inline definition", perhaps you can elaborate?

What I mean: probable solution for handling the problem of case 1 and 2 can be to convert every "reference" schema into "inline" schema inside your comparing algorithm (see your post above for meaning of "referenced" and "inline" schema). With than, the comparing algorithm can only works on inline schemas and no naming conflicts can occur (like different schema names of case 2). Is it clearer now?

from oasdiff.

I think the processing of "allOf" and "anyOff/oneOff" can be controlled by two different command line options. For the latter case constraints should be documented. Also "discriminator" definition should be also taken into account when processing "anyOf/oneOf": change in discriminator is a potentially breaking change.

from oasdiff.

We have also detected false positives due to this issue. Do we have a plan to sort this? Can I help somehow?

from oasdiff.

We need to develop the ability to merge the schemas appearing under AllOf and then apply the normal schema comparison on the resulting merged schema.
As mentioned above we can take inspiration from this javascript project.
I started working on this here but then @fenollp suggested to do it as part of kin-openapi which makes sense.
I'm happy to cooperate on this if you are interested.

from oasdiff.

I started implementing this here: https://github.com/oasdiff/kin-openapi

from oasdiff.

@reuvenharrison , so how it can be used as a CLI tool once it is moved to kin-openapi? Would be still interested to try your fix but
you removed the branch...

Since the fix is controlled by a separate configuration option, it looks to be pretty safe to merge it into main branch, no?

from oasdiff.

I am working on the PR here: https://github.com/oasdiff/kin-openapi
The idea is to make it available to everyone who is using kin-openapi, a large community that will help us optimize this feature, and also to use it in oasdiff.

from oasdiff.

@reuvenharrison , I use a workaround now which flattens the spec with a separate tool before feeding it to oasdiff.
Anyway, let me know if you have any ETA on this feature. Would be great to have it as a part of oasdiff!

from oasdiff.

I started work on this but didn't make much progress yet. Help wanted :-)
Could you share the tool you use to flatten the spec for the benefit of other users?

from oasdiff.

I'm glad to share that @tcdsv has taken ownership on this and is making good progress towards a solution:
https://github.com/tcdsv/kin-openapi/tree/merge-allof

Anyone who is interested is welcome to join the effort.

from oasdiff.

We're making progress on this issue and want to test our code with real use cases. Could any of you provide a spec that uses allOf and specify the library used for flattening? Your input would be really helpful. Thanks.

from oasdiff.

@tcdsv the GitHub OpenAPI specs contains allOf:
https://github.com/github/rest-api-description/tree/main/descriptions

from oasdiff.

I agree it will not be fully equivalent.
Algo:

1. Detect the model with oneOf/anyOf field on the existing component.
2. For each child object (ref in the oneOf) move properties and allOf refs to the parent
3. Remove children components from OpenAPI (replace all references with parent)
4. Skip the process if there are duplicates/inconsistencies.

Example:
https://github.com/wtrocki/api-client-template/blob/main/tools/transformer/src/transformations/oneOf.js

from oasdiff.

We just submitted a PR to merge allOf subschemas: getkin/kin-openapi#828
The accompanying branch on oasdiff takes advantage of the Merge function in getkin to merge allOf subschemas before diff.
Your comments are welcome.

from oasdiff.

Closed with #374

from oasdiff.