search

korma / Korma

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

Insert issue after commit 6299297 #341

Closed ejschoen closed 8 years ago

ejschoen commented 8 years ago

For what it's worth, I think this change is creating an insert issue for me. Sorry that this isn't a particularly concrete bug report with a repro... I'll see if I can gen one up. In the mean time, what I am seeing is that when the values to insert come from a for block applied to some other data, the resulting insert statement is corrupt. For example, I am trying to insert into a many-to-many association table. The data to be inserted comes from:

(insert :docs_semantic_units (values foo))

where foo is bound to:

(for [semunit-id semunits-id] 
   {:docs_id docid :semantic_units_id semunit-id :clients_id clientid})

The resulting insert statement is:

INSERT INTO docs_semantic_units (clients_id, docs_id, semantic_units_id) VALUES (?, ?, ?), (?, ?, NULL)

I then get a SQL error because I'm trying to insert NULL into a non-nullable column. I was curious if something was wrong with the insert values, so I inserted a (pprint foo) into the code, and the error and corrupt statement vanish. So, something to do with an unrealized lazy seq? (But testing foo for realized? returned true).

Thanks, Eric

immoh commented 8 years ago

Commit c62b6cd hasn't been released yet, are you building Korma from master yourself?

It is difficult to say what's wrong based on this information. I would first inspect the data produced by for, it looks like semunit-id is nil but of course this is just guessing.

ejschoen commented 8 years ago

I was building from master. Really just an experiment. I had wanted to add the ability to provide limit and offset arts to union (and union-all and intersect), which was trivial to do.

The data provided in the values call was correct. As I mentioned, all it took was printing the value that came from the for expression to make the insert work right. So I'm thinking it has something to do with being a lazy seq, but it's not clear why. A little more detail: the data that the for is iterating over comes from a prior select query. So, if there is some laziness involved, it might be the effect of generating values for the insert on data that hasn't yet been realized from the prior query. But it's still not clear to me why the code change in the commit would trigger this. (The behavior I'm observing is from our integration test suite, which runs flawlessly on released version 0.4.2.)

I'll try to make a reproducible case.

Thanks, Eric

On Sunday, January 24, 2016, Immo Heikkinen notifications@github.com wrote:

Commit c62b6cd https://github.com/korma/Korma/commit/c62b6cd665c01dfb7579ee937cc4baf5268ba84e hasn't been released yet, are you building Korma from master yourself?

It is difficult to say what's wrong based on this information. I would first inspect the data produced by for, it looks like semunit-id is nil but of course this is just guessing.

— Reply to this email directly or view it on GitHub https://github.com/korma/Korma/issues/341#issuecomment-174265992.

immoh commented 8 years ago

Yeah, I can reproduce with data that comes from select query that has has-many relation. Interesting.

ejschoen commented 8 years ago

It's as if the lazy seq isn't fully realized when the insert statement gets formed, but if so, this breaks my model of how Clojure lazy seqs work.

I was using an H2 DB for testing. However, if this had been a .NET environment and I had been using LINQ with entity framework, where queries are lazy, too, I would have recognized this as a multiple active result sets problem.

Thanks for looking into it. Eric

On Monday, January 25, 2016, Immo Heikkinen notifications@github.com wrote:

Yeah, I can reproduce with data that comes from select query that has has-many relation. Interesting.

— Reply to this email directly or view it on GitHub https://github.com/korma/Korma/issues/341#issuecomment-174420347.

immoh commented 8 years ago

I added a failing test for this, it can be found in branch https://github.com/korma/Korma/tree/issue_341_insert_bug. It gets even more weird, the test is supposed to insert 1 row but the error message shows that it is trying to insert 16 rows:

ERROR in (insert-based-on-select-with-relation) (DbException.java:345)
expected: (not (nil? (insert email (values data))))
  actual: org.h2.jdbc.JdbcSQLException: NULL not allowed for column "user_id"; SQL statement:
INSERT INTO "email" ("email_address", "user_id") VALUES (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL) [23502-187]

I guess that has something to do with lazy sequence chunk size.

The breaking commit is actually 62992975b60b45c6d0e07974fd2cddb4ce826eff (PR #330), not the one that you assumed.

ejschoen commented 8 years ago

Thanks! Your test failure is amazing weird. I wonder if it’s a JDBC behavior impacting Clojure.

Eric

On Jan 27, 2016, at 1:27 AM, Immo Heikkinen notifications@github.com wrote:

I added a failing test for this, it can be found in branch https://github.com/korma/Korma/tree/issue_341_insert_bug https://github.com/korma/Korma/tree/issue_341_insert_bug. It gets even more weird, the test is supposed to insert 1 row but the error message shows that it is trying to insert 16 rows:

ERROR in (insert-based-on-select-with-relation) (DbException.java:345) expected: (not (nil? (insert email (values data)))) actual: org.h2.jdbc.JdbcSQLException: NULL not allowed for column "user_id"; SQL statement: INSERT INTO "email" ("email_address", "user_id") VALUES (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL), (NULL, NULL) [23502-187] I guess that has something to do with lazy sequence chunk size.

The breaking commit is actually 6299297 https://github.com/korma/Korma/commit/62992975b60b45c6d0e07974fd2cddb4ce826eff (PR #330 https://github.com/korma/Korma/pull/330), not the one that you assumed.

— Reply to this email directly or view it on GitHub https://github.com/korma/Korma/issues/341#issuecomment-175462755.

immoh commented 8 years ago

It is not JDBC issue. Adding this to the test case:

(prn (:values (query-only (insert email (values data)))))

outputs:

{:user_id nil, :email_address nil} {:user_id nil, :email_address nil} {:user_id nil, :email_address nil} {:user_id nil, :email_address nil} {:user_id nil, :email_address nil} {:user_id nil, :email_address nil} {:user_id nil, :email_address nil} {:user_id nil, :email_address nil} {:user_id nil, :email_address nil} {:user_id nil, :email_address nil} {:user_id nil, :email_address nil} {:user_id nil, :email_address nil} {:user_id nil, :email_address nil} {:user_id nil, :email_address nil} {:user_id nil, :email_address nil} {:user_id nil, :email_address nil}]

which means the data gets corrupted somewhere in Korma.

immoh commented 8 years ago

Ok, I think I finally understand what's going on. Modifying the test to:

(deftest insert-based-on-select-with-relation
  (let [data (for [user-with-email (select user (with email))
                   [k v] (:email user-with-email)]
               {k (str v)})]
    ;; This makes the test pass
    ;; (prn data)
    (is (not (nil? (insert email (values data)))))

makes it fail with the following error message:

ERROR in (insert-based-on-select-with-relation) (DbException.java:345)
expected: (not (nil? (insert email (values data))))
  actual: org.h2.jdbc.JdbcSQLException: Column "alias" not found; SQL statement:
INSERT INTO "email" ("alias", "aliases", "db", "ent", "fields", "from", "group", "joins", "modifiers", "order", "params", "results", "sql-str", "table", "type", "where") VALUES (NULL, NULL, NULL, ?, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL), (NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, ?), (NULL, NULL, NULL, NULL, NULL, NULL, ?, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL), (NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, ?, NULL, NULL), (NULL, NULL, ?, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL), (NULL, NULL, NULL, NULL, ?, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL), (NULL, NULL, NULL, NULL, NULL, NULL, NULL, ?, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL), (NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, ?, NULL, NULL, NULL, NULL, NULL), (NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, ?, NULL), (?, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL), (NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, ?, NULL, NULL, NULL, NULL, NULL, NULL, NULL), (NULL, NULL, NULL, NULL, NULL, ?, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL), (NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, ?, NULL, NULL, NULL, NULL, NULL, NULL), (NULL, ?, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL), (NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, ?, NULL, NULL, NULL), (NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, ?, NULL, NULL, NULL, NULL) [42122-187]

This reveals that the value of :email key is the query map for selecting emails for user instead of rows from email table. This is caused by change 6299297 which binds :query to exec-mode so that queries can be composed, unfortunately this doesn't play well with lazy sequences. (http://cemerick.com/2009/11/03/be-mindful-of-clojures-binding/ is a very good blog post about it.)

The fix is to revert commit 6299297.