korma / Korma

Tasty SQL for Clojure.
http://sqlkorma.com
1.48k stars 222 forks source link

Fix lazy namespace resolution of relations. #283

Closed juhovh closed 9 years ago

juhovh commented 9 years ago

Earlier the relations used lazy entity resolving from the current namespace, but they should be resolve the entity from the original namespace instead. Otherwise it is possible that if an entity is created dynamically in a different namespace than where it is used, it fails to resolve the referred entities.

For more information and a test case see the mailing list post at https://groups.google.com/forum/#!topic/sqlkorma/Qfr7szJUFdo

immoh commented 9 years ago

The fix is fine, but please add a failing test case that gets fixed by this. Thanks!

juhovh commented 9 years ago

Added a failing test case, as mentioned this is a bit tricky to test since I need to add an additional namespace for this.

immoh commented 9 years ago

I don't think it is especially tricky, you need another namespace to test this. It could be done in one file but I think it's fine to do it like this.

By the way, I think the issue can be reproduced in a simpler way by using defentity in two different namespaces:

If you have

(ns a
  (:use korma.core))

(defentity foo)

and

(ns b
  (:use korma.core)
  (:require a))

(defentity bar 
  (has-one a/foo))

then

(select bar (with a/foo))

will throw.

Could you squash both fix and test into single commit? Thanks.

juhovh commented 9 years ago

Moved the friend entity to core namespace, changed the test to use the definitions as suggested and squashed it all into single commit.

immoh commented 9 years ago

Thanks!

juhovh commented 9 years ago

There's quite many changes since release 0.4.0 and I'm keeping a custom korma binary in my repository to make the code work, any plans for the next release?

immoh commented 9 years ago

I'm planning to cut a release by the end of the week.

juhovh commented 9 years ago

Cool :+1: