clsql icon indicating copy to clipboard operation
clsql copied to clipboard

sqlite3 auto-increment slots in ooddl not working

Open u-u-h opened this issue 8 years ago • 2 comments

I've tried to use auto-increment slots on the sqlite3 backend, and they seem to be working only partially. The following code is not working:

(ql:quickload "clsql")`
(ql:quickload "clsql-sqlite3")
(clsql:def-view-class foo ()
       ((id :accessor id :initarg :id :type integer
       :db-constraints (:not-null :auto-increment))))
(defun test ()
  (clsql:with-database (clsql:*default-database* '("foo.sqlite3") :database-type :sqlite3)
    (let ((clsql:*db-auto-sync* t))
      (ignore-errors
        (clsql:create-view-from-class 'foo))
      (let ((x (make-instance 'foo)))
        ;; this will fail -- error: The slot CL-USER::ID is unbound in the object #<FOO {100BF60C03}>.                        
        (id x)))))

(test)
=> error: The slot CL-USER::ID is unbound in the object #<FOO {100BF60C03}>

It seems that the issue has been mentioned on the mailing list in 2010 http://article.gmane.org/gmane.lisp.clsql.general/1221 and traces of the patch suggested back then (before code refactoring) seem to be around, yet behavior is wrong.

I'm struggling to suggest an appropriate fix. My analysis is that update-autoincrement-keys is not called, since %update-instance-helper decides the slot is not one that needs updating. That is a decision apparently made at view-class construction time. I can't currently test against a different backend to see whether (and why) it works there.

Any hints? I'm ready to try and fix it with some guidance.

(for the record: this is on OSx, sbcl 1.3.3, quicklisp's march release, which contains clsql from kpe.io master branch in january 2016 AFAICS)

uuh

u-u-h avatar Apr 07 '16 08:04 u-u-h

Looking into this further, it seems that with the stripped down view class above update-records-from-instance does not find any slot worth saving (view-classes-and-storable-slots returns NIL -- due to storable-slots in view-classes-and-storable-slots explicitly dropping them). Thus, %update-instance-helper is not even called once in the loop. This seems like it should not work with any backend. Now, one might argue that a view-class with only an auto-increment slot is weird, but the problem also manifests if I add another slot:

(clsql:def-view-class foo2 ()
  ((id :accessor id :initarg :id :type integer
       :db-constraints (:not-null :auto-increment))
  (bar :accessor bar :initarg :bar :type integer)))

Now, if I do (make-instance 'foo2) the bar slot remains unbound, which will let %update-instance-helper bail out early due to ATTRIBUTE-VALUE-PAIRS saying there are no updates. That leaves the auto-increment slot unbound too. (This situation might be considered a user-error; but having an unbound slot in a view-class instance with auto-commit turned on might as well raise an error I think).

If I initialize slot bar, e.g. (make-instance 'foo2 :bar 21) it starts working. Things are more subtle here, though. update-auto-increment-keys is called, but keyslots-for-class consider the id slot a keyslot only I set :db-kind :key. Is it intentional that auto-increment columns always have to be KEYs? I can live with it for now, since I changed to denormalized schemata in my application (The reason was that mixing :db-kind :key and :auto-increment t and :normalized t gave me SQL syntax errors from sqlite3 -- I'll see if I can reproduce that in a separate issue).

Summary:

  1. It seems that a view-class object must have at least one :key or :base slot besides the :auto-increment slot to have that updated correctly. working.
  2. Failing to initialize a slot with clsql:*db-auto-sync* turned on will not raise an error, and may keep the object from being synced to the database; that may include not creating the object at all in the database.

uuh

u-u-h avatar Apr 07 '16 12:04 u-u-h

As you are discovering, there is a lot of silent "by convention" semantics in CLSQL. Its large and complex and has lots of cases like this where things don't work as expected if they are not used as expected ("expected" being how the authors thought).

If you add warnings or errors as patches, or figure out something to add to the documentation that will help, I will gladly merge them.

bobbysmith007 avatar Apr 07 '16 15:04 bobbysmith007