[FIX #2565] data-store: escape leading tildes in message content

[vitvly]

Per Transit documentation, leading tildes have to be escaped, otherwise they are treated as tagged values.

[pablanopete]

Hi @siphiuel thanks for your contribution! One of our team members will review shortly - in the meantime are you already part of the conversation on our Riot? I'd like to invite you at https://chat.status.im/#/register and ping me @hutch when you join! Thanks.

[jeluard]

Please add tests to validate this behavior.

[jeluard]

Looks like ~ is not the only special character. Also we probably want to do this generically, not only for this particular field? @

[vitvly]

Hi!

Thank you for reviewing!

Made the following changes:

1. Added tests in status-im.test.data-store.realm.core namespace
2. The function that does the escaping (prepare-for-transit) is moved to status-im.data-store.realm.core and recursively walks through all string fields in a data structure that is saved to Realm.
3. Additional special characters are taken into account.

I did a force push and some of your comments (the ones regarding items #2 and #3) seem to be gone. Sorry for that.

[jeluard]

Thanks! It's not clear to me if escaped characters will be deserialized correctly. Could we add a round-trip test that would validate the the full serialize / deserialize logic?

[vitvly]

Hi! There's a round-trip test at line #25:

(is (= data (-> prepared-data
                      clj->js
                      internal-convert)))

It checks where Clojure data structure survives the conversion in both directions intact. Do you think it is enough? Or should I add more tests of that kind?

[jeluard]

Great I missed it!

[rasom]

We could use postwalk, then no need to use recursion inside walk-fn.
Something like

(letfn [(walk-fn [e]
            (cond->> e
                     (to-be-escaped? e)
                     (str escape-char)))]
    (walk/postwalk walk-fn message))

But this is just a suggestion

[rasom]

Please fix trailing parenth https://github.com/bbatsov/clojure-style-guide#gather-trailing-parens

[rasom]

No need for this check, i mean (first e)

[rasom]

Please extract this set of characters which should be escaped to a constant.

[rasom]

Please extract magic value ("~") to constant

[rasom]

Also would be great to extract all these predicates to separate function

[rasom]

Please align values in maps and let forms

[rasom]

What is the reason for not using core/prepare-for-transit directly?

[rasom]

See comments above

[vitvly]

Hi, thanks for reviewing!

Made the requested changes, details below:

1. walk->postwalk: agree, looks simpler.
2. Trailing parens: removed.
3. Extra check for (first e): removed.
4. Extracted special char values to constants, created a predicate fn.
5. Not sure if I got the alignment properly in the test fn.
6. Referring to private functions by vars: in Clojure private ns functions can only be referred to from outside via vars, but it seems that in Clojurescript there is no such restriction?

[rasom]

Referring to private functions by vars: in Clojure private ns functions can only be referred to from outside via vars, but it seems that in Clojurescript there is no such restriction?

I just haven't noticed that function is "private". Yeah it doesn't work in cljs and tbh i see no reason for using defn-, but that's not a big problem.

Not sure if I got the alignment properly in the test fn.

We usually align let-binding values, so it should look like

(let [data          {:to-be-escaped1 "~bad string"
                     :to-be-escaped2 "^another bad string"
                     :to-be-escaped3 "`and another bad string"
                     :no-escaping    "no escaping"
                     :vector-content ["a" "b" "c"]}
      prepared-data (core/prepare-for-transit data)]
  ...)

not

(let [data {:to-be-escaped1 "~bad string"
            :to-be-escaped2 "^another bad string"
            :to-be-escaped3 "`and another bad string"
            :no-escaping    "no escaping"
            :vector-content ["a" "b" "c"]}
      prepared-data (core/prepare-for-transit data)]
  ...)

[vitvly]

Got it, made the changes.

[vitvly]

I can squash if there are no other changes to be done.

[rasom]

@siphiuel, yes please squash commits

[vitvly]

Done.