TimeZynk / mongo

A clojure wrapper for the com.mongodb.client Java API.
MIT License
1 stars 0 forks source link

Cannot insert an empty collection #16

Closed eureton closed 4 months ago

eureton commented 6 months ago
=> (require '[com.timezynk.mongo :as mongo2])
nil
=> (mongo2/with-mongo "mongodb://mongodb-test/tzbackend" (mongo2/insert! :coll []))
Execution error (IllegalArgumentException) at com.mongodb.assertions.Assertions/isTrueArgument (Assertions.java:101).
state should be: writes is not an empty list

The expected result above is an empty sequence.

lars-timezynk commented 6 months ago

I take it we have three special cases:

  1. Inserting en empty array,
  2. Inserting a null value,
  3. Inserting a non-document.

Right now, inserting a null returns null, nothing is added to the database. Inserting a non-document throws an exception.

Are there cases for different responses?

eureton commented 6 months ago

Are there cases for different responses?

I can't think of any at the moment. However, let's keep the design open. It is more likely than not that we have to handle another such case in the future, e.g. document which fulfills criteria XYZ throws exception ABC.

lars-timezynk commented 6 months ago

PR: https://github.com/TimeZynk/mongo/pull/17

I changed my mind in one regard: null values should be accepted as little as possible, since they indicate errors.

Update guard only accepts payload, not query since it felt unnecessary.

eureton commented 6 months ago

null values should be accepted as little as possible, since they indicate errors.

I think the API should be modeled after Clojure idioms, where nil indicates absense of result rather than error. If clojure.core API puns nil, why shouldn't we?

Consider the consequences of rejecting nil. Consumers of this API would be forced to either guard with (when abc (mongo2/insert! ...)) or try / catch.

By example:

(defn create-flop [flip-id]
  (->> flip-id
       (mongo2/fetch-by-id :flips)
       (flip-to-flop)               ; puns nil as per the idiom
       (mongo2/insert! :flops)      ; puns nil as per the idiom
       (render-response)))          ; nil gets rendered as "404 Not Found"

A nil-intolerant insert! would turn this :point_up: simple function into a defensive-programming eyesore :point_down: :

(defn create-flop [flip-id]
  (if-let [entry (->> flip-id (mongo2/fetch-by-id :flips) (flip-to-flop))]
    (->> entry
         (mongo2/insert! :flops)
         (render-response))
    (not-found))
lars-timezynk commented 6 months ago

I disagree. This is not a simple function call, it's an API call.

Furthermore, congomongo treats this not just as an illegal argument exception, but as a null pointer exception. So in order to stay consistent, an exception should be thrown.

lars-timezynk commented 6 months ago

I'm making changes so we can bake our own guard for the payload if we want.

eureton commented 6 months ago

I disagree. This is not a simple function call, it's an API call.

Furthermore, congomongo treats this not just as an illegal argument exception, but as a null pointer exception. So in order to stay consistent, an exception should be thrown.

congomongo sets no standards. Certainly not any we want to live up to, anyway. Migrating from congomongo to this library will be the same amount of work, no matter which way this decision goes.

I argue against exceptions because they break the FP paradigm. In the FP world, functions are given values and return values back. Even better - when the function is pure, referential transparency makes reasoning simple. Exceptions violate this by interrupting the call chain - a function which throws never returns! Worse still, it lands you in some catch block far away. Exceptions are a holdover from the OOP world which Clojure only reluctantly adopted.

My case has been made. The final decision rests with you.

lars-timezynk commented 6 months ago

What about update, update-one, fetch-and-update-one, replace-one, fetch-and-replace-one?

eureton commented 6 months ago

What about update, update-one, fetch-and-update-one, replace-one, fetch-and-replace-one?

First thing to note is that these are variations of the same thing: update. Therefore, I expect that we treat them equally.

Unlike insert, these ingest not data, but MongoDB DSL. The principle of punning nil as absence-of-data applies fine to a document handed to insert but not to a MongoDB queries.

Let's examine this from a client-code perspective. When handing a document to insert, it is natural to expect that document to have gone through a data-processing pipeline. As a result, it is natural to expect that what comes out of that pipeline is nil. Neither the query nor the command parameter of update qualify for such reasoning! The values given there will mostly be Clojure hash-map literals. Furthermore, if we ever put any of those values together in a computation, the result of that computation may never be nil. Therefore, in this situation, nil indeed stands for error and an exception is appropriate.