implement request/response allOf checkers #340
Conversation
| if mediaTypeDiff.SchemaDiff.AllOfDiff != nil && mediaTypeDiff.SchemaDiff.AllOfDiff.Added > 0 { | ||
| result = append(result, ApiChange{ | ||
| Id: "request-body-all-of-added", | ||
| Level: ERR, |
| if mediaTypeDiff.SchemaDiff.AllOfDiff != nil && mediaTypeDiff.SchemaDiff.AllOfDiff.Deleted > 0 { | ||
| result = append(result, ApiChange{ | ||
| Id: "request-body-all-of-removed", | ||
| Level: ERR, |
There was a problem hiding this comment.
I don't understand why this is breaking:
Suppose a client sends a request body and it matched "All Of" the schemas in the spec, now we remove one of the allOf specs, the same request body will still match.
Am I missing something?
There was a problem hiding this comment.
you are right, the request body will still match if one of the allOf specs is removed.
I changed to WARN and the BC message to // BC: removing 'allOf' schema from the request body or request body property is breaking with warn, similar (level + BC message) to what we have for request-property-removed.
They are very similar, in the case of allOf we remove one or more properties from the request.
WDYT?
There was a problem hiding this comment.
I see what you mean, and I agree with your solution, but I'd like to clarify the explanation for the record:
Removing a schema from allOf, could result in the effective removal of a property, and as such, it should be treated like request-property-removed.
However, removing a schema from allOf, doesn't necessarily result in the removal of a property, for example when the 2nd object is removed here:
schema:
allOf:
- type: object
properties:
prop1:
type: string
description: Some description
- type: object
properties:
prop1:
type: string
maxLength: 30
| if propertyDiff.AllOfDiff.Added > 0 { | ||
| result = append(result, ApiChange{ | ||
| Id: "request-property-all-of-added", | ||
| Level: ERR, |
| if propertyDiff.AllOfDiff.Deleted > 0 { | ||
| result = append(result, ApiChange{ | ||
| Id: "request-property-all-of-removed", | ||
| Level: ERR, |
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## main #340 +/- ##
==========================================
+ Coverage 79.03% 79.36% +0.32%
==========================================
Files 186 188 +2
Lines 9601 9768 +167
==========================================
+ Hits 7588 7752 +164
- Misses 1724 1725 +1
- Partials 289 291 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
No description provided.