Multiple calls to current_user per action

[jwoertink]

This is a minor thing, but something to think about. Here's a log showing which method is being called on a basic install with Authentic:

web    | GET /
web    |  ▸ Handled by Home::Index
web    |  ▸ Ran protect_from_forgery
web    |  ▸ Ran test_backdoor
web    |  ▸ current_user?
web    |  ▸ current_user?
web    |  ▸ Rendered Lucky::WelcomePage
web    |  ▸ Sent 200 (1.56ms)
web    |
web    | GET /sign_up
web    |  ▸ Handled by SignUps::New
web    |  ▸ Ran protect_from_forgery
web    |  ▸ Ran test_backdoor
web    |  ▸ current_user?
web    |  ▸ Ran redirect_signed_in_users
web    |  ▸ current_user?
web    |  ▸ Rendered SignUps::NewPage
web    |  ▸ Sent 200 (939.0µs)
web    |
web    | POST /sign_ups
web    |  ▸ Handled by SignUps::Create
web    |  ▸ Ran protect_from_forgery
web    |  ▸ Ran test_backdoor
web    |  ▸ current_user?
web    |  ▸ Ran redirect_signed_in_users
web    |  ▸ Sent 302 (75.63ms)
web    |
web    | GET /me
web    |  ▸ Handled by Me::Show
web    |  ▸ Ran protect_from_forgery
web    |  ▸ Ran test_backdoor
web    |  ▸ current_user?
web    |  ▸ find_current_user
web    |  ▸ Ran require_sign_in
web    |  ▸ current_user?
web    |  ▸ find_current_user
web    |  ▸ Rendered Me::ShowPage
web    |  ▸ Sent 200 (2.08ms)

Notice once we get to the /me route, it's calling the find_current_user methods twice. We can't memoize that method because it takes an argument, but if we can find a way to memoize something in here, that could reduce the load time just a tad more.

[paulcsmith]

Maybe we memoize in the auth mixins for current_user since those don’t take any args!

Nice find BTW On Sep 14, 2019, 11:44 PM -0400, Jeremy Woertink notifications@github.com, wrote:

This is a minor thing, but something to think about. Here's a log showing which method is being called on a basic install with Authentic: web | GET /

web | ▸ Handled by Home::Index

web | ▸ Ran protect_from_forgery

web | ▸ Ran test_backdoor

web | ▸ current_user?

web | ▸ current_user?

web | ▸ Rendered Lucky::WelcomePage

web | ▸ Sent 200 (1.56ms)

web |

web | GET /sign_up

web | ▸ Handled by SignUps::New

web | ▸ Ran protect_from_forgery

web | ▸ Ran test_backdoor

web | ▸ current_user?

web | ▸ Ran redirect_signed_in_users

web | ▸ current_user?

web | ▸ Rendered SignUps::NewPage

web | ▸ Sent 200 (939.0µs)

web |

web | POST /sign_ups

web | ▸ Handled by SignUps::Create

web | ▸ Ran protect_from_forgery

web | ▸ Ran test_backdoor

web | ▸ current_user?

web | ▸ Ran redirect_signed_in_users

web | ▸ Sent 302 (75.63ms)

web |

web | GET /me

web | ▸ Handled by Me::Show

web | ▸ Ran protect_from_forgery

web | ▸ Ran test_backdoor

web | ▸ current_user?

web | ▸ find_current_user

web | ▸ Ran require_sign_in

web | ▸ current_user?

web | ▸ find_current_user

web | ▸ Rendered Me::ShowPage

web | ▸ Sent 200 (2.08ms)

Notice once we get to the /me route, it's calling the find_current_user methods twice. We can't memoize that method because it takes an argument, but if we can find a way to memoize something in here, that could reduce the load time just a tad more. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[vlazar]

Looks like this one can be closed?

I've generated full app with Lucky 0.18.0 and don't see the duplicates in logs.

[jwoertink]

@vlazar did you generate the app with the auth stuff? I don't think we've made any changes that would have affected this, but I'll take a look

[paulcsmith]

I think the logging was added by the issue reporter. But there are still multiple SQL calls. I think we need to add memoize in the generated code to avoid calling the db every time current_user is called On Oct 9, 2019, 11:28 AM -0400, Jeremy Woertink notifications@github.com, wrote:

@vlazar did you generate the app with the auth stuff? I don't think we've made any changes that would have affected this, but I'll take a look — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[paulcsmith]

https://github.com/luckyframework/lucky_cli/blob/a0ac6e52bcb0fd65605019905dd06fdb3e6582fa/src/api_authentication_app_skeleton/src/actions/mixins/api/auth/helpers.cr#L2

and https://github.com/luckyframework/lucky_cli/blob/a0ac6e52bcb0fd65605019905dd06fdb3e6582fa/src/browser_app_skeleton/src/actions/browser_action.cr.ecr#L28

need memoize

[vlazar]

Will memoize work in this case with User? type?

The Lucky guides states that

There are a few caveats to the memoize macro. It will not memoize false, or nil return values. If your method returns a “falsey” value, then it will be ran each time.

[paulcsmith]

Good point! Haha. I guess I should know my own framework a bit better :P We'll have to figure something else out then. Maybe we need to bite but bullet and add some query cache to Avram. That'd fix this automatically which would be very cool

[jwoertink]

It's definitely time for some query cache!

[vlazar]

So I assume building memoize macro which allows for nil and false results to be memoized is impossible in Crystal?

[paulcsmith]

It is possible, but we haven't implemented it yet, and it can be error-prone

[vlazar]

@paulcsmith If I understood you correctly having memoize macro allowing nil and false might cause more trouble and not worth it?

This is not a good first issue then :)

[jwoertink]

Yeah, I didn't add in nil or false support because it causes a lot of other issues. Same with passing arguments. There's some trade-offs, but our current memoize support works pretty great. If you're looking for some more "good first issues", I'll go through and start marking some stuff 😄 Thanks for all the help!

[vlazar]

@jwoertink Yeah, I'm looking around currently, going random directions to learn some Crystal and Lucky. Maybe I should also start building some app in Lucky.

[matthewmcgarvey]

Just an update: we can now cache nil and false results with the memoize macro https://github.com/luckyframework/lucky/commit/9be023f06a9dd49a06deb5d6eb1e046c7e366f0f

[paulcsmith]

@matthewmcgarvey YES! Thanks for doing that. I'll update my PR and merge that once green