Breaking change check: New request required default param on existing path

[Jonathan-Rosenberg]

This check will notify a breaking change if a new required default param was added to a request on an existing modified path.

[codecov-commenter]

Codecov Report

Merging #376 (6d0cd01) into main (1bf3971) will increase coverage by 0.06%.
The diff coverage is 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main     #376      +/-   ##
==========================================
+ Coverage   81.74%   81.80%   +0.06%     
==========================================
  Files         194      195       +1     
  Lines       10589    10624      +35     
==========================================
+ Hits         8656     8691      +35     
  Misses       1635     1635              
  Partials      298      298              

Flag	Coverage Δ
unittests	81.80% <100.00%> (+0.06%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
...er/check-new-request-non-path-default-parameter.go	100.00% <100.00%> (ø)
checker/default_checks.go	92.00% <100.00%> (+0.06%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[reuvenharrison]

Hi @Jonathan-Rosenberg and thanks for the nice contribution.
I haven't reviewed the code yet but I noticed that the new check isn't being run by the oasdiff cmd-line.
To make it run you need to add it to the defaultChecks function:
https://github.com/jonathan-rosenberg/oasdiff/blob/e3b94fcb8f4b9a163072fd49dcc0262c2956e3ec/checker/default_checks.go#L61

[Jonathan-Rosenberg]

Hi @reuvenharrison,
Forgot to git add it.
Thanks!

[reuvenharrison]

It seems like this test produces a false-positive when there are no endpoints under the path.
For example, comparing the two specs below reports a breaking-change error which in fact isn't breaking since there are no endpoints.
While this is a rare case, I still think we should take it into considration.

info:
  title: Tufin
  version: 1.0.0
openapi: 3.0.3
paths:
  /api/test:
    parameters:
      - in: header
        name: param1
        required: false

To:

info:
  title: Tufin
  version: 1.0.0
openapi: 3.0.3
paths:
  /api/test:
    parameters:
      - in: header
        name: param2
        required: true

[Jonathan-Rosenberg]

Hi @reuvenharrison,
Added a check and a test for the edge case.
Thanks

[reuvenharrison]

I suggest to report this error on each affected endpoint rather than once per path.
The reason is that ApiChange is designed to report a breaking change at the endpoint (operation level), for example, it has an operation field.
Another reason is that the breaking change may only apply to a subset of the operations, for exampple, consider moving a param from op level to path level.

The downside to this approach is that the error may be reported multiple times, one for each operation, but this is ok since we will eventually support the ability to group breaking changes (see #371).

[reuvenharrison]

This still need to be implemented.

[reuvenharrison]

I'll add a new issue to handle this

[reuvenharrison]

This check refers to a path rather than an endpoint and because of that the resulting ApiChange lacks the Operation and OperationID fields.
Moreover,

[Jonathan-Rosenberg]

The removal of this code makes the check incorrect.
The level will be ERR even if the parameter wasn't added.
I'm creating a fix.

[reuvenharrison]

I was under the impression that it is redundant because we are iterating on pathItem.ParametersDiff.Added
Can you replicate the issue with a unit test?

[Jonathan-Rosenberg]

We do that, but since the pathItem.Revision.Parameters field is a slice and not a map, we traverse through it to find the ParameterRef object that matches the name we found in the Added map.
PR.