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 infinite loop when mounting endpoint with same superclass #1727

Merged
merged 1 commit into from
Jan 6, 2018

Conversation

jkowens
Copy link
Contributor

@jkowens jkowens commented Jan 4, 2018

Parent and child classes were pointing to the same inheritable setting creating an infinite loop when retrieving values. Fixes #1683

This was a regression introduced with commit 2c4d574.

@jkowens
Copy link
Contributor Author

jkowens commented Jan 4, 2018

This was a bit of a mind bender to work through, so I definitely welcome other eyes on this...especially if they are familiar with how InheritableSetting works. 😉

@jkowens jkowens mentioned this pull request Jan 4, 2018
@dblock
Copy link
Member

dblock commented Jan 5, 2018

cc: @pablonahuelgomez

@dblock
Copy link
Member

dblock commented Jan 5, 2018

Amazing.

I think we need specs that call out this issue and scenario rather than making the repro part of the loading performance spec - revert that one and write a more minimal spec that otherwise fails with this particular error?

@pablonahuelgomez
Copy link
Contributor

This is a great catch @jkowens 👍

@dblock
Copy link
Member

dblock commented Jan 5, 2018

@jkowens Will merge if you can address my comment above, please.

@jkowens jkowens changed the title Fix infinite recursive call when mounting endpoint with common ancestor Fix infinite loop when mounting endpoint with same superclass Jan 6, 2018
Subclass APIs were being assigned the same top level inheritable
settings from the superclass. When an API attempted to mount another
API with the same superclass, the same setting object would be added to
the setting stack. When an attempt was made to retrieve a value from the
settings, it would sit in loop calling itself.
@jkowens
Copy link
Contributor Author

jkowens commented Jan 6, 2018

@dblock I've added a spec for this specific scenario as suggested. Thanks for the feedback 👍

@dblock dblock merged commit 35895e5 into ruby-grape:master Jan 6, 2018
@dblock
Copy link
Member

dblock commented Jan 6, 2018

Merged, thank you.

@jkowens jkowens deleted the fix-1683 branch January 10, 2018 22:31
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