Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: fixed handling of additionalProperties to handle the bool/json-schema nature better #180

Merged
merged 3 commits into from
Oct 5, 2023

Conversation

TristanSpeakEasy
Copy link
Contributor

@TristanSpeakEasy TristanSpeakEasy commented Oct 3, 2023

This PR introduces a fix (via way of a breaking change) to allow a schema in an additionalProperties field to be accessed correctly. It also tried to standardise the access of bool/schema properties like additionalProperties and unevaluatedProperties.

This basically fixes the below schema not being returned as a SchemaProxy:

type: object
additionalProperties:
  allOf:
    - type: string
    - type: array
      items:
        type: string

One of the breaking changes this introduces is not dealing with additionalProperties containing non-schema arrays or maps.

It now largely expects additionalProperties to be compliant to json-schema

also incidentally I found a bug that caused an infinite hang and fixed that as part of this

@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (3c9415b) 99.80% compared to head (3a28c24) 99.74%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #180      +/-   ##
==========================================
- Coverage   99.80%   99.74%   -0.06%     
==========================================
  Files         148      148              
  Lines       10673    10646      -27     
==========================================
- Hits        10652    10619      -33     
- Misses         21       27       +6     
Flag Coverage Δ
unittests 99.74% <91.07%> (-0.06%) ⬇️

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

Files Coverage Δ
datamodel/high/base/schema.go 100.00% <100.00%> (ø)
index/extract_refs.go 99.53% <100.00%> (+<0.01%) ⬆️
datamodel/low/base/schema.go 99.46% <95.83%> (-0.21%) ⬇️
what-changed/model/schema.go 98.98% <77.77%> (-0.81%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -332,9 +331,12 @@ func (index *SpecIndex) ExtractRefs(node, parent *yaml.Node, seenPath []string,
if len(seenPath) > 0 {
lastItem := seenPath[len(seenPath)-1]
if lastItem == "properties" {
seenPath = append(seenPath, n.Value)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daveshanley also contains a fix for enum extraction that sorts the issues we were seeing after the last change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixes #175

@daveshanley
Copy link
Member

Thank you for fixing this!

The next major version jump (because this is a breaking change) will need to include #148 as well. I am not sure how much of a conflict that will create if I merge in that first, then this PR.

Does that work for you?

@TristanSpeakEasy
Copy link
Contributor Author

Yeah I can fix any conflicts when that other branch is merged

@daveshanley daveshanley merged commit d4dabca into pb33f:main Oct 5, 2023
1 of 3 checks passed
@TristanSpeakEasy TristanSpeakEasy deleted the fix-additional-properties branch October 5, 2023 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants