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

Make string key access fallback to atom key access #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

drathier
Copy link

@drathier drathier commented Jul 5, 2021

Description of the change
With this change Simple.Json.write and Simple.Json.read works round-trip. Without this change, it always encodes record keys as atoms, and it always reads record keys by string, so all record accesses fail. It feels like a safe default to first check exactly what was asked for, and then fall back to atom access. Also, it's unlikely that we'll do record accesses on user input field names, so atom leaks should not be an issue.

Details

class Codec a where
  toSerializable :: a -> Versioned
  fromSerializable :: Versioned -> Either String a

type Versioned
  = { _version :: Int, value :: Foreign }

encodeJSON :: forall a. Codec a => a -> String
encodeJSON a = SimpleJson.writeJSON (toSerializable a)

decodeJSON :: forall a. Codec a => String -> Either String a
decodeJSON a = case SimpleJson.readJSON a of
  Left err -> Left ("codec-decode:" <> show (map renderForeignError err))
  Right versioned -> fromSerializable versioned

{- blocked on https://github.com/purerl/purescript-foreign/pull/1

encodeForeign :: forall a. Codec a => a -> Foreign
encodeForeign a = SimpleJson.write (toSerializable a)

decodeForeign :: forall a. Codec a => Foreign -> Either String a
decodeForeign a = case SimpleJson.read a of
  Left err -> Left ("codec-decode:" <> show (map renderForeignError err))
  Right versioned -> fromSerializable versioned
-}

Json works fine

{:swedishPersonnummer, "19991231-0123"}
iex(62)> jsn = cm.encodeJSON(um.codecUserId(), pnr)          
"{\"_version\":1,\"value\":{\"__type\":\"UserIdV1SwedishPersonnummer\",\"value\":\"19991231-0123\"}}"
iex(63)> res = cm.decodeJSON(um.codecUserId(), jsn)          
{:right, {:swedishPersonnummer, "19991231-0123"}}

but foreign doesn't work:

iex(69)> pnr = um.'SwedishPersonnummer'.("19991231-0123")
{:swedishPersonnummer, "19991231-0123"}
iex(70)> raw = cm.encodeForeign(um.codecUserId(), pnr)          
%{
  _version: 1,
  value: %{__type: "UserIdV1SwedishPersonnummer", value: "19991231-0123"}
}
iex(71)> res = cm.decodeForeign(um.codecUserId(), raw)          
{:left,
 "codec-decode:(NonEmptyList (NonEmpty (ErrorAtProperty \"_version\" (TypeMismatch \"integer\" \"atom\")) Nil))"}
iex(72)> jsn = cm.encodeJSON(um.codecUserId(), pnr)      
"{\"_version\":1,\"value\":{\"__type\":\"UserIdV1SwedishPersonnummer\",\"value\":\"19991231-0123\"}}"
iex(73)> resjsn = cm.decodeJSON(um.codecUserId(), jsn)
{:right, {:swedishPersonnummer, "19991231-0123"}}

until I made the change in the pr, and then it works fine

iex(111)> raw = cm.encodeForeign(um.codecUserId(), pnr)   
%{
  _version: 1,
  value: %{__type: "UserIdV1SwedishPersonnummer", value: "19991231-0123"}
}
iex(112)> rawres = cm.decodeForeign(um.codecUserId(), raw)
{:right, {:swedishPersonnummer, "19991231-0123"}}

Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

With this change `Simple.Json.write` and `Simple.Json.read` works round-trip. Without this change, it always encodes record keys as atoms, and it always reads record keys by string, so all record accesses fail. It feels like a safe default to first check exactly what was asked for, and then fall back to atom access. Also, it's unlikely that we'll do record accesses on user input field names, so atom leaks should not be an issue.
@drathier
Copy link
Author

drathier commented Jul 5, 2021

Do you want me to fill out the form above? Seems like it's mostly for the parent repo

@nwolverson
Copy link
Member

No that's just inherited from the upstream; I'm going to reflect on this a bit, as the right fix may be in simple-json (or in fact both).

Actually it looks like this Index module has accumulated JS cruft since some merge, I probably want to fix that properly also

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants