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

Return cached result for has_one relationships #933

Merged
merged 3 commits into from Aug 23, 2015

Conversation

Projects
None yet
3 participants
@jacobwgillespie
Contributor

jacobwgillespie commented Aug 21, 2015

We'll see if Travis tests pass - ideally I'd like to add a test to check if cached results are used, but I haven't figured out the test setup yet.

Before this PR:

b = Book.find(book_id)
b.author # queries the database
b.author # queries the database again

After this PR:

b = Book.find(book_id)
b.author # queries the database
b.author # returns the cached copy of the author

b.reload # reloads the book from the database
b.author # queries the database
b.author # returns the cached copy of the author
@cheerfulstoic

This comment has been minimized.

Show comment
Hide comment
@cheerfulstoic

cheerfulstoic Aug 21, 2015

Member

This is great, thanks!

If you look at asociation_proxy_spec.rb you'll see that there is a expect_queries helper method. That's defined in the spec_helper which uses a $expect_queries_count global variable. Not great certainly, but only used in the specs (and it's used to track a global count of queries, so maybe not the worst usage).

Anyway, you could use that to make the spec(s) if you want. If that's good them I'm happy to bring this into the 5.1.x branch and release a patch

Member

cheerfulstoic commented Aug 21, 2015

This is great, thanks!

If you look at asociation_proxy_spec.rb you'll see that there is a expect_queries helper method. That's defined in the spec_helper which uses a $expect_queries_count global variable. Not great certainly, but only used in the specs (and it's used to track a global count of queries, so maybe not the worst usage).

Anyway, you could use that to make the spec(s) if you want. If that's good them I'm happy to bring this into the 5.1.x branch and release a patch

@cheerfulstoic

This comment has been minimized.

Show comment
Hide comment
@cheerfulstoic

cheerfulstoic Aug 21, 2015

Member

(oh, and association_proxy_spec isn't the best name anymore because association proxies aren't always used there. For later refactoring ;)

Member

cheerfulstoic commented Aug 21, 2015

(oh, and association_proxy_spec isn't the best name anymore because association proxies aren't always used there. For later refactoring ;)

@jacobwgillespie

This comment has been minimized.

Show comment
Hide comment
@jacobwgillespie

jacobwgillespie Aug 21, 2015

Contributor

Sweet, added! That was fairly painless :)

Contributor

jacobwgillespie commented Aug 21, 2015

Sweet, added! That was fairly painless :)

@jacobwgillespie

This comment has been minimized.

Show comment
Hide comment
@jacobwgillespie

jacobwgillespie Aug 22, 2015

Contributor

They're failing, but it looks like it's not related to the added tests...

Contributor

jacobwgillespie commented Aug 22, 2015

They're failing, but it looks like it's not related to the added tests...

@cheerfulstoic

This comment has been minimized.

Show comment
Hide comment
@cheerfulstoic

cheerfulstoic Aug 22, 2015

Member

Ah, fiddly-widdly-timey-wimey issues. That's happened before. I'll try to fix it for good now

Member

cheerfulstoic commented Aug 22, 2015

Ah, fiddly-widdly-timey-wimey issues. That's happened before. I'll try to fix it for good now

@cheerfulstoic

This comment has been minimized.

Show comment
Hide comment
@cheerfulstoic

cheerfulstoic Aug 22, 2015

Member

Ah, interesting. So there was one timey-wimey issue, but it looks like the JRuby specs are failing because in ./spec/e2e/has_one_spec.rb:63 there were 3 queries expected but only 2 happened. That seems more related to your change

Member

cheerfulstoic commented Aug 22, 2015

Ah, interesting. So there was one timey-wimey issue, but it looks like the JRuby specs are failing because in ./spec/e2e/has_one_spec.rb:63 there were 3 queries expected but only 2 happened. That seems more related to your change

@cheerfulstoic

This comment has been minimized.

Show comment
Hide comment
@cheerfulstoic

cheerfulstoic Aug 22, 2015

Member

Weird... that line has nothing to do with expect_queries.... I've tried running it locally with JRuby with no issues...

Member

cheerfulstoic commented Aug 22, 2015

Weird... that line has nothing to do with expect_queries.... I've tried running it locally with JRuby with no issues...

@cheerfulstoic

This comment has been minimized.

Show comment
Hide comment
@cheerfulstoic

cheerfulstoic Aug 22, 2015

Member

I've got to head out for the night, but I tried re-running the builds and they're still failing, so it's reproducible on Travis, at least. I won't be around tomorrow but I'll take a look again soon. You're welcome to figure it out as well, of course

Member

cheerfulstoic commented Aug 22, 2015

I've got to head out for the night, but I tried re-running the builds and they're still failing, so it's reproducible on Travis, at least. I won't be around tomorrow but I'll take a look again soon. You're welcome to figure it out as well, of course

@jacobwgillespie

This comment has been minimized.

Show comment
Hide comment
@jacobwgillespie

jacobwgillespie Aug 22, 2015

Contributor

It's passing! Realized I accidentally had the b.reload in the expect_queries - was testing the wrong thing. :) It was failing on my local machine too under jruby, but this fixes everywhere. Thanks for the help!

Contributor

jacobwgillespie commented Aug 22, 2015

It's passing! Realized I accidentally had the b.reload in the expect_queries - was testing the wrong thing. :) It was failing on my local machine too under jruby, but this fixes everywhere. Thanks for the help!

@cheerfulstoic

This comment has been minimized.

Show comment
Hide comment
@cheerfulstoic

cheerfulstoic Aug 23, 2015

Member

Awesome! Thanks! I'll merge and cut a patch shortly

Member

cheerfulstoic commented Aug 23, 2015

Awesome! Thanks! I'll merge and cut a patch shortly

cheerfulstoic added a commit that referenced this pull request Aug 23, 2015

Merge pull request #933 from playlist-forks/fix-has-one-cache
Return cached result for has_one relationships

@cheerfulstoic cheerfulstoic merged commit ee9b5b1 into neo4jrb:master Aug 23, 2015

1 check passed

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

@jacobwgillespie jacobwgillespie deleted the playlist-forks:fix-has-one-cache branch Aug 23, 2015

@jacobwgillespie

This comment has been minimized.

Show comment
Hide comment
@jacobwgillespie

jacobwgillespie Aug 23, 2015

Contributor

Thank you! 🍰

Contributor

jacobwgillespie commented Aug 23, 2015

Thank you! 🍰

@cheerfulstoic

This comment has been minimized.

Show comment
Hide comment
@cheerfulstoic

cheerfulstoic Aug 23, 2015

Member

Released as 5.1.3. Thanks again!

Member

cheerfulstoic commented Aug 23, 2015

Released as 5.1.3. Thanks again!

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 24, 2017

Coverage Status

Changes Unknown when pulling 1eb053a on playlist-forks:fix-has-one-cache into ** on neo4jrb:master**.

coveralls commented May 24, 2017

Coverage Status

Changes Unknown when pulling 1eb053a on playlist-forks:fix-has-one-cache into ** on neo4jrb:master**.

@cheerfulstoic

This comment has been minimized.

Show comment
Hide comment
@cheerfulstoic

cheerfulstoic May 24, 2017

Member

Thanks coveralls... :-/

Member

cheerfulstoic commented May 24, 2017

Thanks coveralls... :-/

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