Skip to content

Restore the behavior of journey root node methods #43006

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

Merged

Conversation

composerinteralia
Copy link
Member

@composerinteralia composerinteralia commented Aug 12, 2021

#39935 changed the behavior of
Path::Pattern#spec and Route#ast to return an Ast rather than the
root Node. After eager loading, however, we clear out the Ast to
limit retained memory and these methods return nil.

While these methods are not public and they aren't used internally after
eager loading, having them return nil makes it difficult for
applications that had been using these methods to get access to the
root Node. (See #39935 (review))

This commit restores the behavior of these two methods to return the
root Node. The Ast is still available via Path::Pattern#ast, and
we still clear it out after eager loading.

Now that spec is a Node and not an Ast masquerading as one, we can
get rid of the delegate methods on `Ast.

Since Route#ast now returns the root Node, the newly added
Route#ast_root is no longer necessary so I've removed it.

I also removed the unused @decorated_ast method, which should have
been removed in #39935.

cc @rafaelfranca

rails#39935 changed the behavior of
`Path::Pattern#spec` and `Route#ast` to return an `Ast` rather than the
root `Node`. After eager loading, however, we clear out the `Ast` to
limit retained memory and these methods return `nil`.

While these methods are not public and they aren't used internally after
eager loading, having them return `nil` makes it difficult for
applications that had been using these methods to get access to the
root `Node`.

This commit restores the behavior of these two methods to return the
root `Node`. The `Ast` is still available via `Path::Pattern#ast`, and
we still clear it out after eager loading.

Now that spec is a `Node` and not an `Ast` masquerading as one, we can
get rid of the delegate methods on `Ast.

Since `Route#ast` now returns the root `Node`, the newly added
`Route#ast_root` is no longer necessary so I've removed it.

I also removed the unused `@decorated_ast` method, which should have
been removed in rails#39935.
@rails-bot rails-bot bot added the actionpack label Aug 12, 2021
@rafaelfranca rafaelfranca merged commit c84dec3 into rails:main Aug 12, 2021
@composerinteralia composerinteralia deleted the journey-root-node-methods branch August 12, 2021 17:45
rafaelfranca added a commit to Shopify/rails that referenced this pull request Aug 12, 2021
…de-methods

Restore the behavior of journey root node methods
@@ -140,7 +135,7 @@ def required_defaults
end

def glob?
ast.glob?
path.ast.glob?
Copy link

@kyrylo kyrylo Jan 13, 2022

Choose a reason for hiding this comment

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

When eager_load = true, this method call will fail because path.ast becomes nil. We hit this issue in airbrake/airbrake#1177 (comment). In the very same issue we have an example app that reproduces the problem.

The code that messes with this method lives in https://github.com/airbrake/airbrake/blob/4d7cacb7c3cef6b3d7f4cecb31bc16afeee7b832/lib/airbrake/rails/app.rb#L48

Is there any way to achieve the old behavior (glob? doesn't raise NoMethodError when eager_load = true)

/cc @composerinteralia

Copy link
Member Author

@composerinteralia composerinteralia Jan 13, 2022

Choose a reason for hiding this comment

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

I'll take a closer look later today, but I think route.path.spec.any?(Nodes::Star) would work in place of route.glob?. That's basically what the implementation of route.glob? used to be before #39935.

Copy link

Choose a reason for hiding this comment

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

route.path.spec.any?(Nodes::Star) does the job, thank you.

I appreciate that you will take a closer look. Please let me know the results (even if you don't have a chance to take a look, please let me know, so that I can release a fix 😁).

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I've refreshed my memory on this.

Route#glob? used to do path.spec.any?(Nodes::Star), visiting every node to see if any of them were stars. #39935 changed Route#glob? to get the value from a new Journey::Ast object instead. Journey::Ast improves the performance of adding routes by visiting the nodes upfront and caching various values along the way so we don't have to visit them again.

Once Rails is done adding the route, it doesn't need the Journey::Ast anymore. To avoid retaining memory for objects that we won't use, we clear out all the Journey::Ast objects as part of eager loading.

I was aware that clearing out the Journey::Ast would break some methods, but these methods were all private Rails methods that Rails doesn't need again after adding the route. I think it's possible to get the glob? method working again by caching it's value, but I'll defer to folks on the core team about whether we want that. It would add an extra instance variable to every route, which might not be desirable to support a private method that Rails doesn't use after eager loading.

route.path.spec is also technically private, but various applications are using it and it's less likely to change (I actually did attempt to change it, but brought back the original behavior in this PR because folks were relying on it). I see that airbrake is already using route.path.spec, so using it instead of route.glob? might be good anyway to limit airbrake's dependence on private Rails methods.

Copy link

Choose a reason for hiding this comment

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

Thanks for your elaborative answer. I appreciate it. I will use route.path.spec.any?(Nodes::Star) instead.
You may want to consider exposing routes in a public API since this is useful for Application Performance Monitoring (APM). Airbrake monitors performance of an app's routes, and therefore we need to know the routes.

kyrylo added a commit to airbrake/airbrake that referenced this pull request Jan 17, 2022
Fixes #1177
(airbrake gem don't work with 7.0.0.alpha2 release)

Alternative to #1206

`glob?` on Rails 7+ raises error because Rails 7+ doesn't keep the route AST in
memory anymore. A Rails contributor who worked on the related code
[suggested][1] to use `Nodes::Star` instead, since that used to be the previous
implementation of the `glob?` method.

[1]: rails/rails#43006 (comment)
kyrylo added a commit to airbrake/airbrake that referenced this pull request Jan 17, 2022
Fixes #1177
(airbrake gem don't work with 7.0.0.alpha2 release)

Alternative to #1206

`glob?` on Rails 7+ raises error because Rails 7+ doesn't keep the route AST in
memory anymore. A Rails contributor who worked on the related code
[suggested][1] to use `Nodes::Star` instead, since that used to be the previous
implementation of the `glob?` method.

[1]: rails/rails#43006 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants