weavejester / medley

A lightweight library of useful Clojure functions
Eclipse Public License 1.0
865 stars 66 forks source link

`assoc-some` broken for custom types after version 1.5 #91

Closed Aeonax closed 2 months ago

Aeonax commented 2 months ago

It appears that this commit introduces a breaking change. After updating from version 1.4.0, we encounter an error when attempting to use assoc-some on custom types:

java.lang.ClassCastException: class aleph.http.core.NettyRequest cannot be cast to class clojure.lang.IEditableCollection (aleph.http.core.NettyRequest is in unnamed module of loader clojure.lang.DynamicClassLoader @eb87c61; clojure.lang.IEditableCollection is in unnamed module of loader 'app')
        at clojure.core$transient.invokeStatic(core.clj:3343)
        at clojure.core$transient.invoke(core.clj:3343)
        at medley.core$assoc_some.invokeStatic(core.cljc:48)
        at medley.core$assoc_some.doInvoke(core.cljc:43)
        at clojure.lang.RestFn.invoke(RestFn.java:1843)
        at hqlive.system.routes$client_data_middleware$fn__61803.invoke(routes.clj:22)
        at ring.middleware.cookies$wrap_cookies$fn__42590.invoke(cookies.clj:214)
        at clojure.lang.AFn.applyToHelper(AFn.java:154)
        at clojure.lang.AFn.applyTo(AFn.java:144)
        at clojure.lang.AFunction$1.doInvoke(AFunction.java:31)
        at clojure.lang.RestFn.invoke(RestFn.java:408)
        at ring.middleware.cors$handle_cors.invokeStatic(cors.cljc:175)
        at ring.middleware.cors$handle_cors.invoke(cors.cljc:167)
        at ring.middleware.cors$wrap_cors$fn__42697.invoke(cors.cljc:205)
        at aleph.http.server$handle_request$fn__17373$f__3126__auto____17374.invoke(server.clj:170)
        at clojure.lang.AFn.run(AFn.java:22)
        at io.aleph.dirigiste.Executor$Worker$1.run(Executor.java:62)
        at manifold.executor$thread_factory$reify__3004$f__3005.invoke(executor.clj:70)
        at clojure.lang.AFn.run(AFn.java:22)
        at java.base/java.lang.Thread.run(Thread.java:1583)

I didn't notice any existing issues regarding this "bug" or mentions somewhere and I'm not entirely sure if it's appropriate to classify it as a bug.😅

tomdl89 commented 2 months ago

Oops @Aeonax that was me. Sorry for the breakage. @weavejester I've got a branch that defaults to the old behaviour if the collection isn't editable. lmk what you think.

weavejester commented 2 months ago

Thanks @tomdl89 for raising the issue and @Aeonax for putting together a fix.

Can we take out the partition to avoid generating another lazy seq? e.g.

   (if (editable? m)
     (loop [acc (assoc-some-transient! (transient (or m {})) k v)
            kvs kvs]
       (if (next kvs)
         (recur (assoc-some-transient! acc (first kvs) (second kvs)) (nnext kvs))
         (if (zero? (count acc))
           m
           (persistent! acc))))
     (loop [acc (assoc-some m k v), kvs kvs]
       (if (next kvs)
         (recur (assoc-some acc (first kvs) (second kvs)) (nnext kvs))
         acc)))
tomdl89 commented 2 months ago

Other way round, but yup, no worries. I've added a PR using a loop to avoid the intermediate lazy-seq.

Aeonax commented 2 months ago

Any thoughts on when it will be released?

weavejester commented 2 months ago

I've released 1.8.1 with the fix.

Aeonax commented 1 month ago

By the way, just noticed that meta is lost after "default" assoc-some, is it a known issue?

tomdl89 commented 1 month ago

I don't think it is a known issue @Aeonax. I've raised a PR to fix. I don't think https://github.com/weavejester/medley/issues/13 is relevant, as it is comparing to map and co (which don't preserve metadata) whereas assoc-some should be compared to assoc which does.