Skip to content

Conversation

@shockey
Copy link
Contributor

@shockey shockey commented Apr 13, 2018

originally from @ponelat in #1264

Description

This adds a test, highlighting a bug.
With nested allOfs + $refs.

Example defintition fragment:

definitions:
  Alpha:
    allOf:
    - type: object
    properties:
      test:
        $ref: '#/definitions/Bravo'
 
  Bravo:
    allOf: 
      - type: object
        properties:
          two: 
            type: string

Causes: TypeError: Cannot read property 'Alpha' of undefined

Types of changes

  • No code changes (changes to documentation, CI, metadata, etc)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@shockey shockey mentioned this pull request Apr 13, 2018
10 tasks
@shockey
Copy link
Contributor Author

shockey commented Apr 13, 2018

Okay, everything appears good now - looks like allOf was dying on a replace patch, which may have been selected for the allOf plugin's consideration incorrectly due to the nested structure we have.

I simply sidestepped this by not trying to keep looking for a value that doesn't exist. The tests pass, @ponelat I'm going to tag you for review to ensure this is the spirit of what you were looking for here.

@shockey shockey requested a review from ponelat April 13, 2018 02:14
@shockey
Copy link
Contributor Author

shockey commented Apr 13, 2018

Merging, per @ponelat:

I'm happy with it being Just Merged. It may not be the solution, but again, its a harmless looking PR... what could go wrong?

he said, on the eve of Friday night... the 13th!

@shockey shockey merged commit 98c9a1c into master Apr 13, 2018
@ponelat
Copy link
Contributor

ponelat commented Apr 16, 2018

Thanks @shockey this fixes my issue!

@ponelat ponelat deleted the test/add-nested-allof-with-ref branch April 16, 2018 13:25
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.

3 participants