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

Add failing test for cyclic allOf + $ref in resolve subtree #1275

Merged
merged 14 commits into from May 3, 2018

Conversation

Projects
None yet
3 participants
@ponelat
Contributor

ponelat commented Apr 4, 2018

No description provided.

@webron

This comment has been minimized.

Show comment
Hide comment
@webron

webron Apr 4, 2018

Member

Come on, @ponelat, you can also add the solution, not just a failing test.

Member

webron commented Apr 4, 2018

Come on, @ponelat, you can also add the solution, not just a failing test.

@webron webron requested a review from shockey Apr 4, 2018

@ponelat

This comment has been minimized.

Show comment
Hide comment
@ponelat

ponelat Apr 4, 2018

Contributor

I preferred the delayed responses, from the more westerly timezones.

Contributor

ponelat commented Apr 4, 2018

I preferred the delayed responses, from the more westerly timezones.

@webron

This comment has been minimized.

Show comment
Hide comment
@webron

webron Apr 4, 2018

Member

This is MOAR better.

Member

webron commented Apr 4, 2018

This is MOAR better.

@webron

This comment has been minimized.

Show comment
Hide comment
@webron

webron Apr 4, 2018

Member

And @ponelat, the build is failing.

Member

webron commented Apr 4, 2018

And @ponelat, the build is failing.

@ponelat

This comment has been minimized.

Show comment
Hide comment
@ponelat

ponelat Apr 4, 2018

Contributor

Its in the title @webron . The chunk of text near the top of the page

Contributor

ponelat commented Apr 4, 2018

Its in the title @webron . The chunk of text near the top of the page

@webron

This comment has been minimized.

Show comment
Hide comment
@webron
Member

webron commented Apr 4, 2018

image

@ponelat

This comment has been minimized.

Show comment
Hide comment
@ponelat

ponelat Apr 4, 2018

Contributor

image

Contributor

ponelat commented Apr 4, 2018

image

@shockey

This comment has been minimized.

Show comment
Hide comment
@shockey

shockey Apr 9, 2018

Member

Hmmmmmmmm

Member

shockey commented Apr 9, 2018

Hmmmmmmmm

@shockey shockey added this to the April 13, 2018 milestone Apr 9, 2018

@shockey

This comment has been minimized.

Show comment
Hide comment
@shockey

shockey Apr 20, 2018

Member

Miiiiight be the same issue as #1243.

Member

shockey commented Apr 20, 2018

Miiiiight be the same issue as #1243.

@shockey

This comment has been minimized.

Show comment
Hide comment
@shockey

shockey Apr 20, 2018

Member

An update here: I hacked on this for quite a while on Thursday, but haven't gotten all green lights yet. I've set this aside so I can get some other things into this release cycle, and will pick it back up next week 😄

Member

shockey commented Apr 20, 2018

An update here: I hacked on this for quite a while on Thursday, but haven't gotten all green lights yet. I've set this aside so I can get some other things into this release cycle, and will pick it back up next week 😄

@shockey

This comment has been minimized.

Show comment
Hide comment
@shockey

shockey Apr 28, 2018

Member

Here we are again.. I spent about two days this week on this issue and still haven't struck at the heart of it.

I'm punting again so I can land some related fixes for tonight's release - they're in the same branch, so I'll need to graft them out.

This will be at the top of my list.... Monday.

Swagger-Client #1275 notes for week of Monday, April 23

  • the allOf plugin is not a factor here, it doesn't run until after the error
  • i set a console log to delineate each call to the $ref plugin with.... a horizontal line! this gives me a good view into the cycles involved with the issue, and i refer to each step as a frame.

$ref plugin frames:

  1. resolve definitions one $ref with value at #/definitions/two
  2. resolve definitions two items $ref with value at #/definitions/three
  3. resolve definitions three allOf 0 properties alternate_product_code $ref with value at #/definitions/three, but abort without a patch since the $ref in question is a direct cycle
  4. resolve definitions one items allOf 0 properties alternate_product_code $ref but error since the target $ref does not exist in the current specmap state

  • theory: the traverser is working off of data that has been mutated and those changes have not been handed back to specmap.state
  • by some mechanism, when we resolved two's ref to the value three, the already-resolved value for two inside of one also received the resolution for two's ref to three - from the traverser's view.
  • the traverser saw this, and asked the ref plugin to look at the self-referencing three ref inside of one->two, but the specmap state was not aware of this data inside of one, so it blew up.
  • this may be object inheritance / reference (more specifically, lack of it in one place). we definitely have that going on in the patch system... why is it being synced into the traverser's context but not specmap's?
  • the culprit could be the refCache logic that's hanging out in the traverser, or alternatively something in the patch result processor not making it back into specmap state.

ok - it is an object inheritance issue.. but it's not an accident. when parsing replace patches when meta patching enabled, we break inheritance before replacing the target location so we can place metadata at the target without it also appearing at the source.

disabling that functionality is a bad idea, for a few reasons... (a) a worse user experience and a breaking change, (b) breakage of quite a few tests, and (c) likely to cause strange adverse behavior in Swagger-UI.

so that leaves us to work in the other direction - if we can't make specmap's internals more optimistic, we have to make the traverser more pessimistic.


traverse's patch argument, in the fourth $ref frame, is a replace patch that would add the correct two->three resolved data to one. that patch, however, is never making it to the applyPatch method, so specmap isn't ever updating its state according to that patch. where is this patch coming from, going, and how is it manging to be partially applied?

this patch is being considered a mutation, so traverse is walking the mutation's value through the plugins without ever actually applying the mutation patch itself.

because the mutation patch disappears, but the patches generated from that mutation persist, the application of the generated patches fail.

$ref frame #4 yields no patches - this appears to be causing the mutation patch drop, while its derivatives hang around.

Member

shockey commented Apr 28, 2018

Here we are again.. I spent about two days this week on this issue and still haven't struck at the heart of it.

I'm punting again so I can land some related fixes for tonight's release - they're in the same branch, so I'll need to graft them out.

This will be at the top of my list.... Monday.

Swagger-Client #1275 notes for week of Monday, April 23

  • the allOf plugin is not a factor here, it doesn't run until after the error
  • i set a console log to delineate each call to the $ref plugin with.... a horizontal line! this gives me a good view into the cycles involved with the issue, and i refer to each step as a frame.

$ref plugin frames:

  1. resolve definitions one $ref with value at #/definitions/two
  2. resolve definitions two items $ref with value at #/definitions/three
  3. resolve definitions three allOf 0 properties alternate_product_code $ref with value at #/definitions/three, but abort without a patch since the $ref in question is a direct cycle
  4. resolve definitions one items allOf 0 properties alternate_product_code $ref but error since the target $ref does not exist in the current specmap state

  • theory: the traverser is working off of data that has been mutated and those changes have not been handed back to specmap.state
  • by some mechanism, when we resolved two's ref to the value three, the already-resolved value for two inside of one also received the resolution for two's ref to three - from the traverser's view.
  • the traverser saw this, and asked the ref plugin to look at the self-referencing three ref inside of one->two, but the specmap state was not aware of this data inside of one, so it blew up.
  • this may be object inheritance / reference (more specifically, lack of it in one place). we definitely have that going on in the patch system... why is it being synced into the traverser's context but not specmap's?
  • the culprit could be the refCache logic that's hanging out in the traverser, or alternatively something in the patch result processor not making it back into specmap state.

ok - it is an object inheritance issue.. but it's not an accident. when parsing replace patches when meta patching enabled, we break inheritance before replacing the target location so we can place metadata at the target without it also appearing at the source.

disabling that functionality is a bad idea, for a few reasons... (a) a worse user experience and a breaking change, (b) breakage of quite a few tests, and (c) likely to cause strange adverse behavior in Swagger-UI.

so that leaves us to work in the other direction - if we can't make specmap's internals more optimistic, we have to make the traverser more pessimistic.


traverse's patch argument, in the fourth $ref frame, is a replace patch that would add the correct two->three resolved data to one. that patch, however, is never making it to the applyPatch method, so specmap isn't ever updating its state according to that patch. where is this patch coming from, going, and how is it manging to be partially applied?

this patch is being considered a mutation, so traverse is walking the mutation's value through the plugins without ever actually applying the mutation patch itself.

because the mutation patch disappears, but the patches generated from that mutation persist, the application of the generated patches fail.

$ref frame #4 yields no patches - this appears to be causing the mutation patch drop, while its derivatives hang around.

shockey added some commits Apr 28, 2018

add `build-dev-bundle` for better debugging within a live browser
Also introduces a friendlier Babel configuration in development, tuned
for good debugging in Chrome.

@shockey shockey merged commit 9ce501c into master May 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@shockey

This comment has been minimized.

Show comment
Hide comment
@shockey

shockey May 3, 2018

Member

OK! Fixed!

The solution was to update specmap's internal mutation applier to also break inheritance when meta patching is enabled. This keeps both sides of the system in harmony, avoiding the generation of patches for mutations that have not yet actually been applied but have bled through in specmap state due to inheritance.

Note that we also dropped testing for Node.js 4, since it reaches end-of-life this month (https://twitter.com/nodejs/status/968177367033229315). Going forward, we're testing Node.js 6, the latest LTS version, and the latest version.

I included the version changes in this PR because... the development experience polishing I did in the process of fixing this required a bump to webpack@3, which needs Node.js 6.

Member

shockey commented May 3, 2018

OK! Fixed!

The solution was to update specmap's internal mutation applier to also break inheritance when meta patching is enabled. This keeps both sides of the system in harmony, avoiding the generation of patches for mutations that have not yet actually been applied but have bled through in specmap state due to inheritance.

Note that we also dropped testing for Node.js 4, since it reaches end-of-life this month (https://twitter.com/nodejs/status/968177367033229315). Going forward, we're testing Node.js 6, the latest LTS version, and the latest version.

I included the version changes in this PR because... the development experience polishing I did in the process of fixing this required a bump to webpack@3, which needs Node.js 6.

@webron webron deleted the feature/subtree-with-allof-ref-cyclic branch May 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment