cognitect / transit-cljs

Transit for ClojureScript
http://transit-format.org
Apache License 2.0
323 stars 20 forks source link

Data is being attached to the wrong keys on read #53

Closed ckirkendall closed 4 months ago

ckirkendall commented 4 years ago

I am reading the same file in through clojure and clojurescript and in reading the clojurescript data is being tied the wrong keys. I have included an example and the transit file in question to allow anyone to reproduce the issue.

ClojureScript REPL

development:cljs.user=> (require '[cljs-node-io.core :as io :refer [slurp spit]])
                   #_=> (require '[cognitect.transit :as ct])
                   #_=> (def tmp
                   #_=>      (let [ins (slurp "/Users/ckirkendall/Development/gr/one-loan-client/resources/trans-ast.transit")
                   #_=>                rdr (ct/reader :json)]
                   #_=>        (ct/read rdr ins)))
                   #_=> (get-in tmp [:loanProductData :children :prepaymentPenalties])
nil
nil
#'cljs.user/tmp
{:type :leaf, :field [:1946], :field-type "string"}

Clojure REPL

user=> (require '[clojure.java.io :as io])

user=> (require '[cognitect.transit :as ct])
nil
user=> (def tmp
  #_=>      (let [ins (io/input-stream (io/file "/Users/ckirkendall/Development/gr/one-loan-client/resources/trans-ast.transit"))
  #_=>                rdr (ct/reader ins :json)]
  #_=>        (ct/read rdr)))
#'user/tmp
user=> (get-in tmp [:loanProductData :children :prepaymentPenalties])
{:type :array, :children [{:idx 1, :children {:fullPrepaymentPenaltyOptionType {:type :leaf, :field [:1971], :field-type "string"}, :termMonthsCount {:type :leaf, :field [:1972], :field-type "int"}, :prepaymentPenaltyPercent {:type :leaf, :field [:1973], :field-type "decimal"}}} {:idx 2, :children {:fullPrepaymentPenaltyOptionType {:type :leaf, :field [:1974], :field-type "string"}, :termMonthsCount {:type :leaf, :field [:1975], :field-type "int"}, :prepaymentPenaltyPercent {:type :leaf, :field [:1976], :field-type "decimal"}}} {:idx 3, :children {:fullPrepaymentPenaltyOptionType {:type :leaf, :field [:1977], :field-type "string"}, :termMonthsCount {:type :leaf, :field [:1978], :field-type "int"}, :prepaymentPenaltyPercent {:type :leaf, :field [:1979], :field-type "decimal"}}} {:idx 4, :children {:fullPrepaymentPenaltyOptionType {:type :leaf, :field [:1980], :field-type "string"}, :termMonthsCount {:type :leaf, :field [:1981], :field-type "int"}, :prepaymentPenaltyPercent {:type :leaf, :field [:1982], :field-type "decimal"}}}]}

Here is the file in question so this issue can be reproduced: trans-ast.transit.zip

puredanger commented 4 years ago

I'm not able to reproduce this - I get the same data in cljs that I get in clj.

puredanger commented 4 years ago

Shadow setup:

$ cat shadow-cljs.edn
;; shadow-cljs configuration
{:source-paths
 ["src/dev"
  "src/main"
  "src/test"]

 :dependencies
 [[com.cognitect/transit-cljs "0.8.256"]
  [cljs-node-io "1.1.2"]]

 :builds
 {}}

At shadow repl:

npx shadow-cljs node-repl
;; blah blah blah

cljs.user=> (require '[cljs-node-io.core :as io :refer [slurp spit]])
nil
cljs.user=> (require '[cognitect.transit :as ct])
nil
cljs.user=> (def d1 (let [ins (slurp "./trans-ast.transit") rdr (ct/reader :json)] (ct/read rdr ins)))
#'cljs.user/d1
cljs.user=> (-> d1 :loanProductData :children :prepaymentPenalties)
{:type :array, :children [{:idx 1, :children {:fullPrepaymentPenaltyOptionType {:type :leaf, :field [:1971], :field-type "string"}, :termMonthsCount {:type :leaf, :field [:1972], :field-type "int"}, :prepaymentPenaltyPercent {:type :leaf, :field [:1973], :field-type "decimal"}}} {:idx 2, :children {:fullPrepaymentPenaltyOptionType {:type :leaf, :field [:1974], :field-type "string"}, :termMonthsCount {:type :leaf, :field [:1975], :field-type "int"}, :prepaymentPenaltyPercent {:type :leaf, :field [:1976], :field-type "decimal"}}} {:idx 3, :children {:fullPrepaymentPenaltyOptionType {:type :leaf, :field [:1977], :field-type "string"}, :termMonthsCount {:type :leaf, :field [:1978], :field-type "int"}, :prepaymentPenaltyPercent {:type :leaf, :field [:1979], :field-type "decimal"}}} {:idx 4, :children {:fullPrepaymentPenaltyOptionType {:type :leaf, :field [:1980], :field-type "string"}, :termMonthsCount {:type :leaf, :field [:1981], :field-type "int"}, :prepaymentPenaltyPercent {:type :leaf, :field [:1982], :field-type "decimal"}}}]}
ckirkendall commented 4 years ago

I worked through this with @swannodette and he was able to reproduce the error. We determined it was me overflowing the key cache in transit-js. I suspect it has been corrected. I was able to get around the issue by using verbos vs json.

aiba commented 4 months ago

This happened to me when I def'ed a transit/reader and then reused the same reader multiple times. I switched to creating a new transit/reader for each read, and the problem went away.

Is it invalid to reuse transit readers/writers? Is there a performance consideration? I wasn't able to find anything on this in the documentation.

In paticular, if a web client is speaking to a server over HTTP/transit, should the client reuse the same reader/writer for the communication, or construct a new reader/writer for each request/response?

Either way, it's a little concerning that this library would read without warnings and produce incorrect data. It's such a fundamental library for web/server communication!

swannodette commented 4 months ago

@aiba there hasn't been a duplicate report about this issue in 5 years - that doesn't mean there isn't a bug. But also it's not clear that your problem is the same as the reported one. If you could create a simple reproduction that would be helpful.

aiba commented 4 months ago

Fair enough! I'll work on a reproducable example. (It's tough because this only seems to happen after a very large amount of data has been communicated between a web client and web server, all reusing the same transit/reader on the client. Eventually a transit/read produces a very weird output with certain maps having the wrong keys. I'll work on narrowing it down, though.)

In the meantime, would you be able to shed some light on when (if ever) one ought to reuse the same transit/reader for across multiple calls to transit/read?

swannodette commented 4 months ago

@aiba in the case of transit-js I believe you should be able to reuse a reader and a writer.

aiba commented 4 months ago

After further investigation, I'm pretty sure the cause in my case was that a custom transit read handler was itself calling transit/read with the same (reused) reader as the outer transit/read. (We have a situation where certain values are double-transit-encoded so we had a read-handler that unwrapped them.)

Now all is working as long as, for any given reader or writer, we only do one call to transit/read or transit/write at the same time.

Sorry for the false alarm here, and @swannodette thank you so much for the clarification about reusing readers and writers.

(Is there now any reason not to close this issue?)

swannodette commented 4 months ago

@aiba phew :) you had me sweating. I tried many times to get the cache to fail with a large number of keys and large payloads but I could never recreate anything. I agree we can close this for now until someone comes up with a true minimal case.