Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Records can't be serialised and deserialised by Conjure #40

Closed
Olical opened this issue Jun 5, 2019 · 14 comments
Closed

Records can't be serialised and deserialised by Conjure #40

Olical opened this issue Jun 5, 2019 · 14 comments
Assignees
Labels
bug Something isn't working

Comments

@Olical
Copy link
Owner

Olical commented Jun 5, 2019

mynomoto over on Slack pointed out that if you try to return a record from an eval Conjure explodes.

(defrecord Foo [bar])
(->Foo "bar")

Because that evaluates to the string

#conjure.code.Foo{:bar \"bar\"}

Which Conjure tries to read because it reads all prepl responses (for better or worse...). I think the main need to read responses is because I want to check if it was an error or not. That will go away when I overhaul how errors and exceptions work.

Would be cool to fix this ASAP, but if you hit this before then just (into {} r) where r is your record. It'll get around this issue until I can patch it out or it vanishes on it's own when I improve errors.

@Olical Olical added the bug Something isn't working label Jun 5, 2019
@Olical Olical self-assigned this Jun 5, 2019
@Olical
Copy link
Owner Author

Olical commented Jun 5, 2019

Patch released in v0.17.1 to mitigate this, it'll show you the parse error and not kill the connection. Full fix will come with the errors / exceptions rework I think..

@Olical
Copy link
Owner Author

Olical commented Jun 19, 2019

Error improvement work is now waiting for this patch to go into ClojureScript https://clojure.atlassian.net/browse/CLJS-3096

Once that's in I can rewrite how the error handling system works which means I don't need to parse output from prepl evaluations which means this error will vanish. This will make Conjure a lot more robust too, right now there's a few cases where returning something out of the ordinary will cause it to choke.

If you hit one of these errors, I'm really sorry. You'll just have to pour the data into Clojure data structures or use bean until this is patched, hopefully it won't be too long!

@justone
Copy link
Contributor

justone commented Jun 25, 2019

I think I'm running into this with Manifold streams. They have a custom print-method that returns this:

<< stream: {:pending-puts 0, :drained? false, :buffer-size 0, :permanent? false, :type "manifold", :sink? true, :closed? false, :pending-takes 0, :buffer-capacity 0, :source? true} >>

and causes Conjure to report "Invalid symbol: stream:.".

No rush, just wanted to add another data point.

Thanks for making Conjure, I enjoy using it every day.

@Olical
Copy link
Owner Author

Olical commented Jun 25, 2019

Ah damn, I'm sorry about that 😭 try pouring it into a map or bean, it might get around the printing temporarily. I'm still waiting on my patch going into ClojureScript, maybe I can do a halfway house soon to fix the Clojure side at least.

Once that CLJS patch is in I won't have to parse anything returned from the prepl which will make things a lot easier. I'll see what I can do to mitigate it until the proper fixes are possible 🤔

@daveyarwood
Copy link
Sponsor Contributor

I'm also being hit by this in a project I'm working on.

I did find a workaround: if you find yourself needing to evaluate a form where you know the result is a record, you can wrap it with (into {} the-form) This turns the record into a plain map.

There is still a problem if you want to evaluate a form that contains records (e.g. a map where the values include records). I found a solution for this on StackOverflow: (clojure.walk/postwalk #(if (record? %) (into {} %) %) the-form)

It's not ideal, but I think it will tide me over until a fix comes down the pike :)

@Olical
Copy link
Owner Author

Olical commented Jun 27, 2019

I've got an idea for how I could potentially work around this until ClojureScript prepl hits feature parity with Clojure prepl 🤔 I'll see what I can do ASAP, just got talk planning to do for next Wednesday.

@Olical
Copy link
Owner Author

Olical commented Jun 27, 2019

Testing Slack integration...

@justone
Copy link
Contributor

justone commented Jun 27, 2019

@daveyarwood great idea to use clojure.walk. Here's my augmented version that handles manifold streams:

(defn nr
  [form]
  (clojure.walk/postwalk
    #(cond
       (record? %) (into {} %)
       ((supers (class %)) manifold.stream.core.IEventStream) (pr-str %)
       :else %)
    form))

@Olical Olical mentioned this issue Jul 4, 2019
3 tasks
@Olical
Copy link
Owner Author

Olical commented Jul 4, 2019

So when someone who's experienced this has some free time, would you be able to try it again but on the result-refactor branch? #47

I'll try to repro too, but it would be great if you could try to do exactly what you did before that caused Conjure to explode. I'm hoping my changes have fixed it, if not I think a slight tweak will be all that's needed after that refactor work.

Thanks a lot!

@daveyarwood
Copy link
Sponsor Contributor

A simple repro case is to evaluate the following using your project's prepl:

;; this works because the return value is the var `my-namespace.Foo`
(defrecord Foo [])

;; this blows up because the return value is a record
(->Foo)

@daveyarwood
Copy link
Sponsor Contributor

I just tried the result-refactor branch, and the issue appears to be fixed!

2019-07-04-162212_322x45_scrot

I also tried it out on the work project where I ran into this issue before, and it's fixed there too 👍

@Olical
Copy link
Owner Author

Olical commented Jul 5, 2019

Yessss, thank you for checking, that's great news. I'll wrap up the rest of that PR and get it out in a tagged release.

@justone
Copy link
Contributor

justone commented Jul 5, 2019

The issue is fixed for me as well. Thanks!

@Olical Olical closed this as completed Jul 5, 2019
@Olical
Copy link
Owner Author

Olical commented Jul 5, 2019

So this is fixed in v0.21.0 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants