Fixes bug with lazy associations and db binding

[ceterumnet]

Sorry about the whitespace differences...my emacs is setup to save with cleanup of whitespace...

The issue that this fixes is when you have a lazily loaded association, the database binding is escaped. I've fixed this by associating the currently bound db to the ent. I'm not sure this is the best fix, but it was the least intrusive.

[ceterumnet]

I'm not sure why my change caused the CI build to fail.

[immoh]

I think you're on right track here. But instead of introducing new dynamic var *current-db*, I think we should change the behavior of with-db so that the default connection korma.db/_connection is overridden by the given db inside the with-db block, and then rolled back. In my understanding that is the purpose of with-db: override the default connection temporarily.

In order to propagate the connection to the lazily loaded associations, I think assigning the db to the entity is fine. But if there's a entity-specific db connection defined we shouldn't override it.

What do you think, does it make sense?

General feedback about the pull request: please lose the whitespace changes, irrelevant changes (e.g. project.clj) and squash the commit history so that it is easier to read. Thanks!

[ceterumnet]

Take a look at my latest pull request and let me know if that works.

[immoh]

In addition to line comments I wrote, can you please add a failing test that this PR fixes. I think you already had it in gist you linked to the issue.

Otherwise, it is starting to be ready for merge.

@bitemyapp @AlexBaranosky If you have time, can you please also check that this is ok.

[ceterumnet]

I wasn't able to easily create a failing test. Because of the way the use-fixtures evaluates - I don't think the issue reproduces with the way the current tests are structured. I can add a separate test file if you'd like, but the changes as a whole are verified by the existing tests I think.

Let me know what you think.

[bitemyapp]

@ceterumnet @immoh problems at the moment are:

The PR needs updated offline so it can merge cleanly.

There needs to be a breaking test. I am extremely leery of any "fixes" not backed by at least one breaking test that the commits fix. The library is too large and not well enough known any one person to slack on this. Particularly because of the scope/complexity of the integration side of the library. Our testing story already needs a lot of improvement, I'd like to avoid worsening it by changing things without some kind of empirical basis.

Also please squash the commits.

[immoh]

I agree about the test. I would create ns korma.test.integration.with-db for the test, I think we need to keep on adding similar kind of integration tests in the future.

[AlexBaranosky]

@ceterumnet yeah, just add a totally new test file, or refactor us away from using fixtures. I am not a fan of fixtures in general, because they force every test in the ns to use the fixture. It'd be nicer if you could pass keywords for a fixture to the test, and mix and match fixtures in a less all-or-nothing way, but alas this is not so...

[ceterumnet]

Ok - I've pushed a squashed commit with all of the changes. I think this is pretty close now, let me know if there is anything missing or any other feedback.

[AlexBaranosky]

This PR can't be automatically merged. Is your branch up to date with master?

[AlexBaranosky]

@ceterumnet two comments: 1) can you please not use naked use in the tests. though its fine to use for clojure.test, but not the others 2) can you please put a detailed testing around the test case to explain in detail what is being tested here? There is quite a lot going on all for one test case, and it is not readily apparent to the reader what is being tested.

[ceterumnet]

@AlexBaranosky 1) I agree, but since we test with the Clojure 1.3 profile...this breaks...I was following the convention already used in connected.clj 2) I've put all the helper functions in a separate file - I'm thinking there is probably an opportunity to refactor the tests a bit more in the future, but I'd like to get this particular fix wrapped up.

[immoh]

1) I didn't quite understand why it breaks with 1.3 profile? So instead of (:use korma.core) you should use (:use [korma.core :only [defentity select ...]]) For clojure.test, naked use is ok. 2) Very good idea.

I also noticed that you had deftest inside testing. Should be the other way around, deftest on the top level.

Next time you squash the commits, please clean the commit message so that it doesn't contain all the commit messages from previous commits. Thanks!

[ceterumnet]

1) I was using require :refer :all at first - since this is the preferred "clojure way" - but that doesn't work with 1.3 - so instead I refactored the naked :use to simply be require statements which makes sense to me since this is an integration test.

The deftest and testing are reversed.

The last commit message should cleaned up - I am using Magit and didn't notice that it was chaining all the messages for me...

[immoh]

Now you have assoc-db-to-entity defined three times, please fix. Otherwise this is fine from my point of view. @AlexBaranosky How about you?

[ceterumnet]

Doh! Fixed.